Skip to content

refactor: efficiency of manage constants - #1436

Open
OH296 wants to merge 2 commits into
Adeptus-Dominus:mainfrom
OH296:optomise_manage_constants
Open

refactor: efficiency of manage constants#1436
OH296 wants to merge 2 commits into
Adeptus-Dominus:mainfrom
OH296:optomise_manage_constants

Conversation

@OH296

@OH296 OH296 commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Summary by cubic

Consolidates and speeds up unit manage constants tooltip generation by moving equipment-attribute aggregation into shared helpers. Previously, scr_ui_manage built DR/HP/Armor lines with per-slot switches; now it calls _equip_data.set_attribute_string(attr) which iterates present_items and uses each item’s item_attribute_string(attr). Percentages for HP and Damage Resistance now use string_format_percentage for consistent display; zero-value attributes remain omitted.

  • Adds EquipmentStruct.item_attribute_string(attribute) returning "Name: value" ("" when 0) for damage_resistance_mod, hp_mod, and armour_value.
  • Adds UnitEquipment.set_attribute_string(attribute) to aggregate lines across equipped items.
  • Replaces manual loops in reset_manage_unit_constants with _equip_data.set_attribute_string("damage_resistance_mod"|"hp_mod"|"armour_value").
  • Updates item description tooltips to use string_format_percentage for hp_mod, damage_resistance_mod, ranged_mod, and melee_mod.

Review notes

  • Confirm present_items includes all slots that can affect these attributes and that the display order is acceptable.
  • Verify percentage formatting (signs/precision) matches UI expectations.
  • Ensure tooltips still hide zero-valued contributions and that no calculation logic changed (display-only refactor).

Written for commit 80b6af9. Summary will update on new commits.

Review in cubic

@github-actions github-actions Bot added Size: Small Type: Refactor Rewriting/restructuring code, while keeping general behavior labels Aug 12, 2026

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

5 issues found across 3 files

Confidence score: 4/5

  • In scripts/scr_equipment_struct/scr_equipment_struct.gml, item_attribute_string duplicates the stat→format switch already used in item_tooltip_desc_gen, which creates a drift risk where new stats render inconsistently across tooltips—extract a shared formatter or reuse the existing mapping.
  • In scripts/scr_equipment_struct/scr_equipment_struct.gml, item_attribute_string has missing Feather/JSDoc tags plus unreachable break statements and no explicit unknown-attribute fallback, which weakens maintainability and can produce ambiguous output paths—add @desc/@param/@returns, remove dead code, and return "" by default.
  • In scripts/scr_ui_manage/scr_ui_manage.gml, switching to set_attribute_string changes manage-screen tooltip ordering and DR sign presentation, so players may see different stat emphasis than before—confirm this behavior is intentional and align ordering/format with prior UI expectations if not.
  • In scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml, set_attribute_string can leave a trailing newline that surfaces as an extra blank line in HP tooltip text—trim the final newline or append separators conditionally.
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_unit_equip_functions/scr_unit_equip_functions.gml">

<violation number="1" location="scripts/scr_unit_equip_functions/scr_unit_equip_functions.gml:702">
P3: `set_attribute_string` appends `\n` after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the `hp` tooltip (`_hp_tool += _equip_data.set_attribute_string("hp_mod");` in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through `_m_string`) instead of appending it after each one.</violation>
</file>

<file name="scripts/scr_ui_manage/scr_ui_manage.gml">

<violation number="1" location="scripts/scr_ui_manage/scr_ui_manage.gml:176">
P3: Replacing the fixed-loop with `set_attribute_string` silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.</violation>
</file>

<file name="scripts/scr_equipment_struct/scr_equipment_struct.gml">

<violation number="1" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:322">
P2: This refactor duplicates the stat->format mapping that already lives in `item_tooltip_desc_gen` in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having `item_attribute_string` produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.</violation>

<violation number="2" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:322">
P2: Custom agent: **Code Quality Review**

The newly added `item_attribute_string` function is missing required JSDoc/Feather tags (`@desc`, `@param`, `@returns`), and the `attribute` parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add `/// @desc`, `/// @param {string} attribute`, and `/// @returns {string}` documentation above the function.</violation>

<violation number="3" location="scripts/scr_equipment_struct/scr_equipment_struct.gml:331">
P3: The `break;` after each `return _str;` inside `item_attribute_string` is dead code that can never execute. Remove the unreachable `break;` lines, add `return "";` as a default for unknown attributes, and terminate `var _str = $"{name}: "` with a semicolon so the refactor does not ship sloppy code.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

return item_desc_tooltip;
};

static item_attribute_string = function(attribute){

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: This refactor duplicates the stat->format mapping that already lives in item_tooltip_desc_gen in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having item_attribute_string produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.

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

<comment>This refactor duplicates the stat->format mapping that already lives in `item_tooltip_desc_gen` in the same constructor. A maintainer adding a new mod stat now has to update both switch statements, which works against the PR's stated goal of making constants easier to maintain. Consider having `item_attribute_string` produce the formatted value only (or drive both from a shared mapping) so new stats are added in one place.</comment>

<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
         return item_desc_tooltip;
     };
 
