Refactor key-mapping system to separate keyboard and gamepad remapping - #345
Conversation
…d gamepad remapping. #313 Refactor key_mapping_menu.tscn in SkyLockAssault using Godot 4.5 to separate keyboard, mouse, and gamepad remapping into dedicated sub-menus or tabs, accessed from the main key mapping menu. Currently all input remapping options are shown together in key_mapping_menu.tscn, which mixes keyboard, mouse, and gamepad controls. This makes it harder for players to locate and configure controls for their device. We should refactor this UI to separate device types into dedicated tabs, improving organization and UX, especially as SkyLockAssault grows support for multiple control schemes.
ikostan
added this to the
🚀 Milestone 10: UI & Input Improvements, System Refactors, Testing & Documentation milestone
Reviewer's GuideRefactors the input mapping system and controls UI to be device-aware, introducing separate keyboard/gamepad handling, per‑device defaults and reset, device-aware remap buttons, updated pause handling, and expanded/unit-tested settings and audio behaviors, plus a headless GUT test runner. Sequence diagram for device-specific controls reset flowsequenceDiagram
actor Player
participant KeyMappingMenu
participant Settings
participant InputMap
participant ConfigFile
Player->>KeyMappingMenu: Press ControlResetButton
KeyMappingMenu->>KeyMappingMenu: Determine device_type (keyboard or gamepad)
KeyMappingMenu->>Settings: reset_to_defaults(device_type)
loop For each action in ACTIONS
Settings->>InputMap: action_get_events(action)
Settings->>Settings: Remove events matching device_type
alt device_type is keyboard and DEFAULT_KEYBOARD has action
Settings->>InputMap: action_add_event(action, InputEventKey)
else device_type is gamepad and DEFAULT_GAMEPAD has action
alt type is button
Settings->>InputMap: action_add_event(action, InputEventJoypadButton)
else type is axis
Settings->>InputMap: action_add_event(action, InputEventJoypadMotion)
end
end
end
Settings->>ConfigFile: save_input_mappings(CONFIG_PATH, ACTIONS)
ConfigFile-->>Settings: Save result
Settings-->>KeyMappingMenu: Defaults reset
KeyMappingMenu->>KeyMappingMenu: update_all_remap_buttons()
KeyMappingMenu->>InputRemapButtons: Set current_device and update_button_text()
KeyMappingMenu-->>Player: UI shows updated per-device bindings
Class diagram for device-aware input mapping and controls UIclassDiagram
class Settings {
+String CONFIG_PATH
+Array~String~ ACTIONS
+Dictionary DEFAULT_KEYBOARD
+Dictionary DEFAULT_GAMEPAD
-bool _needs_migration
+_ready() void
+load_input_mappings(path String, actions Array~String~) void
+serialize_event(ev InputEvent) String
+_deserialize_and_add(action String, serialized String) void
+save_input_mappings(path String, actions Array~String~) void
+reset_to_defaults(device_type String) void
}
class DeviceType {
<<enumeration>>
KEYBOARD
GAMEPAD
}
class InputRemapButton {
<<Button>>
+DeviceType current_device
+String action
-bool listening
+_ready() void
+_on_pressed() void
+_input(event InputEvent) void
+get_normalized_axis_direction(axis_value float) float
+erase_old_event() void
+get_matching_event() InputEvent
+finish_remap() void
+update_button_text() void
+get_event_label(event InputEvent) String
}
class KeyMapping {
+JavaScriptBridgeWrapper js_bridge_wrapper
-Variant _controls_back_button_pressed_cb
-bool _intentional_exit
+Button controls_back_button
+Button controls_reset_button
+CheckButton keyboard
+CheckButton gamepad
+ButtonGroup device_group
+_ready() void
+update_all_remap_buttons() void
+_on_reset_pressed() void
+_on_keyboard_toggled(toggled_on bool) void
+_on_gamepad_toggled(toggled_on bool) void
+_on_controls_back_button_pressed() void
}
class PauseMenu {
+_unhandled_input(event InputEvent) void
}
class MainMenu {
+_ready() void
+_input(_event InputEvent) void
}
class InputMap {
+action_get_events(action String) Array~InputEvent~
+action_add_event(action String, event InputEvent) void
+action_erase_event(action String, event InputEvent) void
+action_erase_events(action String) void
}
class ConfigFile {
+load(path String) int
+save(path String) int
+get_value(section String, key String) Variant
+set_value(section String, key String, value Variant) void
+has_section_key(section String, key String) bool
}
Settings ..> InputMap : uses
Settings ..> ConfigFile : uses
InputRemapButton ..> InputMap : uses
InputRemapButton ..> Settings : saves via
InputRemapButton ..> DeviceType
KeyMapping ..> InputRemapButton : updates group remap_buttons
KeyMapping ..> Settings : reset_to_defaults
PauseMenu ..> Settings : uses pause action from updated mappings
MainMenu ..> Settings : relies on ui navigation actions
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds device-aware key mapping: UI gains Keyboard/Gamepad selectors; remap buttons and controller scripts handle per-device events; settings now store keyboard and gamepad defaults with migration/load/save; tests updated, test-run script added; pause action renamed to "pause". Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as KeyMapping UI
participant KM as key_mapping.gd
participant RB as input_remap_button.gd
participant Settings as settings.gd
participant InputMap as Godot InputMap
User->>UI: Toggle device (Keyboard/Gamepad)
UI->>KM: emit toggled
KM->>KM: set active device, update ButtonGroup
KM->>RB: call update_all_remap_buttons()
RB->>RB: set current_device, update_button_text()
User->>RB: press new key/button
RB->>RB: detect InputEvent, filter by current_device
RB->>InputMap: remove matching old event
RB->>InputMap: add new event
RB->>Settings: save_input_mappings()
Settings->>InputMap: apply defaults/migrations per device
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
_on_keyboard_toggled/_on_gamepad_toggledhandlers setbutton_pressedon each other and may cause recursive toggling; consider using aButtonGroupfor mutual exclusivity or adding a guard flag to avoid feedback loops. - The new
DeviceTypeenum ininput_remap_button.gdis defined but not yet used; either wire it into the remap logic/inspector configuration or remove it until it is needed to keep the script focused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_on_keyboard_toggled` / `_on_gamepad_toggled` handlers set `button_pressed` on each other and may cause recursive toggling; consider using a `ButtonGroup` for mutual exclusivity or adding a guard flag to avoid feedback loops.
- The new `DeviceType` enum in `input_remap_button.gd` is defined but not yet used; either wire it into the remap logic/inspector configuration or remove it until it is needed to keep the script focused.
## Individual Comments
### Comment 1
<location> `scripts/key_mapping.gd:62-63` </location>
<code_context>
+# NEW: Reset button handler—resets InputMap to defaults and updates buttons
+func _on_reset_pressed() -> void:
+ Globals.log_message("Reset controls pressed.", Globals.LogLevel.DEBUG)
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Reset handler is effectively a no-op despite the comment promising a full reset.
This handler currently only logs, so pressing Reset won’t actually revert bindings as implied. If this is hooked to a UI control, users will expect a real reset. Either implement restoring from DEFAULT_KEYS and refreshing all InputRemapButtons here, or update the comment/UI to reflect that reset isn’t implemented yet.
</issue_to_address>
### Comment 2
<location> `scripts/key_mapping.gd:132-137` </location>
<code_context>
_on_controls_back_button_pressed()
+
+
+func _on_keyboard_toggled(toggled_on: bool) -> void:
+ gamepad.button_pressed = not toggled_on
+
+
+func _on_gamepad_toggled(toggled_on: bool) -> void:
+ keyboard.button_pressed = not toggled_on
</code_context>
<issue_to_address>
**issue (bug_risk):** Mutually toggling the CheckButtons risks recursive signal loops.
Because `button_pressed` can emit `toggled` even when changed in code, these handlers may trigger each other repeatedly, causing infinite loops or duplicate signals. Consider only updating the other button when its state actually differs, and use a simple guard flag (e.g. `_updating_device_toggle`) to block re-entrant updates while syncing them.
</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}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@scenes/key_mapping_menu.tscn`:
- Around line 314-318: The button action names in the Key Mapping scene (e.g.,
AcceptInputRemap and the other remap buttons at the referenced ranges) are
incorrect/typoed (e.g., "accept", "meu_up", "menu_*") and thus won’t match
Settings.DEFAULT_KEYS; change each button's action property to the corresponding
ui_* action names used in Settings.DEFAULT_KEYS (for example use "ui_accept",
"ui_menu_up", "ui_menu_down", "ui_menu_left", "ui_menu_right", "ui_cancel" as
appropriate) or alternatively update Settings/InputMap to define the current
action names—ensure the typo "meu_up" is corrected to the intended action and
all menu remap nodes reference the exact action identifiers used elsewhere.
- Around line 286-306: Several menu-related tooltip_text values are incorrect
(they say "next weapon")—update the tooltip_text on the menu key mapping nodes
to reflect their actual actions: change KeyMappingPause.tooltip_text to
something like "Key mapping for pause/menu" and update
KeyMappingAccept.tooltip_text to "Key mapping for accept/confirm" (also inspect
other similar nodes referenced in the comment ranges 320-340, 337-357, 354-374
and replace any "next weapon" copy/paste text with the appropriate descriptive
tooltip). Locate the nodes by their names (KeyMappingPause, PauseInputRemap,
KeyMappingAccept and the other menu mapping nodes) and edit their tooltip_text
strings accordingly.
In `@scripts/input_remap_button.gd`:
- Around line 40-52: The new MOUSE_BUTTON_LABELS dictionary isn't used anywhere:
update the _input(event: InputEvent) handler to detect InputEventMouseButton
(use event is InputEventMouseButton and event.pressed) and treat it like
key/button binding capture by using event.button_index to create a
ButtonList/MOUSE_BUTTON constant index for the binding capture flow (same branch
that handles InputEventKey), and update get_event_label(event: InputEvent) to
return MOUSE_BUTTON_LABELS[event.button_index] (or a fallback string) when event
is an InputEventMouseButton so mouse buttons display proper labels; reference
the MOUSE_BUTTON_LABELS constant, the _input() function, and get_event_label()
when making these changes.
In `@scripts/settings.gd`:
- Around line 10-12: The ACTIONS constant is missing the UI/navigation action
names defined in DEFAULT_KEYS, so load/save ignores those keys; update ACTIONS
to include the ui_* entries used by the menu remap (e.g., add ui_up, ui_down,
ui_left, ui_right, ui_accept, ui_cancel or whatever ui_* names are in
DEFAULT_KEYS) or rename the menu remap actions to match the existing ACTIONS
names; modify the ACTIONS array (symbol: ACTIONS) to exactly mirror the action
names in DEFAULT_KEYS so defaults and remapping work correctly.
♻️ Duplicate comments (2)
scripts/key_mapping.gd (2)
60-62: Reset handler still no-op.This remains just a log statement; users will expect an actual reset.
131-136: Guard against recursive toggled signals.Programmatic
button_pressedchanges can re-triggertoggledand loop.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
scenes/key_mapping_menu.tscnscripts/input_remap_button.gdscripts/key_mapping.gdscripts/pause_menu.gdscripts/settings.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (1)
scripts/pause_menu.gd (1)
54-55: The"pause"action is already properly defined inproject.godotwith default bindings (ESC key and joypad button 2), so there is no integration risk. The code change is safe and functional as-is.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
…signal loops. Because button_pressed can emit toggled even when changed in code, these handlers may trigger each other repeatedly, causing infinite loops or duplicate signals. Consider only updating the other button when its state actually differs, and use a simple guard flag (e.g. _updating_device_toggle) to block re-entrant updates while syncing them.
Replaced custom menu action names with standard Godot UI action names (e.g., 'ui_up', 'ui_down', etc.) in key mapping menu scene and settings. Updated key mapping script to improve logging for reset and toggle events. Removed unused mouse click logging from main menu script.
Improves input remapping by distinguishing between keyboard and gamepad events, updating button text and logging accordingly. Adds device-specific default mappings and enables resetting controls for only the selected device. Refactors event matching and updates settings to support separate keyboard and gamepad defaults.
Simplifies conditional logic in input_remap_button.gd by replacing 'elif' with 'if' for device-specific event filtering. Updates main_menu.gd to use '_event' as the input parameter name for clarity and adjusts related documentation comments.
MOUSE_BUTTON_LABELS was added, but _input() and get_event_label() still ignore InputEventMouseButton, so mouse bindings cannot be captured and will display as "Unbound". Mouse input is used elsewhere in the codebase (menus, audio settings, tests), so this support should be implemented.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/settings.gd`:
- Around line 105-112: The code assumes config.get_value("input", action)
returns an Array; to avoid errors with legacy single int or String values,
normalize the retrieved value into an Array before iterating and set the
migration flag: after var serialized_events := config.get_value("input", action)
check its type (int or String) and if so wrap it as serialized_events =
[serialized_events] and set _needs_migration = true; then proceed to call
InputMap.action_erase_events(action) and loop calling
_deserialize_and_add(action, serialized) as before so old single-key configs are
migrated safely.
♻️ Duplicate comments (2)
scripts/input_remap_button.gd (1)
142-194: Mouse remapping is still blocked (and CI lint fails onelifafterreturn).The device filter only accepts key/joypad events, so mouse buttons are never captured or labeled. CI also flags
no-elif-returnin_inputandget_matching_event. Consider treating mouse as part of the keyboard device (or add a separate device) and switch the filter to standaloneifs.🛠️ Proposed fix
- if current_device == DeviceType.KEYBOARD and not event is InputEventKey: - return - elif ( - current_device == DeviceType.GAMEPAD - and not (event is InputEventJoypadButton or event is InputEventJoypadMotion) - ): - return + if current_device == DeviceType.KEYBOARD and not ( + event is InputEventKey or event is InputEventMouseButton + ): + return + if ( + current_device == DeviceType.GAMEPAD + and not (event is InputEventJoypadButton or event is InputEventJoypadMotion) + ): + return @@ # Handle keyboard key press if event is InputEventKey and event.pressed: erase_old_event() # Remove the old matching event for this device var new_event := InputEventKey.new() # Consistent name: new_event @@ finish_remap() # Wrap up: update text, stop listening, save, etc. return + + # Handle mouse button press + if event is InputEventMouseButton and event.pressed: + erase_old_event() + var new_event := InputEventMouseButton.new() + new_event.button_index = event.button_index + InputMap.action_add_event(action, new_event) + Globals.log_message( + "User remapped action '" + action + "' to '" + get_event_label(new_event) + "'", + Globals.LogLevel.DEBUG + ) + finish_remap() + return @@ - if current_device == DeviceType.KEYBOARD and ev is InputEventKey: - return ev - elif ( - current_device == DeviceType.GAMEPAD - and (ev is InputEventJoypadButton or ev is InputEventJoypadMotion) - ): - return ev + if current_device == DeviceType.KEYBOARD and ( + ev is InputEventKey or ev is InputEventMouseButton + ): + return ev + if ( + current_device == DeviceType.GAMEPAD + and (ev is InputEventJoypadButton or ev is InputEventJoypadMotion) + ): + return ev @@ if event is InputEventKey: # FIX: Use physical_keycode for layout-agnostic labels (QWERTY-based). # This replaces the invalid 'key_label' and ensures consistency with your dict lookups. # OS.get_keycode_string() converts the enum (e.g., KEY_SPACE) to a string like "Space". return KEY_LABELS.get(event.physical_keycode, OS.get_keycode_string(event.physical_keycode)) + + if event is InputEventMouseButton: + return MOUSE_BUTTON_LABELS.get( + event.button_index, + "Mouse Button " + str(event.button_index) + )Also applies to: 215-227, 253-272
scenes/key_mapping_menu.tscn (1)
285-373: Menu tooltips still reference “next weapon”.These are copy/paste leftovers and are misleading for pause/menu actions. Please update the tooltip text to match each action.
🛠️ Proposed fix
- tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for pause button" @@ - tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for accept/confirm" @@ - tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for menu up" @@ - tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for menu down" @@ - tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for menu left" @@ - tooltip_text = "Key mapping for next weapon button" + tooltip_text = "Key mapping for menu right"
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
scenes/key_mapping_menu.tscnscripts/input_remap_button.gdscripts/key_mapping.gdscripts/main_menu.gdscripts/settings.gd
💤 Files with no reviewable changes (1)
- scripts/main_menu.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/input_remap_button.gd
[error] 222-222: Unnecessary "elif" after "return" (no-elif-return)
[error] 145-145: Unnecessary "elif" after "return" (no-elif-return)
🔇 Additional comments (1)
scripts/key_mapping.gd (1)
65-85: Device toggle + per-device reset wiring looks solid.ButtonGroup exclusivity plus
update_all_remap_buttons()keeps the UI in sync with the active device, and the reset handler cleanly scopes to the selected device.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Several menu-related tooltips still say “next weapon,” which is misleading in the UI.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scenes/key_mapping_menu.tscn`:
- Around line 408-413: The ControlResetButton node currently has a tooltip_text
typo ("Reste button"); update the tooltip_text property on the node named
"ControlResetButton" (the Button under Panel/Options/BtnContainer) to read
"Reset button" so the user-facing copy is correct.
♻️ Duplicate comments (1)
scripts/input_remap_button.gd (1)
18-22: Mouse remapping is still missing in the device filter and label path. With only KEYBOARD/GAMEPAD inDeviceTypeand noInputEventMouseButtonhandling, mouse bindings can’t be captured or displayed. If mouse remap is in scope, add aMOUSEdevice path and mouse button labels.🔧 Suggested fix
enum DeviceType { KEYBOARD, + MOUSE, GAMEPAD, } +const MOUSE_BUTTON_LABELS: Dictionary = { + MOUSE_BUTTON_LEFT: "Left Click", + MOUSE_BUTTON_RIGHT: "Right Click", + MOUSE_BUTTON_MIDDLE: "Middle Click", + MOUSE_BUTTON_XBUTTON1: "Mouse Button 4", + MOUSE_BUTTON_XBUTTON2: "Mouse Button 5", +} @@ if current_device == DeviceType.KEYBOARD and not event is InputEventKey: return + if current_device == DeviceType.MOUSE and not event is InputEventMouseButton: + return @@ # Handle keyboard key press if event is InputEventKey and event.pressed: @@ return + + # Handle mouse button press + if event is InputEventMouseButton and event.pressed: + erase_old_event() + var new_event := InputEventMouseButton.new() + new_event.button_index = event.button_index + InputMap.action_add_event(action, new_event) + Globals.log_message( + "User remapped action '" + action + "' to '" + get_event_label(new_event) + "'", + Globals.LogLevel.DEBUG + ) + finish_remap() + return @@ if current_device == DeviceType.KEYBOARD and ev is InputEventKey: return ev + if current_device == DeviceType.MOUSE and ev is InputEventMouseButton: + return ev @@ if event is InputEventKey: return KEY_LABELS.get(event.physical_keycode, OS.get_keycode_string(event.physical_keycode)) + + if event is InputEventMouseButton: + return MOUSE_BUTTON_LABELS.get(event.button_index, "Mouse Button " + str(event.button_index))Also applies to: 24-39, 128-181, 205-215, 241-250
🧹 Nitpick comments (1)
scenes/key_mapping_menu.tscn (1)
107-148: Consider a ButtonGroup so only one device toggle can be active. CheckButtons can both be pressed, which risks conflicting device state if handlers miss a case. A ButtonGroup (radio behavior) prevents inconsistent UI state.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scenes/key_mapping_menu.tscnscripts/input_remap_button.gd
🔇 Additional comments (2)
scripts/input_remap_button.gd (2)
24-75: Label dictionary updates look good. Adds Esc/Enter and trigger labels that improve display clarity.
196-215: This review comment is based on a scenario that doesn't occur in the codebase. The code is designed to support one binding per device type per action. TheDEFAULT_KEYBOARDandDEFAULT_GAMEPADdictionaries each define a single binding per action, andSettings.gdonly adds defaults when none exist (if not has_key_event/if not has_joy_event). The remap workflow atomically erases and replaces a single binding per device, soget_matching_event()returning the first (and only) match is correct for the intended design. No code in the codebase creates multiple keyboard or gamepad bindings for the same action.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
config.get_value("input", action) is assumed to be an Array, but older configs may store a single int keycode or a string. Iterating those can error and prevents migration. Normalize to an array and mark _needs_migration so existing users don’t lose mappings.
Fix tooltip typo (“Reste button”). User-facing copy should read “Reset button.”
|
Working on fixing unit tests now. |
Refactors and improves GdUnit4 tests for InputRemapButton and settings, including explicit device type assignment, stricter type annotations, and more robust fallback/default handling. Updates test descriptions and comments for clarity, and enhances persistence tests for unbound and multi-action scenarios. Also fixes array typing in settings.gd for input mapping deserialization.
Added run_gut_unit_tests.sh to automate running GUT unit tests with Godot. Updated test_combined_multi_manager_loads.gd and test_error_edge_cases.gd to reflect new default input mappings, including both keyboard and gamepad events for 'speed_up', and adjusted assertions to match the updated input serialization and remapping logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@run_gut_unit_tests.sh`:
- Around line 29-35: The script uses the variable $GODOT unquoted when invoking
the Godot executable, which will break for paths containing spaces; update both
invocations that call the Godot binary (the two places invoking $GODOT
--headless --path "$PROJECT_DIR" --import --quit and $GODOT --headless --verbose
--path "$PROJECT_DIR" -s addons/gut/gut_cmdln.gd ...) to wrap the variable in
double quotes (use "$GODOT") so the executable path is handled correctly.
🧹 Nitpick comments (1)
scenes/key_mapping_menu.tscn (1)
120-149: Consider a ButtonGroup to enforce a single active device toggle.
CheckButtons can both be active (or both off) unless code enforces exclusivity. If the UI should always have exactly one selected device, a ButtonGroup is a safer guardrail.🛠️ Suggested tweak (within this range)
[node name="Keyboard" type="CheckButton" parent="Panel/Options/DeviceTypeContainer"] +button_group = SubResource("ButtonGroup_device") @@ [node name="Gamepad" type="CheckButton" parent="Panel/Options/DeviceTypeContainer"] +button_group = SubResource("ButtonGroup_device")You’d also need to add a ButtonGroup sub-resource near the top of the scene:
[sub_resource type="ButtonGroup" id="ButtonGroup_device"]
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
run_gut_unit_tests.shscenes/key_mapping_menu.tscnscripts/settings.gdtest/gut/test_combined_multi_manager_loads.gdtest/gut/test_error_edge_cases.gdtest/test_input_remap_button.gdtest/test_settings.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (7)
scripts/settings.gd (1)
130-159: Default backfill may override device-specific unbinds.
The post-load backfill adds default keyboard/gamepad events whenever a device type is missing. If a user clears all gamepad bindings for an action, a reload will re-add defaults, so the unbind won’t persist. Please verify the intended UX (try unbinding a gamepad mapping, save, reload). If unbinds should persist, you’ll need a way to record “no gamepad” (e.g., per-device sections/sentinels) and skip backfilling for those actions.test/gut/test_combined_multi_manager_loads.gd (6)
40-46: LGTM! Setup correctly initializes keyboard-only defaults.The setup now properly uses
Settings.DEFAULT_KEYBOARD[action]for initialization, which aligns with the device-specific default mappings introduced in this refactor. The tests correctly expectload_input_mappingsto add the gamepad defaults, resulting in 2 events per action post-load.
69-77: LGTM! Assertions correctly verify the two-device mapping behavior.The test properly validates that after loading a config with only keyboard mapping, the
InputMapcontains both the keyboard event from config and the auto-added default gamepad event. The gamepad assertions checkingaxis,axis_value, anddeviceare thorough.
91-110: No changes in TC-SL-12.
126-136: LGTM! Migration test properly validates upgrade to two-device format.The test correctly verifies that:
- Old int format (
87) is migrated to array format with keyboard and gamepad defaults.InputMapcontains both events after migration.- Serialized config is upgraded to
["key:87", "joyaxis:5:1.0:-1"].
172-184: LGTM! Remap simulation correctly tests single-event persistence.The test accurately simulates a user remap scenario: erasing all existing events and adding only a single new key. The expectation of a single serialized entry (
["key:4194320"]) is correct—explicit remaps replace all previous mappings for the action, including auto-added defaults.Note: The test assumes
KEY_UP = 4194320, which is correct for Godot 4.x key constants.
189-208: LGTM! Concurrent save test validates section isolation.The test correctly verifies that rapid saves from
AudioManagerandGlobalsdon't overwrite each other's sections. The input section remains["key:87"]becauseload_input_mappingsisn't called in this test—only the config file's persisted value is checked, not theInputMapstate.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Refactor after_each() in audio-related GUT tests to safely free audio_instance, hide warning dialogs, remove config files, and await process_frame. This ensures proper cleanup and prevents test interference or resource leaks.
Lines 29 and 34 invoke $GODOT without quotes, which breaks when the executable path contains spaces (e.g., "Program Files"). The script's own comment on line 12 indicates users may set full paths like "C:/Path/To/Godot/godot.exe". Wrap both invocations with "$GODOT".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_audio_reset_button.gd`:
- Around line 48-51: The before_each() currently instantiates audio_instance via
audio_scene.instantiate() and adds it with add_child_autofree, but individual
tests like test_tc_reset_01 and test_tc_reset_02 also instantiate
audio_instance, causing potential double-instantiation and leaks; fix by
choosing one place to create the instance: either remove the instantiation lines
(audio_scene.instantiate(), add_child_autofree, await get_tree().process_frame)
from before_each() and keep per-test instantiation where tests require custom
AudioManager state, or remove the redundant instantiation calls in the tests and
rely on the single shared audio_instance created in before_each(); update tests
that expect a fresh instance to explicitly re-create or reset audio_instance as
needed.
🧹 Nitpick comments (5)
test/gut/test_preserve_other_sections.gd (1)
43-43: Trailing whitespace/tab character.Line 43 contains only whitespace (a tab). Consider removing this trailing whitespace for cleaner formatting.
🧹 Proposed fix
- +test/gut/test_sfx_rotor_volume_control.gd (1)
281-288: Missing cleanup for dynamically createdprev_menuControl.Unlike
test_tc_sfx_15intest_sfx_volume_control.gd(which hasprev_menu.queue_free()at line 325), this test doesn't free the dynamically createdControlnode. Consider addingprev_menu.queue_free()after the assertions for consistency.♻️ Suggested fix
assert_true(prev_menu.visible) assert_true(Globals.hidden_menus.is_empty()) + prev_menu.queue_free()test/gut/test_sfx_weapon_volume_control.gd (1)
281-288: Missing cleanup for dynamically createdprev_menuControl.Same as
test_sfx_rotor_volume_control.gd, this test creates aControlnode that isn't freed. Addprev_menu.queue_free()for consistency withtest_sfx_volume_control.gd.♻️ Suggested fix
assert_true(prev_menu.visible) assert_true(Globals.hidden_menus.is_empty()) + prev_menu.queue_free()test/gut/test_master_volume_control_and_music.gd (2)
65-78: Consider removing debugprint()statements.The test contains numerous
print()debug statements (lines 68, 70, 72, 74, 77, and similar patterns throughout other tests). While useful during development, these add noise to test output in CI/production runs. Consider removing them or converting to conditional debug logging.
350-359: Missing cleanup for dynamically createdprev_menuControl.For consistency with
test_sfx_volume_control.gd, addprev_menu.queue_free()after the assertions.♻️ Suggested fix
assert_true(prev_menu.visible) print("Hidden menus empty: ", Globals.hidden_menus.is_empty()) # Debug assert_true(Globals.hidden_menus.is_empty()) + prev_menu.queue_free()
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
test/gut/test_audio_reset_button.gdtest/gut/test_master_volume_control_and_music.gdtest/gut/test_preserve_other_sections.gdtest/gut/test_reset_scenarios.gdtest/gut/test_sfx_rotor_volume_control.gdtest/gut/test_sfx_volume_control.gdtest/gut/test_sfx_weapon_volume_control.gd
🔇 Additional comments (8)
test/gut/test_preserve_other_sections.gd (1)
45-58: Cleanup logic is sound and consistent with the broader test cleanup pattern.The defensive approach of hiding dialogs, explicitly removing nodes, and awaiting a frame ensures reliable teardown. A minor note: since
test_tc_sl_09usesadd_child_autofree(audio_instance)(line 157), GUT will also attempt cleanup automatically. Theis_instance_validchecks make this safe, but you could consider usingadd_childinstead ofadd_child_autofreein tests where you want full manual control over cleanup.test/gut/test_reset_scenarios.gd (1)
43-57: Improved cleanup logic looks good overall.The safe cleanup pattern with validity checks, dialog hiding, and frame await is a solid approach to prevent test interference. However, note that
remove_child(audio_instance)on line 52 may be redundant whenadd_child_autofreeis used (e.g., intest_tc_sl_17,test_tc_sl_19), since GUT automatically removes and frees autofree children after each test. This shouldn't cause errors but is worth being aware of for consistency.test/gut/test_sfx_volume_control.gd (2)
47-61: Cleanup pattern is consistent and well-implemented.The safe cleanup sequence with dialog hiding, node removal, and frame await aligns with the broader test suite pattern. This will help prevent test interference and resource leaks.
325-325: Good addition: Cleaning up dynamically created Control.Adding
prev_menu.queue_free()prevents a memory leak for the dynamically createdControlnode that isn't managed byadd_child_autofree.test/gut/test_audio_reset_button.gd (1)
56-67: Cleanup pattern is consistent with other test files.The safe cleanup logic properly handles dialog hiding, node removal, and frame processing. Note that unlike the other test files, this version doesn't use the local shadowing pattern (
var audio_instance: Control = self.audio_instance), which is fine since the instance variable access works correctly.test/gut/test_sfx_rotor_volume_control.gd (1)
47-61: Cleanup pattern is consistent and appropriate.The safe cleanup sequence aligns with the broader test suite improvements.
test/gut/test_sfx_weapon_volume_control.gd (1)
47-61: Cleanup pattern is consistent and appropriate.The safe cleanup sequence properly handles dialog hiding, node removal, and frame processing.
test/gut/test_master_volume_control_and_music.gd (1)
46-61: Good addition ofafter_each()for consistent cleanup.Adding the cleanup function ensures proper resource management and test isolation, aligning with the pattern used in other test files.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
The before_each() now instantiates audio_instance (lines 49-51), but several tests (e.g., test_tc_reset_01 at line 84, test_tc_reset_02 at line 154, etc.) also instantiate it again. This could cause the first instance to leak since it's overwritten before being freed. Consider either: Removing the instantiation from before_each() and keeping it in tests that need custom AudioManager state setup, or Removing the redundant instantiation from individual tests that don't need custom pre-setup.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
Settings.load_input_mappings, when a saved value has an unexpected type you still erase all events for that action and then add none; consider only callingaction_erase_eventsonce you have at least one validserialized_eventsentry so bad config data doesn’t silently wipe existing bindings. - The hard‑coded Xbox‑style defaults in
DEFAULT_GAMEPADare applied globally (device = -1) and merged into existing configs; if you expect other layouts or per‑device differences, it may be worth centralizing that mapping behind a small helper that can swap layouts or constrain which devices receive the defaults. - The
run_gut_unit_tests.shscript downloads GUT from GitHub at runtime whenaddons/gutis missing; you may want to remove the auto‑download and instead assume GUT is checked into the repo or installed separately to avoid network dependence and unexpected modifications of the project tree.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Settings.load_input_mappings`, when a saved value has an unexpected type you still erase all events for that action and then add none; consider only calling `action_erase_events` once you have at least one valid `serialized_events` entry so bad config data doesn’t silently wipe existing bindings.
- The hard‑coded Xbox‑style defaults in `DEFAULT_GAMEPAD` are applied globally (device = -1) and merged into existing configs; if you expect other layouts or per‑device differences, it may be worth centralizing that mapping behind a small helper that can swap layouts or constrain which devices receive the defaults.
- The `run_gut_unit_tests.sh` script downloads GUT from GitHub at runtime when `addons/gut` is missing; you may want to remove the auto‑download and instead assume GUT is checked into the repo or installed separately to avoid network dependence and unexpected modifications of the project tree.
## Individual Comments
### Comment 1
<location> `scripts/settings.gd:111-112` </location>
<code_context>
- if value is int: # Old format: single keycode int
+ var serialized_events: Array = []
+
+ if value is Array:
+ serialized_events = value as Array[String] # Modern format—assume strings
+ elif value is int:
+ serialized_events = ["key:" + str(value)] # Old int keycode—migrate to key format
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting the saved value to Array[String] can throw if the array contains non-string items, where the previous implementation handled this more defensively.
Previously, non-string elements were skipped with a warning so valid entries could still load. With `value as Array[String]`, any non-string element will cause a runtime error if a config is corrupted or in an older format. To keep loading robust for existing configs, please either restore element-wise iteration with type checks, or validate all items before casting.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…if the array contains non-string items, where the previous implementation handled this more defensively. Previously, non-string elements were skipped with a warning so valid entries could still load. With value as Array[String], any non-string element will cause a runtime error if a config is corrupted or in an older format. To keep loading robust for existing configs, please either restore element-wise iteration with type checks, or validate all items before casting.
|
@sourcery-ai guide |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/gut/test_audio_reset_button.gd`:
- Around line 52-63: The teardown in after_each() can call remove_child on
audio_instance even when it is not a child, causing "node is not a child"
errors; update after_each() to check that audio_instance is valid and is inside
self (or has get_parent() == self) before calling remove_child(audio_instance),
so only remove_child is invoked when audio_instance is actually a child, keeping
the subsequent queue_free(), null assignment, and file cleanup unchanged.
🧹 Nitpick comments (2)
scripts/settings.gd (2)
156-169: Log message may be misleading ifdef["type"]is unrecognized.The log on line 169 executes regardless of whether a gamepad event was actually added. If
def["type"]is neither"button"nor"axis", no event is added but the log still claims one was.♻️ Suggested fix
if not has_joy_event and DEFAULT_GAMEPAD.has(action): var def: Dictionary = DEFAULT_GAMEPAD[action] if def["type"] == "button": var nev: InputEventJoypadButton = InputEventJoypadButton.new() nev.button_index = def["button"] nev.device = -1 InputMap.action_add_event(action, nev) + Globals.log_message("Added default gamepad button for " + action, Globals.LogLevel.DEBUG) elif def["type"] == "axis": var nev: InputEventJoypadMotion = InputEventJoypadMotion.new() nev.axis = def["axis"] nev.axis_value = def["value"] nev.device = -1 InputMap.action_add_event(action, nev) - Globals.log_message("Added default gamepad event for " + action, Globals.LogLevel.DEBUG) + Globals.log_message("Added default gamepad axis for " + action, Globals.LogLevel.DEBUG)
293-295: Consider logging a warning for invaliddevice_type.The silent return when
device_typeis invalid could make debugging difficult if a caller passes an unexpected value.♻️ Suggested fix
func reset_to_defaults(device_type: String) -> void: if device_type not in ["keyboard", "gamepad"]: + Globals.log_message( + "Invalid device_type for reset: " + device_type, Globals.LogLevel.WARNING + ) return
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/settings.gdtest/gut/test_audio_reset_button.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (6)
scripts/settings.gd (6)
10-53: LGTM!The
ACTIONSarray now correctly includes UI navigation actions, and theDEFAULT_KEYBOARD/DEFAULT_GAMEPADconstants provide well-structured device-specific defaults. The nested dictionary approach for gamepad mappings cleanly handles both button and axis types.
58-64: LGTM!The initialization correctly loads mappings and conditionally persists migrated data only when legacy formats are detected, avoiding unnecessary file writes.
109-138: LGTM - Robust legacy format handling.The element-wise iteration with type checks (lines 113-122) correctly addresses the previous concern about
Array[String]casting failures. Non-string items are skipped with a warning, and legacyint/Stringformats are properly migrated.
181-255: LGTM!The deserialization logic is well-structured with comprehensive validation for each format (
key:,joybtn:,joyaxis:). The fallback for plain integer keycodes (lines 247-252) correctly handles legacy configs and triggers migration.
264-286: LGTM!The save logic correctly preserves other config sections by loading before writing, and properly handles errors with appropriate logging.
296-323: LGTM!Good practice duplicating the events array on line 297 before iterating to avoid modification during iteration. The reset logic correctly erases device-specific events and re-applies defaults, with automatic persistence.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Line 58 can throw if audio_instance was already reparented or auto-removed. A quick parent check keeps teardown resilient.
|
@sourcery-ai guide |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
1 participant
Refactor key_mapping_menu.tscn in SkyLockAssault using Godot 4.5 to separate keyboard, mouse, and gamepad remapping into dedicated sub-menus or tabs, accessed from the main key mapping menu.
Currently all input remapping options are shown together in key_mapping_menu.tscn, which mixes keyboard, and gamepad controls. This makes it harder for players to locate and configure controls for their device.
We should refactor this UI to separate device types into dedicated tabs, improving organization and UX, especially as SkyLockAssault grows support for multiple control schemes.
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
Separate device-specific input remapping and prepare controls for clearer keyboard vs gamepad handling while updating defaults and pause behavior.
New Features:
Bug Fixes:
Enhancements:
Summary by Sourcery
Refine the input mapping system to support separate keyboard and gamepad configurations, including defaults, UI controls, and persistence, while tightening related audio/UI tests and scripts.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit
New Features
Changes
Tests & Tools
Other
✏️ Tip: You can customize this high-level summary in your review settings.