Improve input mappings with conflict handling and unbound warnings - #409
Merged
ikostan merged 79 commits intoFeb 18, 2026
Merged
Conversation
…ing any labels #408 Root Cause: _ready() + load_input_mappings() logic does not guarantee defaults exist on first run in HTML5 export. Some actions (menu keys) remain empty in InputMap. Effect: UI shows blank key labels on first load. Fixed after save because save_input_mappings() writes them. Solution: Add _ensure_defaults_saved() after load and save if any defaults were added. This ensures the UI always has proper InputMap entries, even on web first-run.
Contributor
Reviewer's GuideRefactors and hardens input mapping, conflict handling, and device-aware UI flows across settings, key mapping, main menu, and gameplay, while adding helper APIs, HUD warnings, and expanded tests and docs to support robust per-device bindings and web deployment. Sequence diagram for conflict-aware input remapping flowsequenceDiagram
actor Player
participant InputRemapButton
participant KeyMappingMenu
participant Settings
participant InputMap
participant MainScene
Player->>InputRemapButton: press button
InputRemapButton->>InputRemapButton: _on_pressed()
InputRemapButton->>InputRemapButton: listening = true
Player->>InputRemapButton: input event (key/button/axis)
InputRemapButton->>InputRemapButton: _input(event)
InputRemapButton->>InputRemapButton: build new_event
InputRemapButton->>Settings: get_conflicting_actions(new_event, action)
Settings-->>InputRemapButton: conflicts Array
alt conflicts not empty and not same as existing binding
InputRemapButton->>KeyMappingMenu: get_first_node_in_group(key_mapping_menu)
InputRemapButton->>KeyMappingMenu: show_conflict_dialog(self, new_event, conflicts)
KeyMappingMenu->>KeyMappingMenu: store current_remap_button, current_pending_event, current_conflicts
KeyMappingMenu->>KeyMappingMenu: conflict_dialog.popup_centered()
alt player confirms reassign
Player->>KeyMappingMenu: confirm conflict_dialog
KeyMappingMenu->>KeyMappingMenu: _on_conflict_confirmed()
loop for each conflicting action
KeyMappingMenu->>Settings: events_match(ev, current_pending_event)
Settings-->>KeyMappingMenu: bool
KeyMappingMenu->>InputMap: action_erase_event(conflict_action, ev)
end
KeyMappingMenu->>InputRemapButton: erase_old_event()
KeyMappingMenu->>InputMap: action_add_event(action, current_pending_event)
KeyMappingMenu->>Settings: save_input_mappings()
KeyMappingMenu->>InputRemapButton: finish_remap()
KeyMappingMenu->>KeyMappingMenu: _clear_conflict_state()
KeyMappingMenu->>KeyMappingMenu: update_all_remap_buttons()
KeyMappingMenu->>MainScene: clear_unbound_warning()
else player cancels
Player->>KeyMappingMenu: cancel conflict_dialog
KeyMappingMenu->>KeyMappingMenu: _on_conflict_canceled()
KeyMappingMenu->>InputRemapButton: button_pressed = false
KeyMappingMenu->>InputRemapButton: listening = false
KeyMappingMenu->>InputRemapButton: update_button_text()
KeyMappingMenu->>KeyMappingMenu: _clear_conflict_state()
end
else no conflicts
InputRemapButton->>InputRemapButton: erase_old_event()
InputRemapButton->>InputMap: action_add_event(action, new_event)
InputRemapButton->>InputRemapButton: finish_remap()
end
Sequence diagram for Start Game with unbound controls warningsequenceDiagram
actor Player
participant MainMenu
participant Settings
participant Globals
participant KeyMappingMenu
participant LoadingScreen
Player->>MainMenu: press Start button
MainMenu->>MainMenu: _on_start_pressed()
MainMenu->>Settings: has_unbound_critical_actions_for_current_device()
Settings-->>MainMenu: bool
alt critical actions unbound
MainMenu->>MainMenu: start_button.disabled = true
MainMenu->>MainMenu: unbound_dialog.popup_centered()
alt player chooses Open Key Mapping
Player->>MainMenu: confirm unbound_dialog
MainMenu->>Globals: load_key_mapping(ui_panel)
Globals->>ui_panel: visible = false
Globals->>Globals: hidden_menus.push_back(ui_panel)
Globals->>KeyMappingMenu: instantiate and add_child
KeyMappingMenu-->>Player: key mapping menu visible
else player chooses Start Anyway
Player->>MainMenu: press cancel button
MainMenu->>MainMenu: unbound_dialog.hide()
MainMenu->>Globals: load_scene_with_loading(main_scene.tscn)
Globals->>Globals: next_scene = main_scene.tscn
Globals->>LoadingScreen: change_scene_to_file(loading_screen.tscn)
LoadingScreen-->>Player: loading screen shows
end
MainMenu->>MainMenu: unbound_dialog.visibility_changed
MainMenu->>MainMenu: if not visible
MainMenu->>MainMenu: start_button.disabled = false
MainMenu->>MainMenu: start_button.grab_focus()
else no unbound critical actions
MainMenu->>Globals: load_scene_with_loading(main_scene.tscn)
Globals->>Globals: next_scene = main_scene.tscn
Globals->>LoadingScreen: change_scene_to_file(loading_screen.tscn)
LoadingScreen-->>Player: loading screen shows
end
Sequence diagram for in-game unbound input HUD warningssequenceDiagram
actor Player
participant MainScene
participant Settings
Player->>MainScene: input event (key/button/axis)
MainScene->>MainScene: _input(event)
MainScene->>MainScene: ignore echo and small axis jitter
alt event is significant and from key or gamepad
MainScene->>Settings: is_event_bound(event)
Settings-->>MainScene: bool
alt event is unbound and no current unbound key message
MainScene->>MainScene: _showing_unbound_key_message = true
MainScene->>Settings: get_event_device_type(event)
Settings-->>MainScene: device_type
MainScene->>Settings: get_pause_binding_label_for_device(device_type)
Settings-->>MainScene: pause_label
MainScene->>MainScene: build HUD text with pause_label
MainScene->>MainScene: show_message(text, MessageType.KEY_PRESS_UNBOUND)
MainScene->>MainScene: message_label.visible = true
MainScene->>MainScene: await timer 4.0 seconds
MainScene->>MainScene: message_label.visible = false
MainScene->>MainScene: _showing_unbound_key_message = false
else event is bound or message already showing
MainScene-->>Player: no HUD message change
end
else unsupported event type
MainScene-->>Player: ignore
end
Class diagram for updated input mapping and messaging systemclassDiagram
class Settings {
<<Node>>
+const LEGACY_MIGRATION_KEY String
+const CONFIG_PATH String
+const CRITICAL_ACTIONS Array~String~
+const ACTIONS Array~String~
+const DEFAULT_KEYBOARD Dictionary
+const DEFAULT_GAMEPAD Dictionary
-_needs_save bool
+_ready() void
+load_input_mappings(path String, actions Array~String~) void
+save_input_mappings(path String, actions Array~String~) void
+reset_to_defaults(device_type String) void
+has_unbound_critical_actions() bool
+has_unbound_critical_actions_for_current_device() bool
+get_pause_binding_label() String
+get_pause_binding_label_for_device(device_type String) String
+is_event_bound(event InputEvent) bool
+get_conflicting_actions(event InputEvent, exclude_action String) Array~String~
+events_match(a InputEvent, b InputEvent) bool
+save_last_input_device(device String) void
+load_last_input_device() void
+get_event_device_type(event InputEvent) String
+get_event_label(ev InputEvent) String
-_add_missing_defaults(config ConfigFile) bool
-_migrate_legacy_unbound_states() void
-_deserialize_and_add(action String, serialized String) void
}
class Globals {
<<Node>>
enum LogLevel
+const REMAP_PROMPT_KEYBOARD String
+const REMAP_PROMPT_GAMEPAD String
+current_log_level LogLevel
+enable_debug_logging bool
+options_instance CanvasLayer
+hidden_menus Array~Node~
+options_open bool
+key_mapping_scene PackedScene
+previous_scene String
+options_scene PackedScene
+next_scene String
+current_input_device String
+_ready() void
+log_message(msg String, level LogLevel) void
+ensure_initial_focus(candidate Control, allowed_controls Array~Control~, context String) void
+load_key_mapping(menu_to_hide Node) void
+load_scene_with_loading(path String) void
+_load_settings(path String) void
}
class KeyMappingMenu {
<<CanvasLayer>>
+js_window Variant
+os_wrapper OSWrapper
+js_bridge_wrapper JavaScriptBridgeWrapper
+conflict_dialog ConfirmationDialog
+current_remap_button InputRemapButton
+current_pending_event InputEvent
+current_conflicts Array~String~
+_ready() void
+show_conflict_dialog(btn InputRemapButton, new_event InputEvent, conflicts Array~String~) void
+_on_conflict_confirmed() void
+_on_conflict_canceled() void
+_clear_conflict_state() void
+update_all_remap_buttons() void
+_on_reset_pressed() void
+_on_controls_back_button_pressed() void
+_on_controls_back_button_pressed_js(args Array) void
+_grab_initial_focus() void
+_on_keyboard_toggled(toggled_on bool) void
+_on_gamepad_toggled(toggled_on bool) void
}
class InputRemapButton {
<<Button>>
+enum DeviceType
+action String
+current_device DeviceType
+listening bool
+AXIS_DEADZONE_THRESHOLD float
+_ready() void
+_on_pressed() void
+_input(event InputEvent) void
+erase_old_event() void
+finish_remap() void
+update_button_text() void
+get_matching_event() InputEvent
+get_normalized_axis_direction(value float) float
}
class MainScene {
<<Node2D>>
+enum MessageType
-_showing_unbound_warning bool
-_showing_unbound_key_message bool
+player Node2D
+stats_panel Panel
+background ParallaxBackground
+bushes_layer ParallaxLayer
+decor_layer ParallaxLayer
+texture_preloader ResourcePreloader
+hud CanvasLayer
+message_label Label
+_ready() void
+_input(event InputEvent) void
+_process(delta float) void
+show_message(text String, type MessageType) void
+clear_unbound_warning() void
+setup_bushes_layer(viewport Vector2) void
+setup_decor_layer(viewport Vector2) void
}
class MainMenu {
<<CanvasLayer>>
+quit_dialog ConfirmationDialog
+unbound_dialog ConfirmationDialog
+options_menu PackedScene
+last_focused_button Button
+_ready() void
+_input(event InputEvent) void
+_setup_quit_dialog() void
+_setup_unbound_dialog() void
+_on_start_pressed(args Array) void
+_on_options_button_pressed(args Array) void
+_on_quit_pressed() void
}
class LoadingScreen {
<<Control>>
+progress_bar ProgressBar
+status_label Label
+load_failed bool
+is_scene_loaded bool
+loader_progress float
+min_load_time float
+load_start_time float
+transitioning bool
+_ready() void
+_process(delta float) void
}
class OptionsMenu {
<<CanvasLayer>>
+advanced_settings_button Button
+audio_settings_button Button
+key_mapping_button Button
+gameplay_settings_button Button
+options_back_button Button
+_ready() void
+grab_focus_on_key_mapping_button() void
+_grab_first_button_focus() void
}
Settings ..> ConfigFile
Settings ..> InputEvent
Settings ..> InputEventKey
Settings ..> InputEventJoypadButton
Settings ..> InputEventJoypadMotion
Settings ..> Globals
Globals ..> OptionsMenu
Globals ..> KeyMappingMenu
KeyMappingMenu o--> ConfirmationDialog
KeyMappingMenu o--> InputRemapButton
KeyMappingMenu ..> Globals
KeyMappingMenu ..> Settings
KeyMappingMenu ..> MainScene
InputRemapButton ..> Settings
InputRemapButton ..> Globals
InputRemapButton ..> KeyMappingMenu
MainScene ..> Settings
MainScene ..> Globals
MainMenu ..> Settings
MainMenu ..> Globals
LoadingScreen ..> Globals
OptionsMenu ..> Globals
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This was
linked to
issues
Feb 16, 2026
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDevice-aware key remapping, conflict-resolution UI, and extensive focus/navigation wiring for the Key Mapping menu were added; Settings gained device helpers, migration/backfill and critical-action checks; HUD transient messaging and threaded loading improvements were introduced; docs and multiple tests were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Player
participant IRB as InputRemapButton
participant KMM as KeyMappingMenu
participant Settings as Settings
participant Dialog as ConflictDialog
Player->>IRB: press to start remap
IRB->>IRB: show device-specific prompt
Player-->>IRB: input event (key / joy / axis)
IRB->>Settings: get_conflicting_actions(new_event)
alt conflicts found
IRB->>KMM: request show_conflict_dialog(btn,new_event,conflicts)
KMM->>Dialog: display conflicts and options
alt user confirms
KMM->>Settings: unbind_conflicting_actions()
KMM->>Settings: add_new_binding(new_event)
KMM->>IRB: finish_remap(success)
else user cancels
KMM->>IRB: cancel_remap()
end
else no conflicts
IRB->>Settings: erase_old_event(), add_new_binding(new_event)
IRB->>IRB: finish_remap(success)
end
Note right of Settings: may persist InputMap and save last-input-device
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_ensure_defaults_saved, the ternary-style cast assignment todeviceis a bit hard to read; consider splitting theInputEventJoypadButtonvsInputEventJoypadMotionbranches and settingdeviceexplicitly in each for clarity and easier debugging. - When reading
def['type']fromDEFAULT_GAMEPAD[action], there’s no fallback if an unexpected type is configured; adding a finalelsebranch that logs a warning could make misconfigured defaults easier to detect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_ensure_defaults_saved`, the ternary-style cast assignment to `device` is a bit hard to read; consider splitting the `InputEventJoypadButton` vs `InputEventJoypadMotion` branches and setting `device` explicitly in each for clarity and easier debugging.
- When reading `def['type']` from `DEFAULT_GAMEPAD[action]`, there’s no fallback if an unexpected type is configured; adding a final `else` branch that logs a warning could make misconfigured defaults easier to detect.
## Individual Comments
### Comment 1
<location> `scripts/settings.gd:370` </location>
<code_context>
+ (nev as InputEventJoypadMotion).axis = def["axis"]
+ (nev as InputEventJoypadMotion).axis_value = def["value"]
+
+ (nev as InputEventJoypadButton if nev is InputEventJoypadButton else nev as InputEventJoypadMotion).device = -1
+ InputMap.action_add_event(action, nev)
+ changed = true
</code_context>
<issue_to_address>
**suggestion:** Simplify the device assignment to avoid complex casting expression
The ternary cast here is hard to read and will fail if `nev` ever becomes another `InputEvent` type. Since `nev` is only `InputEventJoypadButton` or `InputEventJoypadMotion` here, set `device` directly in each branch where `nev` is created, or use a straightforward `if nev is InputEventJoypadButton` / `elif nev is InputEventJoypadMotion` before assigning `device` to keep the types explicit and avoid complex inline casting.
Suggested implementation:
```
if nev is InputEventJoypadButton:
(nev as InputEventJoypadButton).device = -1
elif nev is InputEventJoypadMotion:
(nev as InputEventJoypadMotion).device = -1
InputMap.action_add_event(action, nev)
changed = true
```
If there are other locations in this file (e.g. the keyboard defaults section or other mapping blocks) that use a similar inline cast pattern for `device` assignment, consider applying the same refactor there for consistency and readability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scenes/key_mapping_menu.tscn`:
- Around line 248-257: The LeftInputRemap button has focus_neighbor_left
incorrectly pointing to MenuUpInputRemap (which is in the right column); remove
or clear the focus_neighbor_left property on the LeftInputRemap node so left
D-pad presses don't jump right, and if you want wrapping instead add/set the
reciprocal focus_neighbor_right on the MenuUpInputRemap node to point to
LeftInputRemap (use the node names LeftInputRemap and MenuUpInputRemap to find
and update the properties).
- Around line 496-515: The ControlResetButton's focus_next and focus_previous
both point to ControlsBackButton, causing Tab/Shift-Tab to loop only between
these two buttons; update the ControlResetButton node (properties focus_next and
focus_previous) to point to the appropriate neighboring controls in the overall
tab order (e.g., the next/previous controls in KeyMapContainer or MenuKeyMap
such as MenuRightInputRemap or whatever logical sibling should follow/precede),
or remove these explicit focus_* assignments to allow the engine's default tab
order to handle traversal; also verify ControlsBackButton.focus_next still
points to ControlResetButton only if that bidirectional link is intended.
In `@scripts/settings.gd`:
- Around line 358-375: The block creates nev based on DEFAULT_GAMEPAD[action]
but leaves nev unassigned if def["type"] is unexpected and uses a long
concatenated log line; update the code in scripts/settings.gd around the nev
handling to validate def["type"] and bail out early (e.g., continue) when it's
not "button" or "axis", ensuring nev is always initialized before setting device
and calling InputMap.action_add_event(action, nev); also shorten the
Globals.log_message call (split the message or use a shorter string) to fix the
max-line-length lint error; reference symbols: DEFAULT_GAMEPAD, def, nev,
InputEventJoypadButton, InputEventJoypadMotion, InputMap.action_add_event,
Globals.log_message, and action.
🧹 Nitpick comments (1)
scripts/settings.gd (1)
332-379: Duplicated default-restoration logic.The body of
_ensure_defaults_saved()(Lines 335–375) is nearly identical to the default-adding block inload_input_mappings()(Lines 143–172). Both iterateACTIONS, check for missing keyboard/gamepad events, and add defaults from the same dictionaries. Consider extracting a shared helper (e.g.,_add_missing_defaults() -> bool) and calling it from both sites to keep the logic in one place.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scenes/key_mapping_menu.tscnscripts/settings.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/settings.gd
[error] 370-370: gdlint: Max allowed line length (100) exceeded (max-line-length)
🔇 Additional comments (2)
scenes/key_mapping_menu.tscn (1)
14-27: LGTM on the new StyleBoxFlat focus styles.The added focus style resources (
StyleBoxFlat_4k3vu,StyleBoxFlat_mxrmg,StyleBoxFlat_rhvir,StyleBoxFlat_hh2p6) are consistent with the existing styling conventions and provide clear visual feedback for keyboard/D-pad navigation.Also applies to: 49-55, 79-85
scripts/settings.gd (1)
62-67: This concern is incorrect—no duplication occurs due to protective conditional logic.Both
load_input_mappings()(lines 143–172) and_ensure_defaults_saved()(lines 348, 358) check for the presence of existing keyboard and gamepad events before adding defaults. The conditionsif not has_key_eventandif not has_gamepadensure that defaults added byload_input_mappings()are not re-added by_ensure_defaults_saved().The second function is redundant but not broken—it serves as a safety net for edge cases where defaults may have been removed between calls, but under normal operation, no duplicates are created.
…xpression The ternary cast here is hard to read and will fail if nev ever becomes another InputEvent type. Since nev is only InputEventJoypadButton or InputEventJoypadMotion here, set device directly in each branch where nev is created, or use a straightforward if nev is InputEventJoypadButton / elif nev is InputEventJoypadMotion before assigning device to keep the types explicit and avoid complex inline casting.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/settings.gd (1)
332-383:_ensure_defaults_saved()duplicates the default-backfill logic already inload_input_mappings()(lines 143–172).Both functions iterate
ACTIONS, check for missing keyboard/gamepad events, and add the same defaults fromDEFAULT_KEYBOARD/DEFAULT_GAMEPAD. Sinceload_input_mappings()runs first and already backfills,_ensure_defaults_saved()will typically find nothing to do.Consider having
load_input_mappings()return aboolindicating whether defaults were added (or set an instance flag), and then save in_ready()based on that — removing this function entirely.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/settings.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/settings.gd
[error] 371-379: gdformat formatting check failed: 1 file would be reformatted. Run 'gdformat -i' to apply formatting changes.
🔇 Additional comments (1)
scripts/settings.gd (1)
62-67: LGTM on placement of the_ensure_defaults_saved()call.Correct ordering: load mappings, backfill missing defaults (with its own save), then handle migration save separately.
Add a comprehensive web deployment guide for SkyLockAssault (Godot v4.5, Win10 x64). The new doc includes a platform comparison, reasons many portals lack publish APIs, a recommended phased automation workflow (automatic via itch.io/poki/viverse, semi-auto with Playwright for some portals, and manual targets), Godot Web export tips, and quickstart snippets for GitHub Actions and CLI-based uploads.
…atially inverted. Line 250 sets focus_neighbor_left to MenuUpInputRemap, which is in the right (Menu) column. Pressing D-pad left from an item already in the left column would unexpectedly jump right. This should likely be unset (to block left navigation) or wrap to MenuUpInputRemap via focus_neighbor_right on that row instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@files/docs/Platforms_for_Web_Deployment_Guide.md`:
- Around line 3-46: Shorten or wrap long lines and fix markdown style: break the
overlong lines containing "Summary of zero-cost publishing platforms for Godot
v4.5 HTML5/WebGL games (Feb 2026)", the paragraph starting "We're building
**SkyLockAssault** — a totally free-to-play browser game in **Godot v4.5**" and
the "This `.md` file is your living playbook..." line so they are <=80 chars (or
wrap them with hard line breaks), replace the bold "Project Context" text with a
proper heading (change "**Project Context**" to "### Project Context"), and
reformat the table so the pipe alignment matches the header row (fix rows under
the header to use the same number of columns and aligned pipes) to resolve
MD013, MD036, and MD060 respectively.
🧹 Nitpick comments (2)
README.md (1)
80-80: Minor path inconsistency with other documentation links.Most documentation entries (lines 63, 66–69) use a leading
/(e.g.,/files/docs/...), but this entry and line 65 omit it. Both forms work on GitHub, but consider making them consistent.scripts/settings.gd (1)
332-383:_ensure_defaults_saved()largely duplicates the default-backfill logic already inload_input_mappings()(lines 143–172).Both blocks iterate
ACTIONS, check for missing keyboard/gamepad events, and add defaults from the same dictionaries using identical logic. This duplication means bug fixes (like thenevnull guard) must be applied in two places.Consider extracting a shared helper (e.g.,
_add_missing_defaults() -> bool) called from both sites, returning whether changes were made.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.mdfiles/docs/Platforms_for_Web_Deployment_Guide.mdscenes/key_mapping_menu.tscnscripts/settings.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
files/docs/Platforms_for_Web_Deployment_Guide.md
[error] 3-3: MD013/line-length Line length [Expected: 80; Actual: 88]
🪛 GitHub Check: Markdown Lint / lint (3.x)
files/docs/Platforms_for_Web_Deployment_Guide.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:162 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:149 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:108 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:82 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:60 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 34-34: Table column style
files/docs/Platforms_for_Web_Deployment_Guide.md:34:39 MD060/table-column-style Table column style [Table pipe does not align with header for style "aligned"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md060.md
[failure] 12-12: Line length
files/docs/Platforms_for_Web_Deployment_Guide.md:12:81 MD013/line-length Line length [Expected: 80; Actual: 88] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md
[failure] 7-7: Line length
files/docs/Platforms_for_Web_Deployment_Guide.md:7:81 MD013/line-length Line length [Expected: 80; Actual: 89] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md
[failure] 5-5: Emphasis used instead of a heading
files/docs/Platforms_for_Web_Deployment_Guide.md:5 MD036/no-emphasis-as-heading Emphasis used instead of a heading [Context: "Project Context"] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md036.md
[failure] 3-3: Line length
files/docs/Platforms_for_Web_Deployment_Guide.md:3:81 MD013/line-length Line length [Expected: 80; Actual: 88] https://github.com/DavidAnson/markdownlint/blob/v0.40.0/doc/md013.md
🪛 LanguageTool
files/docs/Platforms_for_Web_Deployment_Guide.md
[style] ~38-~38: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... | Yes | ❌ No | ✅ Very easy | Instant publish | Simp...
(EN_WEAK_ADJECTIVE)
🔇 Additional comments (2)
scenes/key_mapping_menu.tscn (2)
248-257: Previousfocus_neighbor_leftissue onLeftInputRemapappears resolved.The spatially-inverted
focus_neighbor_leftthat previously pointed to the right column has been removed. Navigation fromLeftInputRemapnow correctly wires top/bottom/right neighbors only.
142-172: Overall navigation wiring looks well thought out.The two-column layout with cross-column
focus_neighbor_right/focus_neighbor_leftlinks, proper top/bottom chaining, and bridging viafocus_nextfromNextWeaponInputRemap→PauseInputRemapprovides a logical traversal order. The device toggle buttons (Keyboard/Gamepad) are correctly wired as entry points from the top.
…ailure on Line 370. If def["type"] is neither "button" nor "axis", nev remains uninitialized (declared but never assigned on Line 360). Lines 370–371 would then crash with a null access. Additionally, Line 370 exceeds the 100-character lint limit flagged by the pipeline.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- There’s quite a bit of duplication between
get_pause_binding_labelandget_pause_binding_label_for_deviceinsettings.gd(including the temporaryInputRemapButtonconstruction); consider centralizing the label resolution into a single helper that takes an optional preferred device to reduce maintenance surface. - Both
Globals._ready()andSettings._ready()callSettings.load_last_input_device()(directly or indirectly), which means the config is loaded twice on startup; you could either let only one of these be the source of truth or guard the second call to avoid redundant I/O. - Several non-UI scripts (
settings.gd) constructInputRemapButtonnodes purely for label generation; extracting the label logic into a non-Control helper (e.g. a static utility or a method onSettings/Globals) would avoid creating UI nodes in core logic and simplify future refactors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s quite a bit of duplication between `get_pause_binding_label` and `get_pause_binding_label_for_device` in `settings.gd` (including the temporary `InputRemapButton` construction); consider centralizing the label resolution into a single helper that takes an optional preferred device to reduce maintenance surface.
- Both `Globals._ready()` and `Settings._ready()` call `Settings.load_last_input_device()` (directly or indirectly), which means the config is loaded twice on startup; you could either let only one of these be the source of truth or guard the second call to avoid redundant I/O.
- Several non-UI scripts (`settings.gd`) construct `InputRemapButton` nodes purely for label generation; extracting the label logic into a non-Control helper (e.g. a static utility or a method on `Settings`/`Globals`) would avoid creating UI nodes in core logic and simplify future refactors.
## Individual Comments
### Comment 1
<location> `scripts/settings.gd:297-299` </location>
<code_context>
+ # For corrupt/no-file: config empty → add defaults.
+ # Sets _needs_migration=true if added → triggers save in _ready() (first-run/repair).
+ # Idempotent with _ensure_defaults_saved().
+ var defaults_config: ConfigFile = ConfigFile.new()
+ defaults_config.load(path)
+ if _add_missing_defaults(defaults_config): # Still pass it (harmless)
+ _needs_save = true
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid re-loading the config file and reuse the existing ConfigFile instance for defaults backfill.
In `load_input_mappings`, you load `config` from `path` and then create a new `defaults_config` and load the same file again just to call `_add_missing_defaults`. This duplicates I/O and re-triggers parse errors for corrupt files. Instead, pass the already-loaded `config` into `_add_missing_defaults` (or clone it if you truly need a separate instance) to avoid the redundant `load()` call.
Suggested implementation:
```
# For corrupt/no-file: config empty → add defaults.
# Sets _needs_migration=true if added → triggers save in _ready() (first-run/repair).
# Idempotent with _ensure_defaults_saved().
+ if _add_missing_defaults(config):
+ _needs_save = true
```
This change assumes that within the same scope there is an existing, already-loaded `ConfigFile` instance named `config` (e.g. created and loaded earlier in `load_input_mappings` with `config.load(path)`). If the variable has a different name, update the `_add_missing_defaults(config)` call accordingly.
</issue_to_address>
### Comment 2
<location> `scripts/settings.gd:557-561` </location>
<code_context>
+
+## Returns true if there is at least one unbound critical action for the currently selected device.
+## This respects Globals.current_input_device.
+func has_unbound_critical_actions_for_current_device() -> bool:
+ var preferred: String = Globals.current_input_device
+ for action: String in CRITICAL_ACTIONS:
+ var events: Array[InputEvent] = InputMap.action_get_events(action)
+ if events.is_empty():
+ # continue # action is unbound, but we check per device
+ return true # unbound for all devices, including current
</code_context>
<issue_to_address>
**nitpick:** Clean up the commented-out `continue` to better reflect the intended control flow.
The early-return branch still contains a commented-out `continue` right before `return true`, which now conflicts with the actual behavior and can mislead future edits. Please either remove the stale comment or reword it to clearly justify the early return when `events.is_empty()`.
</issue_to_address>
### Comment 3
<location> `scripts/key_mapping.gd:87-91` </location>
<code_context>
)
js_window.controlsBackPressed = _controls_back_button_pressed_cb
+ # Load and apply the last selected device (so it remembers across sessions)
+ Settings.load_last_input_device()
+ keyboard.button_pressed = (Globals.current_input_device == "keyboard")
+ gamepad.button_pressed = (Globals.current_input_device == "gamepad")
+ update_all_remap_buttons()
+
+
</code_context>
<issue_to_address>
**suggestion (performance):** Consider avoiding duplicate calls to Settings.load_last_input_device to reduce redundant config reads.
`Globals._ready()` already calls `Settings.load_last_input_device()`, and `key_mapping.gd` calls it again here, so the config is read twice on startup. Since `Globals.current_input_device` is already populated before this menu opens, you can just use it to set `keyboard.button_pressed` / `gamepad.button_pressed` and update the remap buttons, without reloading from disk. If you do need to query Settings here, consider adding a lightweight accessor that doesn’t hit the filesystem again.
```suggestion
# Apply the last selected device (Globals has already loaded it in _ready)
keyboard.button_pressed = (Globals.current_input_device == "keyboard")
gamepad.button_pressed = (Globals.current_input_device == "gamepad")
update_all_remap_buttons()
```
</issue_to_address>
### Comment 4
<location> `scripts/main_menu.gd:170` </location>
<code_context>
- Globals.load_scene_with_loading("res://scenes/main_scene.tscn")
+ if Settings.has_unbound_critical_actions_for_current_device(): # ← FIXED
+ # Show warning dialog (create once or use existing ConfirmationDialog)
+ var dialog: ConfirmationDialog = ConfirmationDialog.new()
+ dialog.title = "Unbound Controls"
+ dialog.dialog_text = "Some critical controls are unbound.\nGo to Key Mapping to fix?"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Re-use a single confirmation dialog instead of creating a new one on every Start press.
In `_on_start_pressed` a new `ConfirmationDialog` is created and added every time Start is pressed while critical actions are unbound. Repeated presses will stack multiple dialogs and signal connections in the scene tree. Instead, create a single dialog (e.g. as an `onready` node or lazily with a stored reference) and just configure it and call `popup_centered()` when needed.
Suggested implementation:
```
## Handles the Start button press.
##
## :param _args: Optional arguments from web overlays (unused).
## :type _args: Array
## :rtype: void
Globals.log_message("Start Game menu button pressed.", Globals.LogLevel.DEBUG)
if Settings.has_unbound_critical_actions_for_current_device(): # ← FIXED
# Show (or create) a single reusable warning dialog about unbound controls.
if unbound_controls_dialog == null:
unbound_controls_dialog = ConfirmationDialog.new()
unbound_controls_dialog.title = "Unbound Controls"
unbound_controls_dialog.dialog_text = "Some critical controls are unbound.\nGo to Key Mapping to fix?"
unbound_controls_dialog.get_ok_button().text = "Open Key Mapping"
unbound_controls_dialog.get_cancel_button().text = "Start Anyway"
add_child(unbound_controls_dialog)
# Continue to Key Mapping menu
unbound_controls_dialog.confirmed.connect(_on_unbound_controls_dialog_confirmed)
# Ensure dialog text/buttons are correct before showing (in case they change in future).
unbound_controls_dialog.dialog_text = "Some critical controls are unbound.\nGo to Key Mapping to fix?"
unbound_controls_dialog.get_ok_button().text = "Open Key Mapping"
unbound_controls_dialog.get_cancel_button().text = "Start Anyway"
unbound_controls_dialog.popup_centered()
return
# Open the gameplay anyway
```
To fully implement this:
1. Add a member variable to `scripts/main_menu.gd` (near the top of the script):
```gdscript
var unbound_controls_dialog: ConfirmationDialog = null
```
2. Implement the handler used in the `confirmed` connection:
```gdscript
func _on_unbound_controls_dialog_confirmed() -> void:
Globals.load_key_mapping(ui_panel) # same as options menu
```
3. Ensure that after the `if Settings.has_unbound_critical_actions_for_current_device():` block, the existing logic that starts the game (previously after the `# Open the gameplay anyway` comment) still runs when controls are bound (and that the `return` added in the replacement keeps the "start game" path from executing when the dialog is shown).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Avoid loading a separate defaults ConfigFile when ensuring input defaults — call _add_missing_defaults(config) directly and mark _needs_save if defaults were added. Also remove the call to update_all_remap_buttons() from key_mapping.gd's _ready(), relying on the default keyboard selection/focus instead.
… creating a new one on every Start press. In _on_start_pressed a new ConfirmationDialog is created and added every time Start is pressed while critical actions are unbound. Repeated presses will stack multiple dialogs and signal connections in the scene tree. Instead, create a single dialog (e.g. as an onready node or lazily with a stored reference) and just configure it and call popup_centered() when needed.
Owner
Author
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
Make the unbound dialog modal (exclusive) and refactor its signal handling to avoid race conditions with the Start button and scene loading. Confirmed now loads key mapping and hides the dialog; cancel uses the cancel button's pressed signal to hide then load the main scene; close_requested simply hides and logs a debug message. Removed per-handler re-enabling of the Start button and added a visibility_changed handler that reliably re-enables and call_deferred("grab_focus") on the Start button after the dialog is fully hidden.
There’s quite a bit of duplication between get_pause_binding_label and get_pause_binding_label_for_device in settings.gd (including the temporary InputRemapButton construction); consider centralizing the label resolution into a single helper that takes an optional preferred device to reduce maintenance surface.
Both Globals._ready() and Settings._ready() call Settings.load_last_input_device() (directly or indirectly), which means the config is loaded twice on startup; you could either let only one of these be the source of truth or guard the second call to avoid redundant I/O.
Owner
Author
|
Addressing following:
|
Owner
Author
|
Working on:
|
Several non-UI scripts (settings.gd) construct InputRemapButton nodes purely for label generation; extracting the label logic into a non-Control helper (e.g. a static utility or a method on Settings/Globals) would avoid creating UI nodes in core logic and simplify future refactors. To address the feedback, add this static method to settings.gd (or a new autoload like InputUtils.gd for broader reuse). It replicates InputRemapButton.get_event_label() logic without instantiating a UI node Important for decoupling UI from core logic, improving maintainability. Address in current PR #409, as it already refactors settings.gd for input mappings and unbound states—fits scope without new issue.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
get_pause_binding_label_for_deviceAPI and its tests use mixed device identifiers ("keyboard"/"gamepad"in the implementation vs."joypad"in tests), which will only hit the fallback path for gamepad in some cases—consider normalizing accepted values (e.g., constants or an enum) and updating callers/tests consistently to avoid subtle mislabeling. - Several tests around settings/device persistence (e.g., EC-09 and the new pause-label tests) manipulate
Settings.CONFIG_PATHfiles directly with ad‑hoc backup/restore logic; it would be more robust to add an injectable config path or helper inSettingsso tests don't touch the real user config and can avoid duplicating backup code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `get_pause_binding_label_for_device` API and its tests use mixed device identifiers (`"keyboard"`/`"gamepad"` in the implementation vs. `"joypad"` in tests), which will only hit the fallback path for gamepad in some cases—consider normalizing accepted values (e.g., constants or an enum) and updating callers/tests consistently to avoid subtle mislabeling.
- Several tests around settings/device persistence (e.g., EC-09 and the new pause-label tests) manipulate `Settings.CONFIG_PATH` files directly with ad‑hoc backup/restore logic; it would be more robust to add an injectable config path or helper in `Settings` so tests don't touch the real user config and can avoid duplicating backup code.
## Individual Comments
### Comment 1
<location> `scripts/main_scene.gd:45-54` </location>
<code_context>
# :type event: InputEvent
# :rtype: void
-# In input_remap_button.gd (full updated _input function)
func _input(event: InputEvent) -> void:
if not listening:
return
</code_context>
<issue_to_address>
**issue (bug_risk):** Unbound-key message may fire for UI-only bindings like pause or cancel, which might already be mapped.
Settings.is_event_bound() only checks ACTIONS and ignores UI actions like ui_cancel/ui_accept. In _input(), a key mapped only to a UI action (e.g. Esc → ui_cancel) is therefore treated as unbound and triggers KEY_PRESS_UNBOUND even though it’s a valid binding. To avoid this, either iterate over InputMap.get_actions() in is_event_bound(), or add a helper that checks “bound to any action” and use that for this case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The get_pause_binding_label_for_device API and its tests use mixed device identifiers ("keyboard"/"gamepad" in the implementation vs. "joypad" in tests), which will only hit the fallback path for gamepad in some cases—consider normalizing accepted values (e.g., constants or an enum) and updating callers/tests consistently to avoid subtle mislabeling.
Owner
Author
|
Addressing following:
|
The suggestion fixes a bug where is_event_bound() in settings.gd only checks the custom ACTIONS array (e.g., "fire", "pause", "ui_accept"), ignoring Godot's built-in UI actions like "ui_cancel" or "ui_focus_next". If a key (e.g., Esc) is bound only to "ui_cancel" (not in ACTIONS), the function returns false, wrongly triggering the unbound-key message in main_scene.gd's _input(). To fix: Change the loop in is_event_bound() to use InputMap.get_actions() (all actions) instead of ACTIONS. This ensures keys bound to any action (custom or UI) are detected as bound.
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
enable-keyboard-&-dpad-navigation-for-key-mapping-settings
branch
February 18, 2026 07:56
8 tasks
This was
linked to
issues
Feb 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Enhancements:
Summary by Sourcery
Improve input mapping robustness, conflict handling, and device-aware UI/UX across settings, key mapping, main menu, and gameplay while expanding automated tests and documentation.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by Sourcery
Improve input handling, device awareness, and user feedback for key mappings and gameplay while hardening settings migration and loading behavior.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation
Tests