fix: various issues from new creation screen works#975
fix: various issues from new creation screen works#975OH296 merged 3 commits intoAdeptus-Dominus:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughBy the Omnissiah: gear-slot indexing moved to 0-based, several UI initialisers scoped to the obj_creation instance, RadioSet/ToggleButton draw/selection behaviour and ToggleButton.draw signature changed, draw-return context wrappers added, colour-role selection hardened, and role setup converted to widget-driven UI with new globals. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
OH296
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
scripts/scr_buttons/scr_buttons.gml (1)
530-538: Title layout gate uses draw_title (bool) instead of title (string).string_length(draw_title) is nonsensical and will not behave as intended. Use title.
Apply this diff:
- if (max_width > 0 && string_length(draw_title) < max_width){ + if (max_width > 0 && string_length(title) < max_width){ if (draw_title){ - draw_text(x1+(max_width/2) - (string_length(draw_title)/2), y1, title); + draw_text(x1 + (max_width/2) - (string_length(title)/2), y1, title); } } else { if (draw_title){ - draw_text(x1 + (string_length(draw_title)/2), y1, title); + draw_text(x1 + (string_length(title)/2), y1, title); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
objects/obj_creation_popup/Draw_0.gml(4 hunks)scripts/scr_buttons/scr_buttons.gml(7 hunks)scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml(2 hunks)scripts/scr_creation/scr_creation.gml(2 hunks)scripts/scr_livery_setup/scr_livery_setup.gml(2 hunks)scripts/scr_role_setup/scr_role_setup.gml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.gml
⚙️ CodeRabbit Configuration File
**/*.gml: - Macro constants require a space between the constant name and value. Without it, the compiler will throw an error. I.e.#macro ARR_body_parts["arm"]will crash the game, because there is no space between the array and the name of the macro.
- Color codes in the code shouldn't have any spaces in their id. I.e., color code
# 80bf40will crash the game.- All code should comply with the main GML documentation: https://manual.gamemaker.io/beta/en/GameMaker_Language/GML_Reference/GML_Reference.htm
Files:
scripts/scr_livery_setup/scr_livery_setup.gmlscripts/scr_complex_colour_kit/scr_complex_colour_kit.gmlscripts/scr_creation/scr_creation.gmlobjects/obj_creation_popup/Draw_0.gmlscripts/scr_buttons/scr_buttons.gmlscripts/scr_role_setup/scr_role_setup.gml
**/*.*
⚙️ CodeRabbit Configuration File
**/*.*: - Having humanly understandable and maintainable code is always the top most priority.
- DRY (Don't repeat yourself) principle is also very important.
- Ensure that the code is compliant with the CODE_STYLE.MD: https://github.com/Adeptus-Dominus/ChapterMaster/blob/main/docs/CODE_STYLE.md
- If a TODO comment is added, ask the user if you should create a GitHub issue for this TODO.
- If a TODO comment is deleted, remind the user if there is an active GitHub issue related to that comment.
Files:
scripts/scr_livery_setup/scr_livery_setup.gmlscripts/scr_complex_colour_kit/scr_complex_colour_kit.gmlscripts/scr_creation/scr_creation.gmlobjects/obj_creation_popup/Draw_0.gmlscripts/scr_buttons/scr_buttons.gmlscripts/scr_role_setup/scr_role_setup.gml
🔇 Additional comments (15)
objects/obj_creation_popup/Draw_0.gml (2)
61-63: Scoped call to set_complex_livery_buttons() is correct.Executing within obj_creation avoids accidental scope bleed. Approved.
193-195: Refreshing roles UI within obj_creation is appropriate.This ensures RadioSet is rebuilt against the correct instance state after persisting custom role. Sanctioned.
scripts/scr_buttons/scr_buttons.gml (4)
551-553: Explicitly driving ToggleButton active state is good.Assigning active based on current_selection clarifies state derivation and prevents stale toggles. Blessed.
577-580: Safe selection_val when nothing is selected.Returning noone on current_selection == -1 prevents invalid array access. Machine-spirits appeased.
616-618: Box button sizing update looks sensible.Using full string_width and a fixed icon height produces more predictable hitboxes. Approved.
642-699: Verify draw(is_active = active) Calls for Button ObjectsThe extension of the
draw(is_active = active)API is sound, but we must be sure we only pass the boolean state to button-type objects (those created viascripts/scr_buttons/scr_buttons.gml) rather than positional parameters.Bullet‐point action items:
- Only calls of the form
myButton.draw(true / false)
should target buttons fromscripts/scr_buttons/scr_buttons.gml.- All other
.draw(x, y, …)usages are Slate, Panel, Sprite, DataSlate, etc., and must keep their original signatures.- Please audit each
.draw(site on your button structs (e.g.tab_buttons.*,specialist_distribution_box,distribute_scouts_box,transfer_button, etc.) to confirm they now only pass a boolean and not coordinates or labels.Example locations to review:
scripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gmlaround lines 405–418scripts/scr_ui_manage/scr_ui_manage.gmllines 164–168, 185–189, 203–207, 215–217, 272–276, 1079–1227 (multiple)scripts/scr_transfer_marines/scr_transfer_marines.gml:17scripts/scr_trade/scr_trade.gml:508scripts/scr_role_setup/scr_role_setup.gml:85–87, 98–103scripts/scr_reequip_units/scr_reequip_units.gml:377–379, 700–702scripts/scr_manage_tags/scr_manage_tags.gml:156–158, 201–203, 242–246- and so on for every
.draw(that references a*_box,*_button, ortab_buttons.*No code snippet changes are required here—just verify and correct any mis-scoped
.drawcalls so that only true button objects receive the single-boolean argument.scripts/scr_livery_setup/scr_livery_setup.gml (2)
4-4: Draw state capture added.Capturing draw state up-front aligns with the new draw-return pattern elsewhere. Approved.
226-226: Draw state restore at end.pop_draw_return_values at the tail maintains render determinism. Ensure no early returns bypass it (none observed). Confirmed.
scripts/scr_creation/scr_creation.gml (2)
248-249: Position adjustment of advanced_helmet_livery.UI shift to x1: 477, y1: 515 is benign and matches bulk_armour_pattern positioning. Approved.
321-321: Affirmed:role_setup_objectssymbol present
The function is declared at scripts/scr_role_setup/scr_role_setup.gml:1 with zero arity. No missing symbols detected—proceed unhindered by the Omnissiah.scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml (2)
29-31: Guarding role_set against noone is sane.Mapping no selection to 0 for full_liveries averts sentinel indexing faults. Approved.
38-41: Harden role_set indexing with clamping
Replace the manual –1 check in scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml (lines 38–41):- if (role_set == -1){ - role_set = 1; - } + // clamp role_set to [1 … last valid index] + role_set = clamp(role_set, 1, max(1, array_length(_comp_livs) - 1));• Location: scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml:38–41
• Verify that _comp_livs is initialised with length ≥ 2 so that array_length(_comp_livs) – 1 ≥ 1.Optional: apply the same clamp pattern to any full_liveries index usages if that array can vary.
scripts/scr_role_setup/scr_role_setup.gml (3)
58-64: Draw context management is correctly paired
add_draw_return_values()is symmetrically closed withpop_draw_return_values(). No leaks of draw-state blessings detected.Also applies to: 268-268
69-69: roles_radio initialization is guaranteed before use
By the Omnissiah, the artefact is rightly consecrated prior to invocation—no null instance will manifest.
- In scripts/scr_creation/scr_creation.gml (line 65), roles_radio is constructed via
new RadioSet(...).- The creation routine then calls role_setup_objects() (line 320) before any draw‐event script runs.
The update/draw in scr_role_setup.gml therefore always has a valid roles_radio to work with. Original concern may be ignored.
Likely an incorrect or invalid review comment.
14-34: Fix tooltip newline escape; RadioSet API confirmedReplaced the incorrect “/n” with “\n” in the first option’s tooltip (and lowercased “start”), and verified that
current_selectionis indeed the correct RadioSet property.--- a/scripts/scr_role_setup/scr_role_setup.gml +++ b/scripts/scr_role_setup/scr_role_setup.gml @@ -17,7 +17,7 @@ str1 : "On Planet", font : fnt_40k_12, style : "box", - tooltip : $"On Planet/nCheck to have your Astartes Start on your home planet.", + tooltip : $"On Planet\nCheck to have your Astartes start on your home planet.", },
1 participant
Purpose and Description
Testing done
Related things and/or additional context