fix: various trading issues#970
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughBy the Omnissiah: wraps shop rendering in a draw-state lifecycle, replaces rectangle hit-tests and hashed labels with scr_hit/direct strings, adds shift-based x5 bulk purchases, extends scr_add_vehicle with an otherdata payload and updates call sites, adds vehicle trade/deployment flows, rewrites fleet arrival logic, and adds garrison helpers and unit-fetch refactor. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)**/*.gml⚙️ CodeRabbit Configuration File
Files:
**/*.*⚙️ CodeRabbit Configuration File
Files:
🧠 Learnings (1)📚 Learning: 2025-07-01T20:29:44.075ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
scripts/scr_check_equip/scr_check_equip.gml (2)
18-36: Uninitialised unit used in combat branch → null dereference riskWhen obj_ncombat exists, unit is never assigned, yet it is dereferenced later (Lines 38–69). Invoke fetch_unit before the combat/non-combat split to appease the Machine Spirit and avoid void-reference faults.
Apply this patch to initialise unit unconditionally:
- if (!instance_exists(obj_ncombat)){ - unit = fetch_unit([c,i]); + unit = fetch_unit([c,i]); + if (!instance_exists(obj_ncombat)){ if (system!="") and (planet_or_ship_id>0){ if (unit.is_at_location(system,planet_or_ship_id,-1)){ marine_present=true; } } if (system="") and (planet_or_ship_id>0){ if (unit.is_at_location("",0,planet_or_ship_id)){ marine_present=true; } } } else { try { if (obj_ncombat.fighting[c][i]==1){ marine_present=true; } } catch (_){} }
25-27: Assignment used instead of comparison in conditionThe test (system="") assigns an empty string. You want equality. This will misroute logic like a misaligned cog.
Correct with:
- if (system="") and (planet_or_ship_id>0){ + if (system=="") and (planet_or_ship_id>0){scripts/scr_specialist_point_handler/scr_specialist_point_handler.gml (1)
448-455: Capacity desynchronisation: you relocate vehicles after creation without undoing carrier incrementscr_add_vehicle now auto-embarks on carriers when not at home world and capacity exists, incrementing ship_carrying. You then forcibly relocate to a hanger (veh_lid=-1), but do not decrement ship_carrying. The Machine Spirit will tally falsely.
Pass the hanger location up-front to scr_add_vehicle and remove post-hoc field edits:
- repeat(item.count){ - var vehicle = scr_add_vehicle(item.name,obj_controller.new_vehicles); - var build_loc = array_random_element(obj_controller.player_forge_data.vehicle_hanger); - obj_ini.veh_loc[vehicle[0]][vehicle[1]] = build_loc[0]; - obj_ini.veh_wid[vehicle[0]][vehicle[1]] = build_loc[1]; - obj_ini.veh_lid[vehicle[0]][vehicle[1]] = -1; - } + repeat(item.count){ + var build_loc = array_random_element(obj_controller.player_forge_data.vehicle_hanger); + var vehicle = scr_add_vehicle(item.name, obj_controller.new_vehicles, { loc: build_loc[0], wid: build_loc[1], lid: -1 }); + }This avoids touching ship_carrying at all for these builds and keeps state blessedly consistent.
scripts/scr_fleet_functions/scr_fleet_functions.gml (1)
409-419: Critical: Eldar return path uses undefined variables and assigns into wrong identifiers
- nearest_star_with_ownership is called with xx, yy, but the function elsewhere is used as nearest_star_with_ownership(owner). xx/yy are undefined here.
- cur_star=targ.x/y is clearly wrong; you want to set action coordinates.
- targ itself is unused/undefined.
Apply this fix:
- if (owner==eFACTION.Eldar){ - cur_star = nearest_star_with_ownership(xx,yy, eFACTION.Eldar); - if (cur_star!="none"){ - cur_star=targ.x; - cur_star=targ.y; - } - } else { + if (owner == eFACTION.Eldar) { + cur_star = nearest_star_with_ownership(eFACTION.Eldar); + if (cur_star != "none") { + action_x = cur_star.x; + action_y = cur_star.y; + set_fleet_movement(); + } + } else { action_x=home_x; action_y=home_y; set_fleet_movement(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
objects/obj_shop/Draw_0.gml(8 hunks)scripts/scr_add_vehicle/scr_add_vehicle.gml(2 hunks)scripts/scr_check_equip/scr_check_equip.gml(1 hunks)scripts/scr_event_code/scr_event_code.gml(1 hunks)scripts/scr_fleet_functions/scr_fleet_functions.gml(2 hunks)scripts/scr_specialist_point_handler/scr_specialist_point_handler.gml(1 hunks)scripts/scr_trade/scr_trade.gml(1 hunks)scripts/scr_trade_dep/scr_trade_dep.gml(1 hunks)scripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.gml
⚙️ CodeRabbit Configuration File
**/*.gml: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e.#macro ARR_body_parts["arm"]will crash the game, because there is no space between the array and the name of the macro.
- Color codes in the code shouldn't have any spaces in their id. I.e., color code
# 80bf40will crash the game.- All code should comply with the main GML documentation: https://manual.gamemaker.io/beta/en/GameMaker_Language/GML_Reference/GML_Reference.htm
Files:
scripts/scr_specialist_point_handler/scr_specialist_point_handler.gmlscripts/scr_trade/scr_trade.gmlscripts/scr_check_equip/scr_check_equip.gmlscripts/scr_trade_dep/scr_trade_dep.gmlscripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gmlscripts/scr_event_code/scr_event_code.gmlscripts/scr_fleet_functions/scr_fleet_functions.gmlobjects/obj_shop/Draw_0.gmlscripts/scr_add_vehicle/scr_add_vehicle.gml
**/*.*
⚙️ CodeRabbit Configuration File
**/*.*: - Having humanly understandable and maintainable code is always the top most priority.
- DRY (Don't repeat yourself) principle is also very important.
- Ensure that the code is compliant with the CODE_STYLE.MD: https://github.com/Adeptus-Dominus/ChapterMaster/blob/main/docs/CODE_STYLE.md
- If a TODO comment is added, ask the user if you should create a GitHub issue for this TODO.
- If a TODO comment is deleted, remind the user if there is an active GitHub issue related to that comment.
Files:
scripts/scr_specialist_point_handler/scr_specialist_point_handler.gmlscripts/scr_trade/scr_trade.gmlscripts/scr_check_equip/scr_check_equip.gmlscripts/scr_trade_dep/scr_trade_dep.gmlscripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gmlscripts/scr_event_code/scr_event_code.gmlscripts/scr_fleet_functions/scr_fleet_functions.gmlobjects/obj_shop/Draw_0.gmlscripts/scr_add_vehicle/scr_add_vehicle.gml
🧠 Learnings (1)
📚 Learning: 2025-07-01T20:29:44.075Z
Learnt from: OH296
PR: Adeptus-Dominus/ChapterMaster#902
File: objects/obj_shop/Create_0.gml:550-655
Timestamp: 2025-07-01T20:29:44.075Z
Learning: In GML shop item initialization code, when setting nobuy flags for shop items, the nobuy[i] = 1 assignment must occur before incrementing the index (i += 1). Setting nobuy after incrementing i will apply the flag to the wrong item, potentially making restricted items purchasable or blocking intended purchasable items.
Applied to files:
objects/obj_shop/Draw_0.gml
🔇 Additional comments (12)
scripts/scr_check_equip/scr_check_equip.gml (1)
38-69: Guard against missing/invalid unit before equipment checksEven with the initialisation fix, defensively skip if unit is undefined or invalid to prevent further heresy.
If fetch_unit can ever return undefined/null, consider:
if (!is_undefined(unit)) { // existing checks }Confirm fetch_unit’s contract; if it can fail, add the guard. I can draft a follow-up patch if desired.
scripts/scr_trade/scr_trade.gml (1)
106-114: Vehicle trade payload wiring looks correctThe “vehic” trade branch mirrors existing equip/merc patterns and populates trading_object.vehicles as expected for scr_trade_dep to consume. Blessed.
scripts/scr_event_code/scr_event_code.gml (1)
200-201: Rhino spawn: signature alignment looks correct, but consider passing real otherdataInvocation matches the extended scr_add_vehicle(type, target_company, otherdata, ...) API. Passing {} is valid as “no loadout”. If this Rhino is meant to appear anywhere specific (loc/wid/lid now supported in otherdata across the PR), consider supplying those so the unit doesn’t end up orphaned.
Would you like me to thread through a sensible default location payload here to parallel trade/dep flows (e.g., orbit of the unit’s current system)?
scripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gml (1)
17-19: Static has_troops accessor: simple and usefulThe helper cleanly exposes presence checks over garrison_log and keeps calling sites tidy. No objections.
scripts/scr_fleet_functions/scr_fleet_functions.gml (2)
542-543: Good: refreshing orbit state before arrival logicCalling is_orbiting() before the post-move processing reduces desync between position and orbit flags. Sanctified.
351-383: Destination fallback chain: good coverage, but ensure viewer is presentThis chain is sensible (player world -> any player presence -> imperial world). It still assumes a valid viewer earlier. After safeguarding the viewer (as proposed above), this becomes robust.
objects/obj_shop/Draw_0.gml (6)
17-22: UI labels clarifiedReplacing hashed strings with “Stocked” and “MC” improves legibility. Praise be.
54-59: Hit-testing switch to scr_hitGood unification. scr_hit appears used consistently across the UI; this aligns behaviour and avoids divergent rectangles.
90-99: Cost rendering logic: OK, but unify multiplier applicationThe cost path correctly multiplies by 5 when shift-buying (non-production). Keep this in sync with the purchase deduction logic below to avoid divergence.
220-223: Draw return-value stack: balanced pushes/popsadd_draw_return_values() paired with pop_draw_return_values() around slate draw is balanced. No stack leakage detected in this routine.
267-268: Global draw return-value pop matches top-level pushTop add_draw_return_values() is correctly balanced here. Well maintained state stack.
112-114: sell_item signature change: verify support for quantity parameterBy the Omnissiah — repo scan found a single caller and no function definition. Confirm the implementation supports sell_item(i, count, mod) or update callers.
- objects/obj_shop/Draw_0.gml:112–114
var sell_mult_count = keyboard_check(vk_shift) ? 5 : 1; sell_item(i, sell_mult_count, _sell_mod)Action: verify sell_item is declared (script/resource or function) with the new signature, or adjust this call accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Purpose and Description
Testing done
Related things and/or additional context