fix: Many more improvements to creation screens#995
fix: Many more improvements to creation screens#995OH296 merged 2 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 decree: whitespace was tidied, a substantial UI module was introduced (draw-state helpers, buttons, radios, toggles, inputs), chapter loading was refactored to use a named identifier and new points logic, creation flow gained initialisation hooks, and RadioSet-based UIs were integrated and repositioned. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
scripts/scr_buttons/scr_buttons.gml (9)
219-260: Use the halign parameter; remove empty-arg call sites riskYou’ve added _halign but always force fa_center and several call sites now pass “,,”. This will either misalign text or hard-fail parsing. Honour the parameter and make skipping explicit via undefined.
Apply:
-function draw_unit_buttons(position, text, size_mod=[1.5,1.5],colour=c_gray,_halign=fa_center, font=fnt_40k_14b, alpha_mult=1, bg=false, bg_color=c_black){ +function draw_unit_buttons(position, text, size_mod=[1.5,1.5], colour=c_gray, _halign=fa_center, font=fnt_40k_14b, alpha_mult=1, bg=false, bg_color=c_black){ // TODO: fix halign usage // Store current state of all global vars add_draw_return_values(); - draw_set_font(font); - draw_set_halign(fa_center); + draw_set_font(font); + draw_set_halign(_halign); draw_set_valign(fa_middle); @@ - draw_set_color(colour); - draw_text_transformed((position[0] + x2)/2, (position[1] + y2)/2, _text, size_mod[0], size_mod[1], 0); + draw_set_color(colour); + var _tx = (_halign==fa_left) ? position[0]+3 : (_halign==fa_right) ? x2-3 : (position[0] + x2)/2; + draw_text_transformed(_tx, (position[1] + y2)/2, _text, size_mod[0], size_mod[1], 0);Also, please convert the TODO into a tracked issue per CODE_STYLE; I can open it if you wish.
341-401: Early returns leak draw state; string assignment used as comparison; bad call to draw_unit_buttons
- pop_draw_return_values() is after return → leaked draw state for the frame.
- style = "standard"/"pixel" assigns instead of compares.
- draw_unit_buttons(...) has an empty parameter slot (,,) which is unsafe; pass undefined or a real value.
Apply:
static draw = function(allow_click = true){ - add_draw_return_values(); - if (style = "standard"){ + add_draw_return_values(); + if (style == "standard"){ var _temp_alpha = alpha; if (disabled){ _temp_alpha = 0.5; allow_click = false; } - var _button_click_area = draw_unit_buttons(w > 0 ? [x1, y1, x2, y2] : [x1, y1] , label, [text_scale,text_scale],active ? color : inactive_col,,font,_temp_alpha); - } else if (style = "pixel"){ + var _button_click_area = draw_unit_buttons( + w > 0 ? [x1, y1, x2, y2] : [x1, y1], + label, [text_scale, text_scale], + active ? color : inactive_col, + fa_center, font, _temp_alpha + ); + } else if (style == "pixel"){ @@ - if (allow_click && active){ - var clicked = point_and_click(_button_click_area) || keystroke; - if (clicked){ + var _result = false; + if (allow_click && active){ + var clicked = point_and_click(_button_click_area) || keystroke; + if (clicked){ if (is_callable(bind_method)){ if (bind_scope != false){ var _method = bind_method; with (bind_scope){ _method(); } } else { bind_method(); } } - } - return clicked - } else { - return false; - } - pop_draw_return_values(); + _result = true; + } + } else { + _result = false; + } + pop_draw_return_values(); + return _result; }
411-437: Same pop-leak and empty args in PurchaseButton; fix and gate callabilityMirror the fixes: pass a valid _halign and ensure pop occurs before return.
Apply:
static draw = function(allow_click=true){ add_draw_return_values(); - var _but = draw_unit_buttons([x1, y1, x2, y2], label, [1,1],color,,,alpha); + var _but = draw_unit_buttons([x1, y1, x2, y2], label, [1,1], color, fa_center, font, alpha); @@ - if (allow_click && _allow_click){ - var clicked = point_and_click(_but) || keystroke; - if (clicked){ - if (is_callable(bind_method)){ - bind_method(); - } - obj_controller.requisition -= req_value; - } - return clicked - } else { - return false; - } - pop_draw_return_values(); + var _result = false; + if (allow_click && _allow_click){ + var clicked = point_and_click(_but) || keystroke; + if (clicked){ + if (is_callable(bind_method)) { + bind_method(); + } + obj_controller.requisition -= req_value; + _result = true; + } + } + pop_draw_return_values(); + return _result; }
457-489: slider_bar maths: undefined identifiers (increments/_rel_cur_pos) → runtime faultsSeveral variables are misspelt/undefined, causing logic failure.
Apply:
- width_increments = w/((value_limits[1]-value_limits[0])/value_increments); - var __rel_cur_pos = increments*(value - value_limits[0]); + var width_increments = w / ((value_limits[1] - value_limits[0]) / value_increments); + var _rel_cur_pos = ( (value - value_limits[0]) / value_increments ) * width_increments; @@ - var _lit_cur_pos = _rel_cur_pos+x1; - if (scr_hit(_rect) && !dragging){ - if (point_distance(_lit_cur_pos, 0, _mouse_pos[0], 0)>increments){ + var _lit_cur_pos = _rel_cur_pos + x1; + if (scr_hit(_rect) && !dragging){ + if (abs(_mouse_pos[0] - _lit_cur_pos) > width_increments){ if (mouse_check_button(mb_left)){ dragging = true } } }
589-624: pop_draw_return_values() is unreachable; leaks draw state; stray current_targetReturn precedes pop; also current_target is used without declaration, likely polluting global state.
Apply:
function drop_down(selection, draw_x, draw_y, options,open_marker){ add_draw_return_values(); + var _current_target = false; if (selection!=""){ var drop_down_area = draw_unit_buttons([draw_x, draw_y],selection,[1,1],c_green); draw_set_color(c_red); if (array_length(options)>1){ if (scr_hit(drop_down_area)){ - current_target=true; + _current_target = true; var roll_down_offset=4+string_height(selection); @@ - if (!scr_hit( + if (!scr_hit( draw_x, draw_y, draw_x + 5 +string_width(selection), draw_y+roll_down_offset, ) ){ open_marker = false; - if (current_target) then current_target=false; + _current_target = false; } } else { - current_target=false; + _current_target = false; } } } - return [selection,open_marker]; - pop_draw_return_values(); + var _ret = [selection, open_marker]; + pop_draw_return_values(); + return _ret; }
79-96: ReactiveString hit-box is zero when max_width < 0x2/y2 remain 0 unless max_width is set, so hit() never triggers for simple text.
Apply:
static update = function(data = {}){ @@ - if (max_width>-1){ - w = string_width_ext(text, -1, max_width); - h = string_height_ext(text, -1, max_width); - x2 = x1 + w; - y2 = y1 + h; - } + if (max_width > -1){ + w = string_width_ext(text, -1, max_width); + h = string_height_ext(text, -1, max_width); + } else { + w = string_width(text); + h = string_height(text); + } + x2 = x1 + w; + y2 = y1 + h;Also applies to: 102-119
747-767: RadioSet.changed never leaves the function scope; external readers breakroles_radio.changed is used elsewhere, but draw() only sets a local changed. Promote to an instance field and reset per draw.
Apply:
function RadioSet(options_array, title="", data = {})constructor{ toggles = []; current_selection = 0; + changed = false; @@ - static draw = function () { + static draw = function () { add_draw_return_values(); @@ - changed = false; + changed = false; // local for flow + self.changed = false; // external visibility @@ - if (_start_current_selection != current_selection) { - changed = true; - } + if (_start_current_selection != current_selection) { + changed = true; + self.changed = true; + }Also applies to: 786-868
121-190: Minor: LabeledIcon width/height calc only for text_position == "right"If other positions are added, w/h will be stale. Not urgent but worth guarding now.
1009-1092: Consistency: wrap InteractiveButton draw with draw-state push/popTo placate the UI Machine Spirit, standardise state save/restore like other widgets.
Apply:
draw = function() { + add_draw_return_values(); @@ - draw_set_halign(fa_left); + draw_set_halign(fa_left); + pop_draw_return_values(); };scripts/scr_chapter_new/scr_chapter_new.gml (1)
252-256: Stop on first match and guard thejsonflagBreak once the chapter is found; null-safe the
jsonflag to prevent undefined access.- if(chapter_identifier == obj_creation.all_chapters[c].name && obj_creation.all_chapters[c].json == true){ + if (chapter_identifier == obj_creation.all_chapters[c].name && (obj_creation.all_chapters[c].json ?? false)) { obj_creation.use_chapter_object = true; chapter_id = obj_creation.all_chapters[c].id; + break; }
📜 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 (7)
objects/obj_controller/Step_1.gml(0 hunks)objects/obj_tooltip/Step_1.gml(1 hunks)scripts/scr_buttons/scr_buttons.gml(15 hunks)scripts/scr_chapter_new/scr_chapter_new.gml(5 hunks)scripts/scr_creation/scr_creation.gml(1 hunks)scripts/scr_creation_draw_slides/scr_creation_draw_slides.gml(3 hunks)scripts/scr_role_setup/scr_role_setup.gml(1 hunks)
💤 Files with no reviewable changes (1)
- objects/obj_controller/Step_1.gml
🧰 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_role_setup/scr_role_setup.gmlscripts/scr_creation/scr_creation.gmlobjects/obj_tooltip/Step_1.gmlscripts/scr_creation_draw_slides/scr_creation_draw_slides.gmlscripts/scr_buttons/scr_buttons.gmlscripts/scr_chapter_new/scr_chapter_new.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_role_setup/scr_role_setup.gmlscripts/scr_creation/scr_creation.gmlobjects/obj_tooltip/Step_1.gmlscripts/scr_creation_draw_slides/scr_creation_draw_slides.gmlscripts/scr_buttons/scr_buttons.gmlscripts/scr_chapter_new/scr_chapter_new.gml
🔇 Additional comments (4)
objects/obj_tooltip/Step_1.gml (1)
5-5: LGTM: stack reset remains intactThe Machine Spirit approves. Reset after GC unchanged; no functional drift.
scripts/scr_role_setup/scr_role_setup.gml (1)
33-34: RadioSet layout shift: verify selection mapping and wrappingCentre + max_width=400 is sane. Confirm load_to_ships[0] ∈ {0,1,2} to avoid OOB and that labels won’t wrap into two rows at 400px. If you anticipate localisation, consider increasing max_width to prevent awkward wrapping.
Also applies to: 87-91
scripts/scr_creation_draw_slides/scr_creation_draw_slides.gml (1)
467-469: State sync OK; ensure chapter_type_radio exists on all code pathsGiven the new guard in scr_creation, this is fine. If you keep the duplicate init above, remove it to prevent churn.
scripts/scr_chapter_new/scr_chapter_new.gml (1)
519-526: Index origin for disadvantages differs from advantages
_open_disadvstarts at 1 while_open_advstarts at 0. Confirm UI indexing; otherwise this is an off-by-one rite.To align, consider:
-var _open_disadv = 1; +var _open_disadv = 0;
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Purpose and Description
Testing done
Related things and/or additional context