Skip to content

refactor(perf): add_marines_to_recovery move switch to dict lookup #1569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
SweetZJ wants to merge 1 commit into
base: main
Choose a base branch
from
+59 −56
Draft

refactor(perf): add_marines_to_recovery move switch to dict lookup #1569

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 59 additions & 56 deletions scripts/scr_after_combat/scr_after_combat.gml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,67 +1,70 @@
/// @self Asset.GMObject.obj_pnunit

function add_marines_to_recovery() {
var _roles = active_roles();

var _bonus_specific = {};

@cubic-dev-ai cubic-dev-ai Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The struct lookup changes precedence semantics from the old switch without flagging it. The switch matched the first case top-down, so when two table entries shared a title the earlier (higher-bonus) case won; in the new table the last assignment wins, so the later (lower-bonus) row silently overwrites it. Example: if a chapter renames the Apothecary to "Forge Master", the old code hit case "Forge Master" first and gave 360, while the new code's later _bonus_specific[$ _roles[eROLE.APOTHECARY]] = 40 overwrites it with 40. Role titles are config data, not constants: active_roles() returns obj_creation.role[100]/obj_ini.role[100] (scripts/is_specialist/is_specialist.gml) and _unit.role() returns the stored title string obj_ini.role[company][marine_number] (scripts/scr_marine_struct/scr_marine_struct.gml), so two roles can legitimately hold the same title. A dropped bonus changes who ds_priority_delete_max revives first in obj_ncombat/Alarm_5.gml. Build the table from an ordered [title, bonus] list with first-write-wins, or define priorities as #macros and construct a single literal so collisions are visible instead of order-dependent.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_after_combat/scr_after_combat.gml, line 6:

<comment>The struct lookup changes precedence semantics from the old switch without flagging it. The switch matched the first case top-down, so when two table entries shared a title the earlier (higher-bonus) case won; in the new table the last assignment wins, so the later (lower-bonus) row silently overwrites it. Example: if a chapter renames the Apothecary to "Forge Master", the old code hit `case "Forge Master"` first and gave 360, while the new code's later `_bonus_specific[$ _roles[eROLE.APOTHECARY]] = 40` overwrites it with 40. Role titles are config data, not constants: `active_roles()` returns `obj_creation.role[100]`/`obj_ini.role[100]` (scripts/is_specialist/is_specialist.gml) and `_unit.role()` returns the stored title string `obj_ini.role[company][marine_number]` (scripts/scr_marine_struct/scr_marine_struct.gml), so two roles can legitimately hold the same title. A dropped bonus changes who `ds_priority_delete_max` revives first in obj_ncombat/Alarm_5.gml. Build the table from an ordered [title, bonus] list with first-write-wins, or define priorities as `#macro`s and construct a single literal so collisions are visible instead of order-dependent.</comment>

<file context>
@@ -1,67 +1,70 @@
 function add_marines_to_recovery() {
     var _roles = active_roles();
+    
+    var _bonus_specific = {};
+    
+
</file context>



_bonus_specific[$ obj_ini.role[100][eROLE.CHAPTERMASTER]] = 720;

@OH296 OH296 Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj_ini.role[100][eROLE.CHAPTERMASTER] references the struct with the roles full data the llocal var _roles = active_roles(); strips all the role strings from those structs into an array and is thus the correct ref here

Suggested change
_bonus_specific[$ obj_ini.role[100][eROLE.CHAPTERMASTER]] = 720;
_bonus_specific[$ _roles[eROLE.CHAPTERMASTER]] = 720;



_bonus_specific[$ "Forge Master"] = 360;

@cubic-dev-ai cubic-dev-ai Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This 24-line one-key-per-assignment table is harder to maintain than the established pattern: the function directly below, add_vehicles_to_recovery, uses a single struct literal ({ "Land Raider": 10, ... }) for the same lookup. A literal is shorter, matches the neighbouring code, and makes a duplicated key visible at compile time instead of silently depending on assignment order. The stray blank lines between priority tiers read as leftover scaffolding; a tier comment or grouped literal conveys the intent instead. While rewriting, also consider the repo style (.coderabbit.yaml) of #macro/enum over bare literals: 720/360/160/80/40/20 and the hardcoded titles like "Forge Master", "Codiciery", and "Lexicanum" are raw values.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/scr_after_combat/scr_after_combat.gml, line 12:

<comment>This 24-line one-key-per-assignment table is harder to maintain than the established pattern: the function directly below, `add_vehicles_to_recovery`, uses a single struct literal (`{ "Land Raider": 10, ... }`) for the same lookup. A literal is shorter, matches the neighbouring code, and makes a duplicated key visible at compile time instead of silently depending on assignment order. The stray blank lines between priority tiers read as leftover scaffolding; a tier comment or grouped literal conveys the intent instead. While rewriting, also consider the repo style (`.coderabbit.yaml`) of `#macro`/`enum` over bare literals: 720/360/160/80/40/20 and the hardcoded titles like "Forge Master", "Codiciery", and "Lexicanum" are raw values.</comment>

<file context>
@@ -1,67 +1,70 @@
+    _bonus_specific[$ obj_ini.role[100][eROLE.CHAPTERMASTER]] = 720;
+    
+
+    _bonus_specific[$ "Forge Master"] = 360;
+    _bonus_specific[$ "Master of Sanctity"] = 360;
+    _bonus_specific[$ "Master of the Apothecarion"] = 360;
</file context>

_bonus_specific[$ "Master of Sanctity"] = 360;
_bonus_specific[$ "Master of the Apothecarion"] = 360;
_bonus_specific[$ $"Chief {_roles[eROLE.LIBRARIAN]}"] = 360;
Comment on lines +13 to +15

@OH296 OH296 Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see lower comment regarding eums



_bonus_specific[$ _roles[eROLE.CAPTAIN]] = 160;
_bonus_specific[$ _roles[eROLE.HONOURGUARD]] = 160;
_bonus_specific[$ _roles[eROLE.ANCIENT]] = 160;


_bonus_specific[$ _roles[eROLE.VETERANSERGEANT]] = 80;
_bonus_specific[$ _roles[eROLE.TERMINATOR]] = 80;


_bonus_specific[$ _roles[eROLE.VETERAN]] = 40;
_bonus_specific[$ _roles[eROLE.SERGEANT]] = 40;
_bonus_specific[$ _roles[eROLE.CHAMPION]] = 40;
_bonus_specific[$ _roles[eROLE.CHAPLAIN]] = 40;
_bonus_specific[$ _roles[eROLE.APOTHECARY]] = 40;
_bonus_specific[$ _roles[eROLE.TECHMARINE]] = 40;
_bonus_specific[$ _roles[eROLE.LIBRARIAN]] = 40;
_bonus_specific[$ "Codiciery"] = 40;
Comment on lines +16 to +34

@OH296 OH296 Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general it's easier to just create the struct and assign the values implicitly e.g
_bonus_specific = { _roles[eROLE.VETERAN]] : 40, _roles[eROLE.SERGEANT] : 40, _roles[eROLE.TECHMARINE]] : 40, etc.....

by assigning this way and decalring _bonus_specific as static not a var efficiency will likewise be improved

_bonus_specific[$ "Lexicanum"] = 40;
Comment on lines +34 to +35

@OH296 OH296 Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these roles (and all marine roles) now have enums e.g eROLE.LEXICANUM



_bonus_specific[$ _roles[eROLE.TACTICAL]] = 20;
_bonus_specific[$ _roles[eROLE.ASSAULT]] = 20;
_bonus_specific[$ _roles[eROLE.DEVASTATOR]] = 20;

for (var i = 0; i < array_length(unit_struct); i++) {
var _unit = unit_struct[i];
if (is_struct(_unit) && ally[i] == false) {
if (marine_dead[i] == 1 && marine_type[i] != "") {
var _role_priority_bonus = 0;
var _chief_librarian = $"Chief {_roles[eROLE.LIBRARIAN]}";
switch (_unit.role()) {
case obj_ini.role[100][eROLE.CHAPTERMASTER]:
_role_priority_bonus = 720;
break;
case "Forge Master":
case "Master of Sanctity":
case "Master of the Apothecarion":
case _chief_librarian:
_role_priority_bonus = 360;
break;
case _roles[eROLE.CAPTAIN]:
case _roles[eROLE.HONOURGUARD]:
case _roles[eROLE.ANCIENT]:
_role_priority_bonus = 160;
break;
case _roles[eROLE.VETERANSERGEANT]:
case _roles[eROLE.TERMINATOR]:
_role_priority_bonus = 80;
break;
case _roles[eROLE.VETERAN]:
case _roles[eROLE.SERGEANT]:
case _roles[eROLE.CHAMPION]:
case _roles[eROLE.CHAPLAIN]:
case _roles[eROLE.APOTHECARY]:
case _roles[eROLE.TECHMARINE]:
case _roles[eROLE.LIBRARIAN]:
case "Codiciery":
case "Lexicanum":
_role_priority_bonus = 40;
break;
case _roles[eROLE.TACTICAL]:
case _roles[eROLE.ASSAULT]:
case _roles[eROLE.DEVASTATOR]:
_role_priority_bonus = 20;
break;
case _roles[eROLE.SCOUT]:
default:
_role_priority_bonus = 0;
break;
}

var _priority = _unit.experience + _role_priority_bonus;
var _recovery_candidate = {
"id": i,
"unit": _unit,
"column_id": id,
"priority": _priority,
};

ds_priority_add(obj_ncombat.marines_to_recover, _recovery_candidate, _recovery_candidate.priority);
}
}

if (!is_struct(_unit) || ally[i] == true) continue;
if (marine_dead[i] != 1 || marine_type[i] == "") continue;
Comment on lines +45 to +46

@OH296 OH296 Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!is_struct(_unit) || ally[i] == true) continue;
if (marine_dead[i] != 1 || marine_type[i] == "") continue;
if (!is_struct(_unit) || ally[i] == true){
continue;
}
if (marine_dead[i] != 1 || marine_type[i] == ""){
continue;
}

we like to be explicit with the brackets on this project



var _role_title = _unit.role();
var _role_priority_bonus = struct_exists(_bonus_specific, _role_title) ? _bonus_specific[$ _role_title] : 0;

var _priority = _unit.experience + _role_priority_bonus;
var _recovery_candidate = {
"id": i,
"unit": _unit,
"column_id": id,
"priority": _priority,
};

ds_priority_add(obj_ncombat.marines_to_recover, _recovery_candidate, _recovery_candidate.priority);
}
}





/// @self Asset.GMObject.obj_pnunit
function add_vehicles_to_recovery() {
var _vehicles_priority = {
Expand Down
Loading