+    static item_attribute_string = function(attribute){
+        var _str = $"{name}: "
+        switch(attribute){
</file context>

Comment on lines +322 to +336
static item_attribute_string = function(attribute){
var _str = $"{name}: "
switch(attribute){
case "damage_resistance_mod":
if (damage_resistance_mod == 0){
return "";
}
_str += $"{string_format_percentage(damage_resistance_mod)}";
return _str;
break;
case "hp_mod":
if (hp_mod == 0){
return "";
}
_str += $"{string_format_percentage(hp_mod)}";

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: Custom agent: Code Quality Review

The newly added item_attribute_string function is missing required JSDoc/Feather tags (@desc, @param, @returns), and the attribute parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add /// @desc, /// @param {string} attribute, and /// @returns {string} documentation above the function.

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

<comment>The newly added `item_attribute_string` function is missing required JSDoc/Feather tags (`@desc`, `@param`, `@returns`), and the `attribute` parameter is untyped. The Code Quality rule requires JSDoc or Feather tags on every newly added function. Add `/// @desc`, `/// @param {string} attribute`, and `/// @returns {string}` documentation above the function.</comment>

<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
         return item_desc_tooltip;
     };
 
+    static item_attribute_string = function(attribute){
+        var _str = $"{name}: "
+        switch(attribute){
</file context>
Suggested change
static item_attribute_string = function(attribute){
var _str = $"{name}: "
switch(attribute){
case "damage_resistance_mod":
if (damage_resistance_mod == 0){
return "";
}
_str += $"{string_format_percentage(damage_resistance_mod)}";
return _str;
break;
case "hp_mod":
if (hp_mod == 0){
return "";
}
_str += $"{string_format_percentage(hp_mod)}";
/// @desc Returns a formatted attribute string for the item
/// @param {string} attribute The attribute key to format (e.g. "hp_mod", "damage_resistance_mod", "armour_value")
/// @returns {string} The formatted attribute string, or empty string if the attribute value is 0
static item_attribute_string = function(attribute){
var _str = $"{name}: "
switch(attribute){
case "damage_resistance_mod":
if (damage_resistance_mod == 0){
return "";
}
_str += $"{string_format_percentage(damage_resistance_mod)}";
return _str;
break;
case "hp_mod":
if (hp_mod == 0){
return "";
}
_str += $"{string_format_percentage(hp_mod)}";
return _str;
break;
case "armour_value":
if (armour_value == 0){
return "";
}
_str += $"{format_number_with_sign(armour_value)}";
return _str;
break;
}
}

var _str = "";
for (var i = 0; i < array_length(present_items); i++){
var _item = equipment[$ present_items[i]];
var _m_string = _item.item_attribute_string(attribute);

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: set_attribute_string appends \n after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the hp tooltip (_hp_tool += _equip_data.set_attribute_string("hp_mod"); in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through _m_string) instead of appending it after each one.

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

<comment>`set_attribute_string` appends `\n` after every non-empty item string, so when the last present item contributes, the returned string ends with a trailing newline. In the `hp` tooltip (`_hp_tool += _equip_data.set_attribute_string("hp_mod");` in scr_ui_manage) nothing is appended afterward, so the tooltip renders with a trailing blank line. Join with the separator between entries (and skip the round-trip through `_m_string`) instead of appending it after each one.</comment>

<file context>
@@ -695,6 +695,16 @@ function UnitEquipment(equipment_set, _unit = noone) constructor {
+        var _str = "";
+        for (var i = 0; i < array_length(present_items); i++){
+            var _item = equipment[$ present_items[i]];
+            var _m_string = _item.item_attribute_string(attribute);
+            _str +=  _m_string != "" ? _m_string + "\n" : "";
+        }
</file context>

_res_tool += $"{name}: {dr}%\n";
}
}
_res_tool += _equip_data.set_attribute_string("damage_resistance_mod");

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: Replacing the fixed-loop with set_attribute_string silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.

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

<comment>Replacing the fixed-loop with `set_attribute_string` silently changes the manage-screen tooltips: contributing items are now listed in weapon-1/weapon-2/armour/gear/mobi order instead of armour-first, and positive DR/armour values now render with a '+' prefix (e.g. "5%" becomes "+5%"). If the reorder or the added '+' is not intended, preserve the old slot order and plain formatting; otherwise call out the change explicitly.</comment>

<file context>
@@ -173,44 +173,7 @@ function reset_manage_unit_constants(unit) {
-                _res_tool += $"{name}: {dr}%\n";
-            }
-        }
+        _res_tool += _equip_data.set_attribute_string("damage_resistance_mod");
         _res_tool += $"CON: {round(unit.constitution / 2)}%";
 
</file context>

Comment on lines +331 to 351
break;
case "hp_mod":
if (hp_mod == 0){
return "";
}
_str += $"{string_format_percentage(hp_mod)}";
return _str;
break;
case "armour_value":
if (armour_value == 0){
return "";
}
_str += $"{format_number_with_sign(armour_value)}";
return _str;
break;
}
}

static special_value = function(special) {
if (is_struct(specials)) {
var _specials = struct_get_names(specials);

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: The break; after each return _str; inside item_attribute_string is dead code that can never execute. Remove the unreachable break; lines, add return ""; as a default for unknown attributes, and terminate var _str = $"{name}: " with a semicolon so the refactor does not ship sloppy code.

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

<comment>The `break;` after each `return _str;` inside `item_attribute_string` is dead code that can never execute. Remove the unreachable `break;` lines, add `return "";` as a default for unknown attributes, and terminate `var _str = $"{name}: "` with a semicolon so the refactor does not ship sloppy code.</comment>

<file context>
@@ -319,6 +319,33 @@ function EquipmentStruct(item_data = undefined, core_type = "", quality_request
+                }
+                _str += $"{string_format_percentage(damage_resistance_mod)}";
+                return _str;
+                break;
+            case "hp_mod":
+                if (hp_mod == 0){
</file context>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Size: Small Type: Refactor Rewriting/restructuring code, while keeping general behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant