refactor: Remove static company sizes and KILL parallel arrays - #1405
Conversation
github-actions
Bot
added
Size: Medium
Type: Refactor
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
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 20 unresolved issues from previous reviews.
Re-trigger cubic
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 4 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.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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 17 unresolved issues from previous reviews.
Re-trigger cubic
|
/oc Check for code quality issues of the newly added/changed code (docs/CODE_STYLE.md) |
|
Here's my review as the project maintainer. OverviewThis PR removes the fixed-size 501-slot arrays and parallel P0 — Will crash at runtime1.
|
|
/review |
Review —
|
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Confidence score: 2/5
- In
objects/obj_ini/Create_0.gml, usingrace = array_create(11, []);will make all entries point to the same underlying array, so updating one race slot can unintentionally mutate every slot and corrupt game state/setup data; initialize each index with a distinct array instance (e.g., looped per-slot assignment) to remove the shared-reference risk.
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="objects/obj_ini/Create_0.gml">
<violation number="1" location="objects/obj_ini/Create_0.gml:121">
P0: Custom agent: **Code Quality Review**
The change to `race = array_create(11, []);` introduces a shared-reference bug. In GameMaker, `array_create(n, value)` stores the same reference in every slot when `value` is an array, so all 11 company indices alias the same array. During initialization in `scr_initialize_custom`, the loop writing `race[c][i]` for companies 1–10 mutates that single shared array, meaning later changes to one company will corrupt the others. Use an explicit loop to create a distinct inner array for each company instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
8 issues found across 10 files (changes from recent commits).
Confidence score: 2/5
- In
scripts/scr_company_order/scr_company_order.gml,company_length()now returns an array while callers inscr_max_marine.gml,scr_crusade.gml, andscr_ship_occupants.gmlstill treat it as an integer loop bound, which can break iteration logic and occupant calculations at runtime — restore an integer count return (or update all call sites to use array length explicitly). - In
scripts/scr_max_marine/scr_max_marine.gml, the refactor switched lookup tocobut max-tracking still writesman_c = c, so the selected company can be wrong or undefined and propagate bad assignment decisions — align downstream assignments to the new loop variable and add a quick assertion/test around max-company selection. - In
scripts/scr_chapter_managent_events/scr_chapter_managent_events.gml,find_company_open_slot(0)has side effects (mutating parallel arrays) and can also allow a Forge Master replacement into reserved slot 0, risking silent state corruption of leadership slots — keep the slot-1 guard for this path or split lookup from allocation to avoid mutation during checks. - In
scripts/scr_add_man/scr_add_man.gml, moving_unit.ageassignment to the end overwritesroll_age()results for marines, causing incorrect character age data that can affect progression/balance logic — only apply_unit.agewhen intended (or gate by unit type) before/around biological age rolls.
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="objects/obj_controller/Alarm_5.gml">
<violation number="1" location="objects/obj_controller/Alarm_5.gml:297">
P2: Custom agent: **Code Quality Review**
This line introduces a raw magic number `10` as a domain identifier for the penitent/reclusiam `god_status` state. The value `10` is already compared against `god_status` in at least six other places across the codebase, so it qualifies as a reused semantic constant. Per the code-quality rule, raw numbers used as identifiers more than once should be declared as a `#macro` or `enum` (a scalar constant, not an array). Consider defining something like `#macro GOD_STATUS_PENITENT 10` and replacing all occurrences so the intent is self-documenting.</violation>
</file>
<file name="scripts/scr_chapter_managent_events/scr_chapter_managent_events.gml">
<violation number="1" location="scripts/scr_chapter_managent_events/scr_chapter_managent_events.gml:275">
P2: Custom agent: **Code Quality Review**
The newly added code repeats the raw literal `0` as a company identifier in two coupled call sites (`find_company_open_slot(0)` and `scr_move_unit_info(..., 0, ...)`). Because both refer to the same destination company, this obscures intent and makes future refactors error-prone. Consider introducing a named constant (e.g., a `#macro` or `enum` value) for the headquarters/unassigned company index.</violation>
<violation number="2" location="scripts/scr_chapter_managent_events/scr_chapter_managent_events.gml:275">
P2: Custom agent: **Code Quality Review**
The call to `find_company_open_slot(0)` looks like a simple lookup, but the function mutates `obj_ini` parallel arrays by pushing new empty entries when no open slot exists. Because `scr_move_unit_info` immediately writes into `obj_ini.*[0][_company_slot]`, this control path implicitly depends on the array-growth side effect. A function named `find_*` should ideally be a pure query; consider renaming it to something like `get_or_create_company_open_slot`, or split the logic so callers explicitly reserve a slot before moving a unit.</violation>
<violation number="3" location="scripts/scr_chapter_managent_events/scr_chapter_managent_events.gml:275">
P2: A Forge Master replacement can be placed into the reserved Chapter Master slot when slot 0 is empty, overwriting that slot’s state. Preserve the existing slot-1 lower bound for this flow, or let `find_company_open_slot` accept a minimum slot.</violation>
</file>
<file name="scripts/scr_add_man/scr_add_man.gml">
<violation number="1" location="scripts/scr_add_man/scr_add_man.gml:123">
P2: Custom agent: **Code Quality Review**
The current-age calculation `(obj_controller.millenium * 1000) + obj_controller.year` is repeated in at least six other files (e.g., `scr_initialize_custom`, `scr_after_combat`, `obj_p_assra`). Using a raw `1000` as a conversion factor across that many call sites creates maintenance debt if the calendar scale ever changes. You already have `obj_controller` in scope here, so extracting a `get_current_year()` method on `obj_controller` (or a global helper) would let you unify the logic and remove the scattered raw constant in one step.</violation>
<violation number="2" location="scripts/scr_add_man/scr_add_man.gml:123">
P1: Custom agent: **Code Quality Review**
Moving `_unit.age` to the end of the function causes it to silently overwrite the biological age rolled by `roll_age()` for marine units. In the original placement, this assignment happened before the marine branch reassigned `_unit`, so marines retained their `roll_age()` value; now the current calendar year (~40000) replaces that rolled age. Because `age` is used in equipment quality calculations like `age + exp`, this corrupts spawned marine stats. Preserve the rolled marine age by making the date assignment conditional on non-marine roles, or restore the original placement where the marine reassignment naturally discarded it.</violation>
</file>
<file name="scripts/scr_max_marine/scr_max_marine.gml">
<violation number="1" location="scripts/scr_max_marine/scr_max_marine.gml:13">
P1: Custom agent: **Code Quality Review**
This refactor updated `fetch_unit` to use the loop counter `co`, but the downstream assignments that record the max company (`man_c = c`) still reference the undeclared variable `c` in all three branches. Because `c` is never declared in this function, GameMaker falls back to instance-scope resolution, which will likely yield 0 or a stale value rather than the actual company index. To keep the fetch and the tracking consistent, change `man_c = c` to `man_c = co` in the chaos, age, and exp branches.</violation>
</file>
<file name="scripts/scr_company_order/scr_company_order.gml">
<violation number="1" location="scripts/scr_company_order/scr_company_order.gml:20">
P1: company_length now returns the TTRPG array instead of the unit count, but every caller uses it as an integer: scr_max_marine.gml:12 and scr_crusade.gml:66 loop `i < company_length(co)`, and scr_ship_occupants.gml:6 sets `_co_length = company_length(co)` then loops `i < _co_length`. Since obj_ini.TTRPG[company] is a 2D array of units (obj_ini/Create_0.gml), these become number-vs-array comparisons, which raise GML runtime errors and break unit iteration. Return the array length instead.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| continue; | ||
| for (co = 0; co <= obj_ini.companies; co++) { | ||
| for (var i = 0; i < company_length(co); i++) { | ||
| unit = fetch_unit([co, i]); |
There was a problem hiding this comment.
P1: Custom agent: Code Quality Review
This refactor updated fetch_unit to use the loop counter co, but the downstream assignments that record the max company (man_c = c) still reference the undeclared variable c in all three branches. Because c is never declared in this function, GameMaker falls back to instance-scope resolution, which will likely yield 0 or a stale value rather than the actual company index. To keep the fetch and the tracking consistent, change man_c = c to man_c = co in the chaos, age, and exp branches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_max_marine/scr_max_marine.gml, line 13:
<comment>This refactor updated `fetch_unit` to use the loop counter `co`, but the downstream assignments that record the max company (`man_c = c`) still reference the undeclared variable `c` in all three branches. Because `c` is never declared in this function, GameMaker falls back to instance-scope resolution, which will likely yield 0 or a stale value rather than the actual company index. To keep the fetch and the tracking consistent, change `man_c = c` to `man_c = co` in the chaos, age, and exp branches.</comment>
<file context>
@@ -10,7 +10,7 @@ function scr_max_marine(max_type) {
for (co = 0; co <= obj_ini.companies; co++) {
for (var i = 0; i < company_length(co); i++) {
- unit = fetch_unit([c, i]);
+ unit = fetch_unit([co, i]);
if (!is_struct(unit)) {
continue;
</file context>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Confidence score: 3/5
- In
scripts/scr_max_marine/scr_max_marine.gml, the fetch uses loop keycobut assignment and name lookup still usec(man_c = candobj_ini.name[man_c][man_i]), which can attach the wrong candidate/category and surface incorrect marine selection or naming at runtime — align all three branches and the return lookup to the same loop variable. - In
scripts/scr_add_man/scr_add_man.gml, age assignment logic is split under conditions tied toarray_contains(non_marine_roles, man_role), so marines and non-marines may follow inconsistent age paths and produce unintended demographics — consolidate the branch gating so each role type takes exactly one intended age calculation path.
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_max_marine/scr_max_marine.gml">
<violation number="1" location="scripts/scr_max_marine/scr_max_marine.gml:13">
P2: The fetch is now correctly keyed by the loop variable `co`, but the result is still recorded against `man_c = c;` (three branches) and the return name lookup `obj_ini.name[man_c][man_i]`. `c` is not the loop variable and is never assigned in this function, so if the max marine is in company co>0, the returned/all dialogue column reports company 0's marine name while the value it found is from another company. Complete the rename to `co` in all branches so the returned marine identity matches what was actually selected.</violation>
</file>
<file name="scripts/scr_add_man/scr_add_man.gml">
<violation number="1" location="scripts/scr_add_man/scr_add_man.gml:123">
P3: The marine branch sets age via roll_age() inside the `!array_contains(non_marine_roles, man_role)` block, while non-marines get the date-based age in this second, separate block gated by the same array_contains check. Keeping one initial-age mechanism in two disconnected places with a duplicated guard makes it easy for the two branches to drift. Consider setting age inside the existing non-marine (`else`) path or behind a small helper so the age logic lives in one place.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 11 files (changes from recent commits).
Confidence score: 3/5
- In
objects/obj_ini/Create_0.gml,array_create(11, [])risks aliasing inner arrays and only allocates rows0..10, while other scripts indexrace[100..102]; this can cause cross-slot data contamination or missing-row/runtime errors when defaults/custom setup paths run — initialize each row as a distinct array and explicitly size or guard high-index rows used bydefaults_slot. - In
scripts/scr_enemy_ai_d/scr_enemy_ai_d.gml,string_count("0", _unit.specials) > 0uses a magic literal that obscures intent and can silently misclassify abilities if the specials format changes — replace it with a named constant or helper predicate that documents the flag meaning. - In
scripts/scr_add_man/scr_add_man.gml, the duplicatedarray_contains(non_marine_roles, man_role)checks with opposite polarity create avoidable branch-maintenance risk and can drift into inconsistent behavior — collapse to one membership evaluation and branch from that single result.
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="objects/obj_ini/Create_0.gml">
<violation number="1" location="objects/obj_ini/Create_0.gml:121">
P2: race = array_create(11, []) leaves only 11 outer rows (0-10), but race is indexed at row 100/101/102 via defaults_slot across scr_ui_settings/scr_role_setup/scr_initialize_custom. Sizing now depends on rows being created on demand by out-of-bounds writes in scr_initialize_custom (race[100]/race[102] = array_create(...)) plus GameMaker's copy-on-write on a single shared empty array; reading race[100][...] before those rows are built is an out-of-bounds read and will error. This replaces the target branch's self-contained array_create_2d(11, 501, 0) with implicit, undocumented sizing.</violation>
<violation number="2" location="objects/obj_ini/Create_0.gml:121">
P1: Custom agent: **Code Quality Review**
In GameMaker, `array_create(11, [])` stores the *same* empty array reference in all 11 slots. While GML’s copy-on-write can protect some direct index assignments, any mutation through a function call (e.g. `array_push`) or an indirect reference will leak data across company rows. This is a fragile aliasing bug that is easy to miss and exactly the kind of subtle language behavior the Code Quality Review criteria ask us to flag.
**Suggested fix:** Initialize each slot with its own array instance, for example:
```gml
race = array_create(11);
for (var i = 0; i < 11; i++) {
race[i] = [];
}
(Note that the same issue already exists for name, role, etc. in this file, but this diff newly introduces it for race.)
The array_contains(non_marine_roles, man_role) check appears twice in this function with opposite polarity. Since the two paths are mutually exclusive, they should be combined into a single if/else block. Duplicating the same membership test makes the control flow harder to follow and increases maintenance burden if the role list or branching logic ever changes.
The check string_count("0", _unit.specials) > 0 relies on an unexplained numeric string literal as a magic constant, which makes the intent opaque to readers and forces the same fragile pattern to be repeated across multiple files. Because this PR is already a code-quality refactor, consider introducing an enum or a helper method such as has_special(special_id) so ability checks are self-documenting and can be maintained in one place.
</details>
<sub>**Tip**: Review your code locally with the [cubic CLI](https://docs.cubic.dev/ide/cli-review?utm_source=github&utm_content=general_review_body) to iterate faster.<br /><br />[Re-trigger cubic](https://www.cubic.dev/action/re-review/pr/Adeptus-Dominus/ChapterMaster/1405/ai_pr_review_1785607719961_420d2c12-4789-4a4b-953d-9cf5d7ff9cbb?returnTo=https%3A%2F%2Fgithub.com%2FAdeptus-Dominus%2FChapterMaster%2Fpull%2F1405)</sub>
<!-- cubic:review-post:ai_pr_review_1785607719961_420d2c12-4789-4a4b-953d-9cf5d7ff9cbb:7413137bf750c8a47a2ea89b36ebe05a6a528119:ba2b52ee-82d9-4502-8a89-a7148285bd7b,0f37d097-5fc9-432c-80df-c842ecf4309b -->
1 participant
Summary by cubic
Removed the static 501-slot arrays and all parallel fields; units now own
age,specials, andgod_status. Company rosters and spawns are fully dynamic, loops/serialization use real lengths, and marine ascension is turn-based.Refactors
age/spe/godmirrors and stored those on the unit.find_company_open_slotgrows mirrors/TTRPGand returns a valid index;scr_company_orderresizes mirrors to roster length.scr_max_marinereturns a unit instead of a delimited string; dialogue and CM psyker checks readunit.specials. Ship occupants rebuilt to useUnitIndexand tally vehicle roles.Bug Fixes
unit.god_statusand iterate current roster lengths.unit.ageandmarine_ascension(turn‑based).Written for commit 7413137. Summary will update on new commits.