fix: hardcoded values counting vehicles (and marines) on managmenet pane.#1052
fix: hardcoded values counting vehicles (and marines) on managmenet pane.#1052OH296 merged 4 commits intoAdeptus-Dominus:mainfrom
Conversation
…management screen, hopefully
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughTech-Priest: Iteration logic for units and vehicles was refactored to use data-driven array lengths (obj_ini.TTRPG and obj_ini.veh_role per company). Indexing moved to zero-based arrays, Captain detection switched to name string comparison, and vehicle counting was consolidated and relocated outside inner unit loops. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Tech-Priest: The arrays whisper their bounds; heed them, and prevent the silent rupture of indices. Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
OH296
left a comment
There was a problem hiding this comment.
one minor change otherwise all seems good to me, as a note we generally are trying to avoid the use of then and are swapping it out with the more conventional if (condition){<do thing>} format but that is a a lesser concern that i'm not going to enforce minor changes
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/scr_vehicle_order/scr_vehicle_order.gml (2)
45-49: Tech-Priest, heretical corruption detected in the logic-circuits.The comparison operators have been replaced with assignment runes. The variable
_is_vehicle_roleshall always resolve to truth, and the sacred role-designations shall be overwritten with each iterative pass.Apply this ritual of correction:
- var _is_vehicle_role = - veh_role[company_number][i] = "Rhino" // TODO change to enums/string ids - || veh_role[company_number][i] = "Predator" - || veh_role[company_number][i] = "Whirlwind" - || veh_role[company_number][i] = "Land Speeder" - || veh_role[company_number][i] = "Land Raider"; + var _is_vehicle_role = + veh_role[company_number][i] == "Rhino" // TODO change to enums/string ids + || veh_role[company_number][i] == "Predator" + || veh_role[company_number][i] == "Whirlwind" + || veh_role[company_number][i] == "Land Speeder" + || veh_role[company_number][i] == "Land Raider";
72-87: Grave inconsistency detected in the indexing protocol.The final loop now iterates from the void-point (0), yet
scr_add_vehicle.gml:14deliberately begins iteration at unity, implying index 0 is reserved or sanctified. This mismatch will corrupt the data-matrix when vehicle assignment and reordering functions collide.The code commences iteration at index 1 and searches for empty vehicle slots, suggesting index 0 must remain untouched. Your reordering sequence must either:
- Begin iteration at index 1 to align with
scr_add_vehicle.gml, or- Confirm that the entire vehicle system has been migrated to 0-based indexing and update
scr_add_vehicle.gmlaccordingly.Verify the indexing doctrine across all vehicle-access functions before this ritual completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
scripts/scr_vehicle_order/scr_vehicle_order.gml(3 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_vehicle_order/scr_vehicle_order.gml
**/*.*
⚙️ CodeRabbit configuration file
**/*.*: - Having humanly understandable and maintainable code is the top most priority.
- DRY (Don't repeat yourself) principle is also very important.
- 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_vehicle_order/scr_vehicle_order.gml
🧠 Learnings (2)
📓 Common learnings
Learnt from: EttyKitty
Repo: Adeptus-Dominus/ChapterMaster PR: 647
File: scripts/scr_en_weapon/scr_en_weapon.gml:24-928
Timestamp: 2025-03-29T10:30:25.598Z
Learning: A data-driven approach for weapon management in `scripts/scr_en_weapon/scr_en_weapon.gml` is planned for a future PR, not within the scope of PR #647.
Learnt from: VanWeapon
Repo: Adeptus-Dominus/ChapterMaster PR: 725
File: scripts/scr_special_view/scr_special_view.gml:49-49
Timestamp: 2025-04-15T10:54:19.720Z
Learning: In the ChapterMaster codebase, obj_ini.role[100][2] and obj_ini.role[100][eROLE.ChapterMaster] reference different roles, and both are needed in conditions checking for specific types of units. Numeric indices should not be assumed to match their corresponding enum values without verification.
Learnt from: MCPO-Spartan-117
Repo: Adeptus-Dominus/ChapterMaster PR: 508
File: scripts/scr_specialist_training/scr_specialist_training.gml:371-371
Timestamp: 2025-02-24T07:38:42.267Z
Learning: The spec_train_data array index for Techmarine training data is 0, as confirmed by the maintainer.
Learnt from: VanWeapon
Repo: Adeptus-Dominus/ChapterMaster PR: 725
File: scripts/scr_initialize_custom/scr_initialize_custom.gml:2474-2572
Timestamp: 2025-04-15T11:03:37.154Z
Learning: In the ChapterMaster initialization code, negative values in the company structure (e.g., for vehicles like rhinos, landspeeders) are acceptable as only positive values will result in units being created.
Learnt from: EttyKitty
Repo: Adeptus-Dominus/ChapterMaster PR: 0
File: :0-0
Timestamp: 2025-04-05T20:54:45.813Z
Learning: EttyKitty requested a rework of the vehicle and marine recovery system in ChapterMaster. The current system allows guaranteed recovery with enough recovery points, while the proposed system would use random chance modified by techmarine skill and item bonuses.
Learnt from: EttyKitty
Repo: Adeptus-Dominus/ChapterMaster PR: 0
File: :0-0
Timestamp: 2025-03-20T22:22:57.319Z
Learning: In the ChapterMaster game PR #424, the psychic power system was changed to be data-driven, with perils now able to occur on both successful and failed casts but with reduced frequency overall.
📚 Learning: 2025-04-15T10:54:19.720Z
Learnt from: VanWeapon
Repo: Adeptus-Dominus/ChapterMaster PR: 725
File: scripts/scr_special_view/scr_special_view.gml:49-49
Timestamp: 2025-04-15T10:54:19.720Z
Learning: In the ChapterMaster codebase, obj_ini.role[100][2] and obj_ini.role[100][eROLE.ChapterMaster] reference different roles, and both are needed in conditions checking for specific types of units. Numeric indices should not be assumed to match their corresponding enum values without verification.
Applied to files:
scripts/scr_vehicle_order/scr_vehicle_order.gml
🔇 Additional comments (2)
scripts/scr_vehicle_order/scr_vehicle_order.gml (2)
25-40: Tech-Priest, the machine-spirit acknowledges your correction.The iterative bounds now draw from the data-matrix itself rather than rigid numerals etched in stone. This aligns with the sacred principles of adaptability.
43-43: Tech-Priest, the iterative boundary has been purified.The machine-spirit no longer references immutable numerals but instead queries the data-structure's true extent.
|
Looks good on the change, looks like you committed it already. |
…ane. (Adeptus-Dominus#1052) Co-authored-by: Nelsonh <81228864+OH296@users.noreply.github.com>
…management screen, hopefully
Purpose and Description
Testing done
Related things and/or additional context