refactor: Remove all remaining unit parallel arrays - #1414
Conversation
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
/review |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 38 unresolved issues from previous reviews.
Re-trigger cubic
|
/review |
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 40 unresolved issues from previous reviews.
Re-trigger cubic
|
Here's where we stand. I was verifying the chaos-meeting force scan because there are two copies of the same logic, and both have unsafe derefs: What we found
Next steps offered
Nothing has been edited yet — this was verification only. Want me to apply the guardrail fix to both locations now? |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
scripts/scr_UnitGroup/scr_UnitGroup.gml(kill_percent), the loop direction was changed to backward but the chapter-master skip still incrementsi, which can drive the index past valid bounds before_kill_numbis reached; this risks out-of-range access or incorrect unit removal whenkill_percent < 100— align the skip step with backward iteration (or add explicit bounds guards) and retest partial-kill paths.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/scr_UnitGroup/scr_UnitGroup.gml">
<violation number="1" location="scripts/scr_UnitGroup/scr_UnitGroup.gml:180">
P1: kill_percent now iterates backward but the chapter-master skip still increments i, so when kill_percent < 100 and the chapter master is reached before _kill_numb is met, i advances up past the array end (or back over already-deleted slots), producing an out-of-bounds unit read and a crash on `_unit.role()`. Make the skip decrement: `i--; continue;` (or only skip when i > 0).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| var _killed = 0; | ||
| var i = 0; | ||
| while (_killed < _kill_numb && i < number()) { | ||
| var i = number() - 1; |
There was a problem hiding this comment.
P1: kill_percent now iterates backward but the chapter-master skip still increments i, so when kill_percent < 100 and the chapter master is reached before _kill_numb is met, i advances up past the array end (or back over already-deleted slots), producing an out-of-bounds unit read and a crash on _unit.role(). Make the skip decrement: i--; continue; (or only skip when i > 0).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_UnitGroup/scr_UnitGroup.gml, line 180:
<comment>kill_percent now iterates backward but the chapter-master skip still increments i, so when kill_percent < 100 and the chapter master is reached before _kill_numb is met, i advances up past the array end (or back over already-deleted slots), producing an out-of-bounds unit read and a crash on `_unit.role()`. Make the skip decrement: `i--; continue;` (or only skip when i > 0).</comment>
<file context>
@@ -166,19 +167,26 @@ function UnitGroup(units = []) constructor {
var _killed = 0;
- var i = 0;
- while (_killed < _kill_numb && i < number()) {
+ var i = number() - 1;
+ while (_killed < _kill_numb && i >= 0) {
var _unit = units[i];
</file context>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 39 unresolved issues from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 39 unresolved issues from previous reviews.
Re-trigger cubic
Summary by cubic
Finishes the migration to struct-based units and roles by removing the last parallel arrays and name checks. Boarding, events, shaders, and saves now pass real unit structs; artifacts persist bearer by unit UID and safely re-link on load.
Refactors
scr_master_loc,scr_move_unit_info,scr_bionics_count,scr_marine_count; Chapter Master now viaChapterMaster; location checks use.get_struct().is_at_location(...).company_length(...)andfetch_unit(...); removedobj_ini.nameusage.role_compare(...),active_roles(), andget_department_head(...); applied across UI, dialogue, AI, management, fleets, boarding, combat, and apothecarion/librarium.player_role_data; addeddefault_role_data,setup_default_gears, andglobal.role_data_keys; Librarian auto-hidden if Psyker Intolerant; custom role names editplayer_role_data._unit.kill(...),_unit.move_to_company(...),_unit.update_role(...); used in ship loss, crusade, garrison, inspections, events, perils, promotions, and transfers.ArtifactStruct; restored viafetch_unit_uid(...); gossip/events inspect equipment structs.tally_marines();UnitGroupgainedpush(...),tally_attr(...), and batch-kill.-1; Mechanicus bionics mission counts viaUnitGroup.tally_attr("bionics").board_*arrays withobj_p_ship.board_marineandobj_p_assra.occupants; flows pass struct refs.unit_stats.jsonbase_groupfixed to "eldar"; livery shader usesactive_roles()with bounds checks to prevent OOB; minor indexing fixes and lingering legacy removals.Migration
player_role_data(available_to_player); update role radios and livery preview._unit.move_to_company(...),ChapterMaster.is_at_location(...), and_unit.kill(...).name == ""checks and array reads; rely onfetch_unit(...),company_length(...),active_roles(), androle_compare(...).UnitGroup.tally_attr(...)for mission tallies; useobj_p_ship.board_marineandobj_p_assra.occupantsfor boarding; use-1sentinel for unset event targets.Written for commit cbc2c05. Summary will update on new commits.