fix: Make spawning new defence fleets work #523
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe code modifications replace a detailed block responsible for constructing Imperial ships in Possibly related PRs
Suggested labels
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
OH296
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (4)
scripts/scr_imperial_navy_functions/scr_imperial_navy_functions.gml (3)
374-376:⚠️ Potential issueSafeguard the holy loop
Your while loop re-summons the same fleet if it remains at identical coordinates, risking an infinite iteration. Enshrine a maximum loop counter to preserve logical sanctity.-while (nearest_fleet.x==x && nearest_fleet.y==y){ +var loop_guard = 0; +while (nearest_fleet.x==x && nearest_fleet.y==y && loop_guard < 20){ nearest_fleet = instance_nearest(x, y, obj_en_fleet); + loop_guard++; }
377-379: 🛠️ Refactor suggestionRefine the condition for _valid
When faction is not an array, setting _valid to nearest_fleet.owner is always truthy (unless zero). Compare explicitly against the faction for consistency in the eyes of the Omnissiah.-_valid = nearest_fleet.owner; +_valid = (nearest_fleet.owner == faction);
382-389: 🧹 Nitpick (assertive)Activate only the chosen fleet
Invokinginstance_activate_object(obj_en_fleet)awakens all fleets of that type. If your sole intent is to animate the nearest_fleet, direct the ritual to its unique ID.-instance_activate_object(obj_en_fleet); +instance_activate_object(nearest_fleet);objects/obj_controller/Alarm_5.gml (1)
57-60: 🧹 Nitpick (assertive)Consolidate the training rites for specialized brethren.
While these invocations ofapothecary_training(),chaplain_training(),librarian_training(), andtechmarine_training()are clear, consider exposing them through a single orchestrating function (e.g.,perform_specialist_training()) to reduce repeated calls. This ensures more maintainable rites of code purity.A possible refactor:
- apothecary_training(); - chaplain_training(); - librarian_training(); - techmarine_training(); + perform_specialist_training();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
objects/obj_controller/Alarm_5.gml(1 hunks)scripts/scr_fleet_functions/scr_fleet_functions.gml(1 hunks)scripts/scr_forge_world_functions/scr_forge_world_functions.gml(1 hunks)scripts/scr_imperial_navy_functions/scr_imperial_navy_functions.gml(2 hunks)
🔇 Additional comments (6)
scripts/scr_imperial_navy_functions/scr_imperial_navy_functions.gml (2)
1-28: Question the undefined 'construction_forge'
O most enlightened adepts, the invocation of construction_forge within this blessed script is found, yet it is not defined or passed from beyond. This could invoke mysterious errors from the machine spirit. Ensure it references a valid instance or parameter when forging your fleets.
18-21: Scrutinize potential negative total ship count
The function decrements total_ships by (capital_number - 1), then compensates by enforcing a minimum of 1 ship if any vessels exist. Confirm this design is truly the will of the Omnissiah and does not allow fleeting negative states.scripts/scr_forge_world_functions/scr_forge_world_functions.gml (3)
65-65: Confirm forge references
You evokebuild_new_navy_fleet(construction_forge), yet the previously added function expects a parameter namedconstruction_system. Verify that the relevant variable name is indeed correct and consistent.
90-90: Caution with planet indexing
Your loop (for (var i=0; i<=planets; i++)) may risk an off-by-one scenario if planet indexing is 1-based. Confirm that 0 is a valid planet index to preserve logical harmony.
105-105: Reevaluate the allowance threshold
Your condition(imp_ships < ship_allowance)ensures new fleets spawn only if the total remains below a certain ratio. Confirm this ratio aligns with the Imperium’s strategic demands and will not overproduce or starve the defense fleets.objects/obj_controller/Alarm_5.gml (1)
56-56: Praise the Machine Spirit for streamlined fleet-building.
By the Omnissiah’s design, the invocation ofbuild_planet_defence_fleets()here is a worthy replacement for the prior labyrinthine logic. Ensure that all resource counts and world ownership checks are properly handled within that script to prevent runaway fleets.Would you like an automated search for references to confirm that no prior calls to old fleet-building logic remain?
| function scr_orbiting_fleet(faction, system="none"){ | ||
| if (system == "none"){ | ||
| nearest_fleet = instance_nearest(x,y,obj_en_fleet); | ||
| var _valid = false; | ||
| if (is_array(faction)){ | ||
| _valid = array_contains(faction, nearest_fleet.owner); | ||
| } else { | ||
| _valid = nearest_fleet.owner; | ||
| while (nearest_fleet.x==x && nearest_fleet.y==y){ | ||
| nearest_fleet = instance_nearest(x,y,obj_en_fleet); | ||
| var _valid = false; | ||
| if (is_array(faction)){ |
There was a problem hiding this comment.
Ward against the infinite loop
Within this while incantation, ‘instance_nearest’ may perpetually yield itself if no other fleets exist at different coordinates. Bestow a failsafe to avoid eternal recursion.
| instance_activate_object(obj_en_fleet); | ||
| return nearest_fleet.id; | ||
| } else { | ||
| instance_deactivate_object(nearest_fleet.id); | ||
| } | ||
| } | ||
| if (_valid && nearest_fleet.action == ""){ | ||
| instance_activate_object(obj_en_fleet); | ||
| return nearest_fleet.id; | ||
| } else { | ||
| instance_deactivate_object(nearest_fleet.id); | ||
| instance_activate_object(obj_en_fleet); | ||
| } else { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Employ precise object reactivation
Observe that instance_activate_object(obj_en_fleet) resurrects all fleets of that type. If the plan is to quicken only the chosen orbiting fleet, feed in its direct ID to appease the machine spirit.
| var targeted=false; | ||
| var target; | ||
| //shuffle the contents, if any | ||
| array_shuffle_ext(system_4); | ||
| array_shuffle_ext(system_3); | ||
| array_shuffle_ext(system_other); | ||
|
|
||
| if (targeted) { | ||
| target = array_pop(system_4) | ||
| targeted=true; | ||
| } | ||
| if (targeted) { | ||
| target = array_pop(system_3) | ||
| targeted=true; | ||
| } | ||
| if (targeted) { | ||
| target = array_pop(system_other) | ||
| targeted=true; | ||
| } |
There was a problem hiding this comment.
Fix reversed condition for system targeting
This segment sets targeted=false but only performs array_pop if targeted is already true. This logic never triggers. Adjust the condition to set targeted before popping from the arrays.
-var targeted=false;
-if (targeted) {
- target = array_pop(system_4);
- targeted=true;
+var targeted=false;
+if (!targeted && array_length(system_4) > 0) {
+ target = array_pop(system_4);
+ targeted=true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var targeted=false; | |
| var target; | |
| //shuffle the contents, if any | |
| array_shuffle_ext(system_4); | |
| array_shuffle_ext(system_3); | |
| array_shuffle_ext(system_other); | |
| if (targeted) { | |
| target = array_pop(system_4) | |
| targeted=true; | |
| } | |
| if (targeted) { | |
| target = array_pop(system_3) | |
| targeted=true; | |
| } | |
| if (targeted) { | |
| target = array_pop(system_other) | |
| targeted=true; | |
| } | |
| var targeted=false; | |
| var target; | |
| //shuffle the contents, if any | |
| array_shuffle_ext(system_4); | |
| array_shuffle_ext(system_3); | |
| array_shuffle_ext(system_other); | |
| if (!targeted && array_length(system_4) > 0) { | |
| target = array_pop(system_4); | |
| targeted = true; | |
| } | |
| if (targeted) { | |
| target = array_pop(system_3) | |
| targeted = true; | |
| } | |
| if (targeted) { | |
| target = array_pop(system_other) | |
| targeted = true; | |
| } |
Purpose of the PR
Make forge worlds spawning new defence fleets for systems work again, currently they just create dozzens of stacks of fleets that hang over the forge worlds causing memory issues
Describe the solution
rewrite from scratch for the most part and abstract to usable functions