refactor: Use role lookup for marine recovery

[SweetZJ]

I refactored the add_marines_to_recovery function because before that logic was a giant nest of if statements and I was taught that is not great practise so I attempted to simplify it by:

• creating a structure to hold all of the values which says how important a marine is to apothacary instead of the switch case used inside of the for loop which goes through every killed marine

• reducing the amount of nested if statements by using continue after the conditional, just to make it look better

Testing:
I'm new and have basically no clue how to test, so I just ran the program, fought ten battles before the change, and then fought ten battles against similar opposing forces after, and what died and what got revived SEEMED to be somewhat similar.

*Note: is this a copy of a prior commit? Yes:
Reason: I managed to mess up my branch something fierce and now I just don't know why it isn't able to show new updates, so I am just recreating this here after a decent amount of futile trying.

---

Summary by cubic

Refactors add_marines_to_recovery to replace the role priority switch with a dict lookup built from active_roles(), and shortens the nested if blocks with continue guards.

• Builds _role_priority_bonuses once with the same role-to-priority values the switch used, including explicit 0 entries for aspirant roles and SCOUT.
• Looks up the bonus per unit and treats undefined entries as 0, preserving the original default behavior.
• Fixes the switch fall-through where the last matching case won; each role now resolves unambiguously.
• CHAPTERMASTER now resolves via _roles[eROLE.CHAPTERMASTER] instead of obj_ini.player_role_data[eROLE.CHAPTERMASTER].role.

^{Written for commit f59f788. Summary will update on new commits.}

[OH296]

/review

[EttyKitty]

/review

[lyra-the-bot]

Good direction - flattening the nesting with guard continues and replacing the per-iteration switch with a lookup table is more readable, and the guard inversion (ally, marine_dead, marine_type) preserves the original semantics.

Blocking issue: the leftover if on line 53 has no body and turns the var _specific_bonus declaration on line 55 into its conditional branch, so SCOUT (bonus 0, falsy) and unknown roles (undefined, falsy) skip the declaration and reuse a stale value or error. It must be deleted.

Also, the aspirant roles newly scoring 20 instead of the old default 0 is an unacknowledged behavior change - please confirm it is intentional or revert it. Given the manual-only testing (10 battles before/after), keep this as REQUEST_CHANGES until the lookup bug and the priority question are resolved.

Minor follow-ups: build the table once (static), cache _unit.role(), and use ?? 0 instead of the explicit undefined check.

[lyra-the-bot]

Leftover line with no body or semicolon. In GML this parses as if (expr) <next statement>, so var _specific_bonus = ... on the next line becomes the conditional branch. For SCOUT (0, falsy) and any unknown role (undefined, falsy) the declaration is skipped, leaving a stale value from the previous iteration or an undeclared variable. Delete this line - line 55 already does the lookup.

Suggested change

	if _role_priority_bonuses[$ _unit.role()]
	var _role = _unit.role();
	var _specific_bonus = _role_priority_bonuses[$ _role] ?? 0;

[lyra-the-bot]

Behavior change vs the old switch: these four aspirant roles fell into default: 0 before, now they score 20. Nothing in the PR description mentions this. If intentional, call it out; otherwise remove these entries so unknown/aspirant roles stay at 0 as before.

[lyra-the-bot]

Calls _unit.role() twice (line 53 and here) and hand-rolls the undefined fallback. Cache the role once and use nullish coalescing per the style guide:

Suggested change

	var _specific_bonus = _role_priority_bonuses[$ _unit.role()];
	var _specific_bonus = _role_priority_bonuses[$ _unit.role()] ?? 0;

This also makes the if (_specific_bonus == undefined) block below redundant - prefer is_undefined() over == undefined if you keep the explicit check.

[lyra-the-bot]

Table is rebuilt on every call, which undercuts the perf goal. Since role names are fixed per game, build it once:

Suggested change

	var _role_priority_bonuses = {};
	static _role_priority_bonuses = undefined;
	if (_role_priority_bonuses == undefined) { ... build ... }

