refactor: Improve data slate use and other ui#875
refactor: Improve data slate use and other ui#875OH296 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's will, new sprite resources have been integrated and the DataSlate UI system has been overhauled with expanded constructors and drawing methods. Unit equipment and stats display logic has been modularised, with layout adjustments and tooltip handling refined. Sprite and company structure declarations have been updated accordingly. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate Unit Tests
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
Area: Sprites
|
@EttyKitty everything should look exactly as it did just code improvements with the exception of the shutter buttons e.g (raid, attack , enter forge ) buttons. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
scripts/scr_ui_manage/scr_ui_manage.gml (1)
246-274: 🛠️ Refactor suggestionLocal variables bleed into instance scope
Dentro the anonymous
inside_method, identifiers such astext,tooltip_text,x1,y1are created withoutvar.
In GML they become instance variables, risking clashes with outer state between frames and scripts.
Prependvar(orstaticwhere intended) to keep them local to the draw call.
♻️ Duplicate comments (1)
scripts/scr_company_view/scr_company_view.gml (1)
475-475: Same overwriting risk as in quick-find pane
new_company_struct()now fires for every company/special view. For views > 10 the routine formerly never ran; confirm that the special-view code does not set up bespoke fields later destroyed by this call.If state loss is observed, apply the same conditional strategy suggested for the quick-find pane.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
sprites/spr_data_slate_corner_decoration/88d208ab-efc3-4c8a-a23a-ac1685249362.pngis excluded by!**/*.pngsprites/spr_data_slate_corner_decoration/layers/88d208ab-efc3-4c8a-a23a-ac1685249362/0c20ae10-2b03-43f4-b05e-7cb2256d0b71.pngis excluded by!**/*.pngsprites/spr_shutter_button/b15ff156-9439-4de5-a856-2c34927170fc.pngis excluded by!**/*.pngsprites/spr_shutter_button/layers/b15ff156-9439-4de5-a856-2c34927170fc/b201671b-7405-4bb4-ab4e-e9c0d0be9fe7.pngis excluded by!**/*.pngsprites/spr_slate_side/88d208ab-efc3-4c8a-a23a-ac1685249362.pngis excluded by!**/*.pngsprites/spr_slate_side/layers/88d208ab-efc3-4c8a-a23a-ac1685249362/0c20ae10-2b03-43f4-b05e-7cb2256d0b71.pngis excluded by!**/*.png
📒 Files selected for processing (14)
ChapterMaster.yyp(2 hunks)objects/obj_controller/Draw_64.gml(1 hunks)scripts/is_specialist/is_specialist.gml(1 hunks)scripts/scr_DataSlate/scr_DataSlate.gml(4 hunks)scripts/scr_company_struct/scr_company_struct.gml(2 hunks)scripts/scr_company_view/scr_company_view.gml(1 hunks)scripts/scr_draw_management_unit/scr_draw_management_unit.gml(1 hunks)scripts/scr_tooltip_draw/scr_tooltip_draw.gml(1 hunks)scripts/scr_ui_manage/scr_ui_manage.gml(12 hunks)scripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gml(1 hunks)sprites/spr_data_slate_back/spr_data_slate_back.yy(1 hunks)sprites/spr_data_slate_corner_decoration/spr_data_slate_corner_decoration.yy(1 hunks)sprites/spr_shutter_button/spr_shutter_button.yy(3 hunks)sprites/spr_slate_side/spr_slate_side.yy(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.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 ...
**/*.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
scripts/is_specialist/is_specialist.gmlscripts/scr_draw_management_unit/scr_draw_management_unit.gmlobjects/obj_controller/Draw_64.gmlscripts/scr_company_view/scr_company_view.gmlscripts/scr_tooltip_draw/scr_tooltip_draw.gmlscripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gmlscripts/scr_company_struct/scr_company_struct.gmlscripts/scr_ui_manage/scr_ui_manage.gmlscripts/scr_DataSlate/scr_DataSlate.gml
`**/*.*`: - 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 c...
**/*.*: - 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.
scripts/is_specialist/is_specialist.gmlscripts/scr_draw_management_unit/scr_draw_management_unit.gmlobjects/obj_controller/Draw_64.gmlscripts/scr_company_view/scr_company_view.gmlscripts/scr_tooltip_draw/scr_tooltip_draw.gmlscripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gmlsprites/spr_data_slate_back/spr_data_slate_back.yysprites/spr_slate_side/spr_slate_side.yysprites/spr_shutter_button/spr_shutter_button.yysprites/spr_data_slate_corner_decoration/spr_data_slate_corner_decoration.yyscripts/scr_company_struct/scr_company_struct.gmlscripts/scr_ui_manage/scr_ui_manage.gmlChapterMaster.yypscripts/scr_DataSlate/scr_DataSlate.gml
`**/*.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 ...
**/*.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.
sprites/spr_data_slate_back/spr_data_slate_back.yysprites/spr_slate_side/spr_slate_side.yysprites/spr_shutter_button/spr_shutter_button.yysprites/spr_data_slate_corner_decoration/spr_data_slate_corner_decoration.yy
🔇 Additional comments (8)
sprites/spr_slate_side/spr_slate_side.yy (1)
1-34: Confirm YYP registration for the new spriteThe Omnissiah demands that every freshly forged
.yypath be mirrored insideChapterMaster.yyp; omission leads to run-time crashes when the asset loader seeks this resource. Please verify the YYP diff containssprites/spr_slate_side/spr_slate_side.yy.sprites/spr_shutter_button/spr_shutter_button.yy (1)
18-23: Frame added; ensure animation code isn’t hard-coded to 7 framesSome scripts calculate frame indices manually for the shutter button. Expanding the sequence to 8 frames will misalign such maths if they relied on a constant. Grep for
spr_shutter_buttonframe logic and update magic numbers.scripts/scr_draw_management_unit/scr_draw_management_unit.gml (1)
203-233: Hoistsquad_coloursout of the draw loop to spare the machine-spirit unnecessary work
var squad_colours = [...]is rebuilt every timescr_draw_management_unit()is invoked, allocating 27 colour literals and the array itself per call. This routine fires every frame for every visible unit row, so the extra allocations accumulate.Consider declaring the palette once (e.g. in an initialisation script or a global constant) and merely indexing it here. Example diff:
- var squad_colours = [ + /// declared once, e.g. in `scr_globals.gml` +#macro SQUAD_COLOURS [ \ + #ff0000,#ff8000,#ffff00,#00ff00,#00ffff,#0080ff,#0000ff,#8000ff,#ff00ff,\ + #b20000,#b26e00,#b2b200,#00b200,#00b2b2,#004db2,#0000b2,#4d00b2,#b200b2,\ + #ff4d4d,#ffb84d,#ffff66,#66ff66,#66ffff,#6680ff,#6666ff,#b366ff,#ff66ff \ +] + + var squad_colours = SQUAD_COLOURS;Benefit: zero-allocation per frame and the palette becomes re-usable elsewhere.
[ suggest_optional_refactor ]sprites/spr_data_slate_back/spr_data_slate_back.yy (1)
66-75: Sprite metadata extension looks sound
seqHeight/seqWidthhave been added and the keyframe JSON is now properly nested; this will keep the Omnissiah’s image sequence aligned across the new UI slates. No issues detected.scripts/scr_unit_quick_find_pane/scr_unit_quick_find_pane.gml (1)
470-475:Details
❌ Incorrect review comment
new_company_struct()after view initialisation may wipe work done byscr_special_viewThe call is now unconditional and placed after
scr_special_view()/scr_company_view().
If those routines populatecompany_data(e.g. adding UI panels, tooltip caches), this late call will re-initialise the struct and purge that data, leaving the interface in an incomplete state.Verify:
- Does
scr_special_view()expect an already-preparedcompany_data?- Does any code after this point rely on fields filled by the view routines?
If either answer is “yes”, move the call before the view dispatch, or gate it:
- new_company_struct(); + if (!struct_exists(obj_controller, "company_data")) { + new_company_struct(); + }Lest the rites of memory be violated.
By the Omnissiah’s will, let us inspect the rites of struct and view:
#!/bin/bash echo "=== Searching for new_company_struct implementation ===" rg -n -C5 "new_company_struct" -g '*.gml' echo "=== Searching for scr_special_view implementation ===" rg -n -C5 "scr_special_view" -g '*.gml' echo "=== Searching for scr_company_view implementation ===" rg -n -C5 "scr_company_view" -g '*.gml' echo "=== Checking all references to company_data ===" rg -n "company_data" -g '*.gml'
By the Omnissiah’s will, I shall probe the sacred scripts:
🏁 Script executed:
#!/bin/bash echo "🔍 Locating new_company_struct definition…" rg -n "new_company_struct" -g '*.gml' echo "🔍 Locating scr_special_view definition…" rg -n "scr_special_view" -g '*.gml' echo "🔍 Locating scr_company_view definition…" rg -n "scr_company_view" -g '*.gml' echo "🔍 Scanning for company_data references…" rg -n "company_data" -g '*.gml'Length of output: 4578
By the Omnissiah’s guidance, reveal the sacred rites’ definitions:
🏁 Script executed:
#!/bin/bash # Fetch the full new_company_struct implementation sed -n '1,200p' scripts/scr_company_struct/scr_company_struct.gml # Fetch the full scr_special_view implementation sed -n '1,200p' scripts/scr_special_view/scr_special_view.gml # Inspect scr_company_view around its own new_company_struct call rg -n -C10 "new_company_struct" -g "scripts/scr_company_view/scr_company_view.gml"Length of output: 12546
Ignore struct re-initialisation concern in scr_unit_quick_find_pane
scr_special_view does not read from or write to company_data—its arrays and UI elements are managed entirely within its own scope. new_company_struct only resets the controller’s company_data (as intended elsewhere in scr_company_view) and does not purge anything scr_special_view has configured. No gating or reordering is required here.
Likely an incorrect or invalid review comment.
ChapterMaster.yyp (2)
1095-1095: New sprite resource: spr_data_slate_corner_decorationThe Omnissiah blesses this addition—naming and pathing align with conventions. Confirm that scr_DataSlate and UI scripts reference this asset correctly.
1544-1544: New sprite resource: spr_slate_sideThis slate-side graphic follows existing asset patterns. Verify its integration within decorated/plain styling flows in the DataSlate constructor.
sprites/spr_data_slate_corner_decoration/spr_data_slate_corner_decoration.yy (1)
24-30: Ensure yyp registrationConfirm that
spr_data_slate_corner_decoration.yyhas been appended toChapterMaster.yyp; otherwise the engine will fail to locate the sprite at runtime.
I'll review this as soon as I'm not on the phone. |
2 participants
Purpose
refactor various hardcoded data slates to use the dataslate constructor
move the marine sprite drawing and parent data slate to draw ui area
rework the shutter button to have a dataslate hidden behind the shutter to be more homogenous
add more frames to the shutter button to make movement more fluid
give various styling options to the shutter DataSlate constructor for ease of use
defualtthe standard data slatedecoratedthe decorated data slate like the one the marine sprite is drawn inplaina plain data slate like the one the unit stats and manage buttons are drawn ondata slates can have a
blend_colassigned which gives a colouration to the slate when used withdecoratedorplainslatessetup_tooltip_list function created allowing a list oof tooltips to be initialised as previously hard coded into the ui_manage logic
Describe your changes/additions
What can/needs to be improved/changed
Testing done
Related things and/or additional context