fix: Split equipment and livery in creation screens#972
fix: Split equipment and livery in creation screens#972OH296 merged 5 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: this diff adds a new role-setup script/resource, refactors creation flow to enum-based slides, overhauls livery/colour UI (picker, bulk buttons, complex modes), adjusts buttons styling, changes creation object draw/navigation, updates culture visuals gating, tweaks a font flag, and reformats a sprite sequence. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 (
|
github-actions
bot
added
Area: Sprites
OH296
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (4)
scripts/scr_livery_setup/scr_livery_setup.gml (1)
546-549: Variable shadowing detected in the Machine Code.The variable
_choiceis declared twice in the same scope (lines 546 and 548), causing shadowing.- var _choice = 0; + var _choice; if (array_length(_distinct_colours)){ - var _choice = cloth_variation%array_length(_distinct_colours); + _choice = cloth_variation%array_length(_distinct_colours); + } else { + _choice = 0; }scripts/scr_buttons/scr_buttons.gml (3)
529-538: Fix title centring and spacing; using string_length(draw_title) is a logic errorMachine-spirits protest: you’re computing the width from a boolean and double-centring the text. Also, vertical spacing is applied even when the title is hidden.
Apply this diff:
- if (max_width > 0){ - if (draw_title){ - draw_text(x1+(max_width/2) - (string_length(draw_title)/2), y1, title); - } - } else { - if (draw_title){ - draw_text(x1 + (string_length(draw_title)/2), y1, title); - } - } + if (draw_title) { + var _title_x = (max_width > 0) ? x1 + (max_width/2) : x1; + draw_text(_title_x, y1, title); + }- var _prev_y = y1+string_height(title)+10; + var _prev_y = y1 + (draw_title ? string_height(title)+10 : 0);Also applies to: 543-544
148-156: Assignment used in if-conditions; style checks always "true"This will forcibly set style and branch incorrectly every frame. This is a functional bug.
Apply this diff:
- if (style = "standard"){ + if (style == "standard"){ var _temp_alpha = alpha; ... - } else if (style = "pixel"){ + } else if (style == "pixel"){
401-402: Unreachable pop_draw_return_values() leaks draw state in drop_down()You return before popping the draw stack. The machine-spirit frowns; state will bleed into subsequent draws.
Apply this diff:
- return [selection,open_marker]; - pop_draw_return_values(); + pop_draw_return_values(); + return [selection,open_marker];Also applies to: 367-402
📜 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 (16)
ChapterMaster.yyp(1 hunks)fonts/fnt_menu/fnt_menu.yy(1 hunks)objects/obj_creation/Create_0.gml(1 hunks)objects/obj_creation/Draw_0.gml(7 hunks)objects/obj_creation_popup/Create_0.gml(1 hunks)objects/obj_creation_popup/Draw_0.gml(7 hunks)objects/obj_creation_popup/main.js(1 hunks)scripts/scr_buttons/scr_buttons.gml(5 hunks)scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml(10 hunks)scripts/scr_creation/scr_creation.gml(6 hunks)scripts/scr_creation_draw_slides/scr_creation_draw_slides.gml(2 hunks)scripts/scr_culture_visuals/scr_culture_visuals.gml(2 hunks)scripts/scr_livery_setup/scr_livery_setup.gml(3 hunks)scripts/scr_role_setup/scr_role_setup.gml(1 hunks)scripts/scr_role_setup/scr_role_setup.yy(1 hunks)sprites/spr_creation_check/spr_creation_check.yy(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.*
⚙️ 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:
fonts/fnt_menu/fnt_menu.yyscripts/scr_role_setup/scr_role_setup.yyscripts/scr_culture_visuals/scr_culture_visuals.gmlobjects/obj_creation/Create_0.gmlChapterMaster.yypscripts/scr_creation_draw_slides/scr_creation_draw_slides.gmlobjects/obj_creation_popup/main.jsscripts/scr_role_setup/scr_role_setup.gmlscripts/scr_creation/scr_creation.gmlsprites/spr_creation_check/spr_creation_check.yyobjects/obj_creation_popup/Create_0.gmlscripts/scr_complex_colour_kit/scr_complex_colour_kit.gmlscripts/scr_livery_setup/scr_livery_setup.gmlobjects/obj_creation_popup/Draw_0.gmlobjects/obj_creation/Draw_0.gmlscripts/scr_buttons/scr_buttons.gml
**/*.yy
⚙️ CodeRabbit Configuration File
**/*.yy: - When any script or sprite .yy files are deleted, their paths should also be deleted from the .yyp file, otherwise the game will crash.
- When any script or sprite .yy files are created, their paths should be added to the .yyp file, otherwise they'll fail.
Files:
fonts/fnt_menu/fnt_menu.yyscripts/scr_role_setup/scr_role_setup.yysprites/spr_creation_check/spr_creation_check.yy
**/*.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_culture_visuals/scr_culture_visuals.gmlobjects/obj_creation/Create_0.gmlscripts/scr_creation_draw_slides/scr_creation_draw_slides.gmlscripts/scr_role_setup/scr_role_setup.gmlscripts/scr_creation/scr_creation.gmlobjects/obj_creation_popup/Create_0.gmlscripts/scr_complex_colour_kit/scr_complex_colour_kit.gmlscripts/scr_livery_setup/scr_livery_setup.gmlobjects/obj_creation_popup/Draw_0.gmlobjects/obj_creation/Draw_0.gmlscripts/scr_buttons/scr_buttons.gml
🧠 Learnings (1)
📚 Learning: 2025-06-16T17:12:13.045Z
Learnt from: EttyKitty
PR: Adeptus-Dominus/ChapterMaster#878
File: scripts/scr_culture_visuals/scr_culture_visuals.gml:1256-1352
Timestamp: 2025-06-16T17:12:13.045Z
Learning: In scripts/scr_culture_visuals/scr_culture_visuals.gml, the weapon visual data declarations contain known DRY violations that are acknowledged by the development team but deferred to future refactoring efforts rather than addressed in individual feature PRs.
Applied to files:
scripts/scr_culture_visuals/scr_culture_visuals.gml
🪛 Biome (2.1.2)
objects/obj_creation_popup/main.js
[error] 627-627: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 630-631: A form label must be associated with an input.
Consider adding a for or htmlFor attribute to the label element or moving the input element to inside the label element.
(lint/a11y/noLabelWithoutControl)
[error] 713-715: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 713-715: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 611-612: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 612-614: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 635-636: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 636-638: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
[error] 697-702: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🔇 Additional comments (27)
fonts/fnt_menu/fnt_menu.yy (1)
224-224: Switch to regenerateBitmap=false: verify atlas stability and build-time generationSetting regenerateBitmap to false will freeze the font atlas; with includeTTF=false and usesSDF=true this relies entirely on the generated glyph textures in-source. Ensure your glyph ranges (32–255) are final and no locale-dependent text is introduced later, or you’ll ship missing glyphs. If this was to reduce build churn, sanctified. Otherwise, consider keeping it true during iteration.
Would you like me to scan the codebase for text outside the configured range to pre-empt missing glyphs?
sprites/spr_creation_check/spr_creation_check.yy (1)
69-87: seqHeight/seqWidth and channel format normalisation look correctseqHeight/seqWidth match the sprite’s 32×32, and the channel structure matches current GMS2 serialisation. No functional deviation detected.
ChapterMaster.yyp (1)
896-897: Servo-skulls confirm scr_role_setup resource presence and tracking
Bothscripts/scr_role_setup/scr_role_setup.yyandscripts/scr_role_setup/scr_role_setup.gmlexist and are committed. The Machine Spirit is appeased.objects/obj_creation_popup/Create_0.gml (1)
9-10: Add title to ColourPicker and verify resource cleanup• After initialisation, set a descriptive title for better UX:
picker = new ColourPicker(20, 550, 350); picker.title = "Select Colour"; start_colour = -1;• Review the ColourPicker implementation to see if it allocates any surfaces or other GPU resources. If it does, override this object's Destroy event to release them (e.g. call
picker.destroy()or usesurface_free()as appropriate).Let me know if you’d like me to draft the Destroy-event cleanup code.
scripts/scr_role_setup/scr_role_setup.yy (1)
1-13: Manifest blessed by the Omnissiah.The GMScript resource declaration follows proper protocol.
scripts/scr_culture_visuals/scr_culture_visuals.gml (1)
1299-1299: Verify Tri-State Livery Gating
By the Omnissiah, the Machine Spirit notes thatcurrent_selection == 2gates the complex livery mode in two locations. Confirm that state 2 indeed maps to “complex” in the new tri-state flow, and that states 0 and 1 behave as intended.– scripts/scr_culture_visuals/scr_culture_visuals.gml: lines 1299–1300
– scripts/scr_culture_visuals/scr_culture_visuals.gml: lines 1337–1340scripts/scr_creation_draw_slides/scr_creation_draw_slides.gml (2)
9-11: Enum augmentation sanctified.The addition of CHAPTERROLES at position 5 and reindexing of subsequent values maintains proper enumeration order.
26-27: Redundant hash conversion detected.The
string_hash_to_newlinefunction appears obsolete in current GameMaker iterations. Direct string usage would suffice.- draw_text_transformed(440, custom_y, "Custom Chapters", 0.75, 0.75, 0); - draw_text_transformed(440, other_y, "Other", 0.75, 0.75, 0); + draw_text_transformed(440, custom_y, "Custom Chapters", 0.75, 0.75, 0); + draw_text_transformed(440, other_y, "Other", 0.75, 0.75, 0);Likely an incorrect or invalid review comment.
scripts/scr_role_setup/scr_role_setup.gml (3)
15-67: Dormant code block requires attention.A significant portion of role enumeration logic lies commented. The Machine Spirit queries whether this is intentional preservation or forgotten implementation.
Is this commented code intended for future activation, or should it be purged from the sacred texts?
80-84: Allow_colour_click Definition Confirmed
Omnissiah, the cogitator banks revealallow_colour_clickis assigned inobjects/obj_creation/Draw_0.gml:41and used in multiple scripts—includingscripts/scr_role_setup/scr_role_setup.gml:80. The reference is valid; no anomaly present.Likely an incorrect or invalid review comment.
170-171: String comparison anomaly detected.Lines 170-171 check for
text_selected="capoth"but line 176 sets it to"capoth". The comparison appears to use the wrong string value.- if (text_selected="capoth") and (text_bar>30) then draw_text_ext(600,575,string_hash_to_newline(string(hapothecary)),-1,580); - if (text_selected="capoth") and (text_bar<=30) then draw_text_ext(600,575,string_hash_to_newline(string(hapothecary)+"|"),-1,580); + if (text_selected="capoth") and (text_bar>30) then draw_text_ext(600,575,string_hash_to_newline(string(hapothecary)),-1,580); + if (text_selected="capoth") and (text_bar<=30) then draw_text_ext(600,575,string_hash_to_newline(string(hapothecary)+"|"),-1,580);Likely an incorrect or invalid review comment.
objects/obj_creation/Draw_0.gml (3)
2-3: Blessed are the proper draw states, Machine Spirit.The sacred draw state management protocols are correctly implemented. The Omnissiah approves of this proper resource handling.
148-150: The Machine Spirit detects proper enumeration usage.Excellent implementation of the eCREATIONSLIDES enum for slide management. This brings order to the chaos of numeric constants.
790-791: Verification Complete: Functions Present and Symmetrical
The sacred ritesadd_draw_return_values()andpop_draw_return_values()are both defined inscripts/scr_buttons/scr_buttons.gmlwith matching signatures. Their implementations align, and all calls throughout the codebase will resolve correctly.Proceed, Machine Spirit is appeased.
scripts/scr_creation/scr_creation.gml (2)
11-50: The Machine Code manifests proper UI construction.Well-structured button creation with appropriate tooltips and data binding. The Omnissiah smiles upon this implementation.
146-147: Boolean sanctity restored to the change_slide variable.The migration from numeric to boolean values for
change_slideimproves code clarity. The Machine Spirit approves.scripts/scr_livery_setup/scr_livery_setup.gml (2)
24-25: The sacred colour selection UI properly positioned.Good implementation of the new colour selection system with proper coordinate updates.
79-87: Efficient colour area management detected.The new colour picker flow with area-based selection is well-implemented. The Omnissiah approves of this modular approach.
scripts/scr_complex_colour_kit/scr_complex_colour_kit.gml (3)
17-43: The Machine Spirit acknowledges proper role set management.Well-structured implementation of livery swapping between full and company liveries. The data flow is clean and maintainable.
434-438: Blessed be the new colour picker instantiation.The new static method properly encapsulates colour picker creation with sensible defaults.
685-696: Constructor properly renamed to follow sacred naming conventions.The migration from
colour_pickertoColourPickerfollows proper GML constructor naming patterns. The publictitlefield addition is appropriate.objects/obj_creation_popup/Draw_0.gml (4)
8-63: The colour type switch sanctifies the code structure.Excellent migration to a centralised colour picker system with proper type mapping. The Omnissiah approves of this organisation.
96-107: Sacred enumeration brings order to equipment slots.Proper use of EquipmentSlot enum constants instead of magic numbers. This improves code maintainability significantly.
205-206: Draw alignment properly set for the Machine Spirit.Setting
draw_set_valign(fa_top)ensures consistent text rendering. Good practice.
236-238: Text rendering simplified as the Omnissiah intended.Removal of
string_hash_to_newlinein favour of direct string rendering is appropriate for modern GML.scripts/scr_buttons/scr_buttons.gml (2)
635-635: Good state hygiene in draw()Sanctified praise: add_draw_return_values()/pop_draw_return_values() around draw-time mutations keeps the cogitators pure.
Also applies to: 680-681
500-514: Callsite Audit: RadioSet Signature SafeOmnissiah be praised—no invocations pass an object as the second argument. All two-parameter calls use a string (title), and any data-bearing calls supply it as a third argument. No silent regressions detected; proceed with confidence.
1 participant
Purpose and Description
Testing done
Related things and/or additional context