or a static struct literal. Also: line 40 is missing its trailing ;, lines 35-40/53/62 use tabs instead of 4 spaces, and the commented-out line 62 should be deleted.

[SweetZJ]

@OH296

Hello, sorry to bother after you approved the change but Lira_bot brought up an interesting suggestion:

 "use nullish coalescing per the style guide": var _specific_bonus = _role_priority_bonuses[$ _unit.role()] ?? 0; (essentially              make less explicit than if statement check I added instead)

I may be misremembering but didn't you say that you guys prefered more explicit to less? Does this style guide thing say use less explicit lines in these situations? Also isn't a style guide documentation? I was under the impression you guys didn't have any of that, is there actually good documentation I can read that'll help me?

[SweetZJ]

I believe I am finished with correcting my own self made errors

I'm not sure if this is the correct way to do things but this green button is here so I'm submitting for review

[The-Real-Nyx]

The first and third suggestions are just style suggestions, so apply them or ignore them as you want.

The second one should be applied though as the current version in the branch will create a bug.

[The-Real-Nyx]

Also your title is too long it needs to be at or under 50 characters. You don't have to use this but I would suggest something like "refactor: Use role lookup for marine recovery" over the current title.

Also also if you ever need help with git stuff feel free to ask in the discord.

[SweetZJ]

Hello, I'm curious what your first suggestion does:

(old)/// @self Asset.GMObject.obj_pnunit

(new)/// @desc Queues this column's dead player marines in obj_ncombat.marines_to_recover, ranked by experience plus a role bonus.
/// @self Asset.GMObject.obj_pnunit
/// @returns {Undefined}

is this just documentation for when someone sees the definition of the function or does this change also affect the popup someone sees when they hover over the function in Gamaker?

I'll impliment it cause I know i'd have loved any additional detail when I was first looking over this, but I was just curious what the comments do.

[SweetZJ]

I fixed the stuff found by first reviewer, but don't have the time to look it over for quite the bit to see if I missed anything when I first looked it over before the reviewer found what they found, so am unsure if I missed anything else on top of that.

[The-Real-Nyx]

Hello, I'm curious what your first suggestion does:

(old)/// @self Asset.GMObject.obj_pnunit

(new)/// @desc Queues this column's dead player marines in obj_ncombat.marines_to_recover, ranked by experience plus a role bonus. /// @self Asset.GMObject.obj_pnunit /// @returns {Undefined}

is this just documentation for when someone sees the definition of the function or does this change also affect the popup someone sees when they hover over the function in Gamaker?

I'll impliment it cause I know i'd have loved any additional detail when I was first looking over this, but I was just curious what the comments do.

It's both. But yes mainly it's for feather which is the code completion/intellisense equiv that the IDE ships with.

The manual I linked is just one page; there are a few more with details on Feather and the style of JSDoc comments it expects. So if you add any JSDoc comments yourself in the future, reference those. You can also reference my existing JSDoc comments in the codebase for exact styling, since Feather can be quite flexible about exact styling on many things. For example, types (in like params or returns) can be Types or types, feather is okay with either, but I use Types with the capital (Types meaning Bool, Real, String, etc.). To be clear, though, the style choices I use in my JSDoc comments are just my opinionated choices. They are not more or less correct than any other styling Feather accepts, so you don't have to follow them, but do decide on some style and then be consistent with that.

[The-Real-Nyx]

I fixed the stuff found by first reviewer, but don't have the time to look it over for quite the bit to see if I missed anything when I first looked it over before the reviewer found what they found, so am unsure if I missed anything else on top of that.

If you are talking about the comments from the bot, Lyra, they are all in the branch, not relevant, or would reintroduce a bug (I checked). If you are talking more broadly, then I at least don't see anything in the code that needs changes.

Since it's been 4 days since Etty queued up Lyra you are probably safe to ping them for another review by the end of tomorrow if they haven't already looked at it by then.