Merged
Conversation
To resolve issue #279, we need to decouple the UI speed updates from the _physics_process loop. By introducing a speed_changed signal, the UI will only update when the speed value actually changes, rather than calculating warnings and bar sizes every single frame. Introduce a new speed_changed(float) signal in player.gd and hook it up to update UI and warnings. Connect/disconnect the handler in _ready/_exit_tree to avoid dangling connections, and emit the signal when speed actually changes (in _physics_process and on flameout). Also add guards for _settings validity and ensure flameout resets speed and notifies listeners. Add comprehensive GUT tests (test_player_fuel_logic.gd, test_player_movement_signals.gd and their uid files) that validate fuel behavior, rotor/timer reactions, lateral movement constraints, signal emission semantics, UI reactivity, and speed clamping.
ikostan
added this to the
Milestone 15: Resource-Driven Componentization & UI Standardization milestone
Contributor
Reviewer's GuideDecouples player speed/fuel UI from the player controller by introducing player speed signals and a dedicated HUD script, centralizes speed configuration in GameSettingsResource, and adds comprehensive GUT/GdUnit tests for speed, fuel, HUD, and movement behavior. Sequence diagram for player speed change and HUD updatesequenceDiagram
actor User
participant InputSystem
participant Player
participant GameSettingsResource as GameSettings
participant HUD
User->>InputSystem: Press speed_up / speed_down
InputSystem->>Player: Input.is_action_pressed()
activate Player
Player->>GameSettings: read current_fuel, acceleration, deceleration, max_speed, min_speed
Player->>Player: _set_speed(target_speed)
alt settings invalid
Player-->>Player: return without change
else settings valid
Player->>Player: clamp target_speed using min_speed, max_speed and fuel
alt speed changed
Player-->>HUD: speed_changed(new_speed, max_speed)
par HUD reacts
activate HUD
HUD->>HUD: _on_player_speed_changed(new_speed, max_speed)
HUD->>HUD: update_speed_bar()
HUD->>HUD: check_speed_warning()
deactivate HUD
and settings observers
GameSettings-->>HUD: setting_changed(max_speed, value) (optional later)
end
else speed unchanged
Player-->>Player: no signal emitted
end
end
deactivate Player
Sequence diagram for fuel depletion, engine flameout, and HUD reactionsequenceDiagram
participant FuelTimer as FuelTimer
participant Player
participant GameSettingsResource as GameSettings
participant HUD
loop every fuel tick
FuelTimer->>Player: _on_fuel_timer_timeout()
activate Player
Player->>GameSettings: read current_fuel, max_speed, base_consumption_rate, difficulty
Player->>GameSettings: current_fuel -= computed_consumption
deactivate Player
alt current_fuel <= 0 and first time depleted
GameSettings-->>Player: fuel_depleted
GameSettings-->>HUD: fuel_depleted
activate Player
Player->>Player: _on_player_out_of_fuel()
Player->>Player: _set_speed(0.0)
Player->>Player: rotor_stop(left,right), fuel_timer.stop()
Player-->>HUD: speed_changed(0.0, max_speed)
deactivate Player
activate HUD
HUD->>HUD: _on_player_out_of_fuel()
HUD->>HUD: update_speed_bar()
HUD->>HUD: check_speed_warning()
HUD->>HUD: update_fuel_bar()
HUD->>HUD: check_fuel_warning()
deactivate HUD
else fuel remaining
GameSettings-->>HUD: setting_changed(current_fuel, value)
activate HUD
HUD->>HUD: _on_setting_changed(current_fuel, value)
HUD->>HUD: update_fuel_bar()
HUD->>HUD: check_fuel_warning()
deactivate HUD
end
end
Class diagram for decoupled player, HUD, and settingsclassDiagram
class Player {
<<Node2D>>
+signal speed_changed(new_speed: float, max_speed: float)
+signal speed_low(threshold: float)
+signal speed_maxed()
+screen_size: Vector2
+player_x_min: float
+player_x_max: float
+player_y_min: float
+player_y_max: float
+speed: Dictionary
+_settings: GameSettingsResource
+_ready() void
+_exit_tree() void
+_input(event: InputEvent) void
+_physics_process(delta: float) void
+_on_setting_changed(setting_name: String, new_value: Variant) void
+_on_player_out_of_fuel() void
+_on_fuel_timer_timeout() void
+_set_speed(target_speed: float) void
+rotor_start(rotor: Node2D, rotor_sfx: AudioStreamPlayer2D) void
+rotor_stop(rotor: Node2D, rotor_sfx: AudioStreamPlayer2D) void
}
class HUD {
<<Panel>>
-_settings: GameSettingsResource
-_current_speed: float
-_fuel_state: Dictionary
-_speed_state: Dictionary
-_fuel_bar_style: StyleBoxFlat
-_speed_bar_style: StyleBoxFlat
-_connected_player: Node2D
+fuel_bar: ProgressBar
+fuel_label: Label
+fuel_blink_timer: Timer
+speed_bar: ProgressBar
+speed_label: Label
+speed_blink_timer: Timer
+_ready() void
+_exit_tree() void
+setup_hud(player_node: Node2D) void
+_on_player_speed_changed(new_speed: float, max_speed: float) void
+_on_setting_changed(setting_name: String, new_value: Variant) void
+_on_player_out_of_fuel() void
+update_fuel_bar() void
+update_speed_bar() void
+check_fuel_warning() void
+check_speed_warning() void
+start_blinking(state: Dictionary) void
+stop_blinking(state: Dictionary) void
+_on_fuel_blink_timer_timeout() void
+_on_speed_blink_timer_timeout() void
+_toggle_label(state: Dictionary) void
+get_label_text_color(label: Label) Color
+set_label_text_color(label: Label, new_color: Color) void
+set_bar_fill_style(bar: ProgressBar, bar_fill_style: StyleBoxFlat) void
+get_current_speed() float
+get_settings() GameSettingsResource
+get_fuel_bar_color() Color
+get_speed_bar_color() Color
+is_fuel_warning_active() bool
+is_speed_warning_active() bool
+is_speed_timer_running() bool
}
class GameSettingsResource {
<<Resource>>
+signal setting_changed(setting_name: String, new_value: Variant)
+signal fuel_depleted
+max_speed: float
+min_speed: float
+lateral_speed: float
+acceleration: float
+deceleration: float
+high_yellow_fraction: float
+low_yellow_fraction: float
+max_fuel: float
+current_fuel: float
+high_fuel_threshold: float
+medium_fuel_threshold: float
+low_fuel_threshold: float
+no_fuel_threshold: float
+base_consumption_rate: float
+difficulty: float
-_max_speed: float
-_min_speed: float
-_lateral_speed: float
-_acceleration: float
-_deceleration: float
-_high_yellow_fraction: float
-_low_yellow_fraction: float
+_init() void
+set_max_speed(value: float) void
+set_min_speed(value: float) void
+set_lateral_speed(value: float) void
+set_acceleration(value: float) void
+set_deceleration(value: float) void
+set_high_yellow_fraction(value: float) void
+set_low_yellow_fraction(value: float) void
}
class Globals {
<<AutoLoad>>
+settings: GameSettingsResource
+enum LogLevel
+log_message(message: String, level: LogLevel) void
}
class MainScene {
<<Node>>
+player: Node2D
+stats_panel: Variant
+_ready() void
}
Player --> GameSettingsResource : uses _settings
Player --> Globals : uses log_message
Player --> HUD : emits speed_changed
HUD --> GameSettingsResource : observes setting_changed
HUD --> GameSettingsResource : observes fuel_depleted
HUD --> Globals : uses log_message
Globals --> GameSettingsResource : owns settings
MainScene --> Player : has child
MainScene --> HUD : has child
MainScene --> HUD : calls setup_hud(player)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughPlayer now emits external speed signals and relies on Globals.settings for speed/fuel config; HUD moved into its own script and listens to settings/player signals; tests added for fuel and movement signal behavior; GameSettingsResource gained new speed-related exported properties and emits setting_changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Player (scripts/player.gd)
participant Settings as Globals.settings (GameSettingsResource)
participant HUD as HUD Panel (scripts/hud.gd)
participant Rotor as Rotor/Animations
participant Timer as FuelTimer
rect rgba(200,220,255,0.5)
Settings->>Player: setting_changed (speed/fuel props)
end
rect rgba(220,255,200,0.5)
Player->>Player: _physics_process (accel/decel, clamp)
Player-->>Player: emit speed_changed(new, max) [only on change]
Player-->>Player: emit speed_maxed / speed_low as applicable
end
rect rgba(255,230,200,0.5)
Player->>HUD: speed_changed(...)
Settings->>HUD: setting_changed (fuel/max updates)
HUD->>HUD: update bars/colors/blink timers
end
rect rgba(255,200,220,0.5)
Timer->>Player: fuel depleted -> _on_player_out_of_fuel()
Player->>Rotor: stop animations
Player-->>HUD: emit speed_changed(0.0, max) and speed_low if threshold hit
Timer->>Timer: stop
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Apr 14, 2026 4:57a.m. | Review ↗ |
| JavaScript | |
Apr 14, 2026 4:57a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
![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 left some high level feedback:
- The speed change + signal emission logic is duplicated in both
_physics_processand_on_player_out_of_fuel; consider centralizing this into a small helper (e.g.,set_speed(new_speed: float)or_update_speed(new_speed: float)) that handles clamping and emittingspeed_changedwhen the value actually changes. - The mock scene construction logic in
_build_mock_player_scene()is nearly identical intest_player_fuel_logic.gdandtest_player_movement_signals.gd; extracting this into a shared test helper would reduce duplication and keep future changes to the player scene shape in one place. - In
_on_speed_changed(new_speed: float)the parameter is unused and the method re-reads state fromspeed; either rely solely on the signal argument (to avoid extra coupling to the internalspeeddictionary) or make the unused parameter explicit (e.g.,_new_speed) and document that the handler intentionally uses internal state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The speed change + signal emission logic is duplicated in both `_physics_process` and `_on_player_out_of_fuel`; consider centralizing this into a small helper (e.g., `set_speed(new_speed: float)` or `_update_speed(new_speed: float)`) that handles clamping and emitting `speed_changed` when the value actually changes.
- The mock scene construction logic in `_build_mock_player_scene()` is nearly identical in `test_player_fuel_logic.gd` and `test_player_movement_signals.gd`; extracting this into a shared test helper would reduce duplication and keep future changes to the player scene shape in one place.
- In `_on_speed_changed(new_speed: float)` the parameter is unused and the method re-reads state from `speed`; either rely solely on the signal argument (to avoid extra coupling to the internal `speed` dictionary) or make the unused parameter explicit (e.g., `_new_speed`) and document that the handler intentionally uses internal state.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
🧹 Nitpick comments (2)
test/gut/test_player_fuel_logic.gd (1)
32-38: Teardown input cleanup can be made more robust.
after_each()only releasesmove_left; consider releasing all potentially pressed actions (speed_up,speed_down,move_left,move_right) to avoid future leakage as tests expand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_player_fuel_logic.gd` around lines 32 - 38, The teardown in after_each() currently only releases "move_left" and may leave other inputs pressed; update after_each() to iterate over a list of known actions (e.g., ["speed_up","speed_down","move_left","move_right"]) and call Input.action_release for each, and also ensure any actions tracked in _added_actions are erased via InputMap.erase_action(action) as already done, so all potential pressed inputs are released and removed to avoid test leakage.test/gut/test_player_movement_signals.gd (1)
124-233: Consider extracting the mock player scene builder into a shared test helper.This helper is largely duplicated with
test/gut/test_player_fuel_logic.gd, which will make fixture updates harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_player_movement_signals.gd` around lines 124 - 233, The duplicated scene builder (_build_mock_player_scene) should be extracted into a shared test helper module and both tests should call it; create a new helper script (e.g., a Gut test helper) that exposes a function build_mock_player_scene() which contains the current body of _build_mock_player_scene (move the GDScript creation, rotor setup, mock weapon script, timers, etc.), ensure it accepts any environment-specific inputs (like PLAYER_SCRIPT_PATH or pass it in) or uses the same constant, then update test/gut/test_player_movement_signals.gd and test/gut/test_player_fuel_logic.gd to call build_mock_player_scene() instead of duplicating the code and remove the local _build_mock_player_scene definition from each test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/player.gd`:
- Around line 219-221: Remove trailing whitespace on the blank lines reported by
the formatter (e.g., around the speed signal connect call
speed_changed.connect(_on_speed_changed) and the other blank lines near the
functions/blocks around those regions), then run gdformat to reformat the file;
ensure no trailing spaces remain on blank lines (including the ranges noted:
around the connect call and the other groups) so gdformat --check passes.
- Around line 568-570: The UI is still being updated every physics frame despite
emitting speed_changed; update the UI only when the speed actually changes by
moving the UI update logic out of _physics_process and into the speed_changed
signal handler (or wrap the UI update in a conditional tied to the emission),
e.g., use the existing speed_changed.emit(speed["speed"]) as the single trigger
and remove the unconditional per-frame UI update in _physics_process so the UI
update runs only when speed_changed fires.
In `@test/gut/test_player_movement_signals.gd`:
- Around line 104-119: The test function test_speed_clamps_to_max_and_min
currently only verifies the upper clamp (uses _player.speed["speed"], max_cap
and calls Input.action_press("speed_up") then _player._physics_process(10.0)) —
either rename it to reflect only max behavior (e.g., test_speed_clamps_to_max)
or extend it to also verify the minimum clamp by setting _player.speed["speed"]
below the configured min (use _player.speed["min"]), simulate the appropriate
input (e.g., Input.action_press for deceleration or directly call
_player._physics_process with a negative delta), then
assert_eq(float(_player.speed["speed"]), min_cap) to validate the lower bound;
update assertions and test name accordingly.
---
Nitpick comments:
In `@test/gut/test_player_fuel_logic.gd`:
- Around line 32-38: The teardown in after_each() currently only releases
"move_left" and may leave other inputs pressed; update after_each() to iterate
over a list of known actions (e.g.,
["speed_up","speed_down","move_left","move_right"]) and call
Input.action_release for each, and also ensure any actions tracked in
_added_actions are erased via InputMap.erase_action(action) as already done, so
all potential pressed inputs are released and removed to avoid test leakage.
In `@test/gut/test_player_movement_signals.gd`:
- Around line 124-233: The duplicated scene builder (_build_mock_player_scene)
should be extracted into a shared test helper module and both tests should call
it; create a new helper script (e.g., a Gut test helper) that exposes a function
build_mock_player_scene() which contains the current body of
_build_mock_player_scene (move the GDScript creation, rotor setup, mock weapon
script, timers, etc.), ensure it accepts any environment-specific inputs (like
PLAYER_SCRIPT_PATH or pass it in) or uses the same constant, then update
test/gut/test_player_movement_signals.gd and test/gut/test_player_fuel_logic.gd
to call build_mock_player_scene() instead of duplicating the code and remove the
local _build_mock_player_scene definition from each test file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f001678-52a1-48e0-8f38-b9e55d162067
📒 Files selected for processing (5)
scripts/player.gdtest/gut/test_player_fuel_logic.gdtest/gut/test_player_fuel_logic.gd.uidtest/gut/test_player_movement_signals.gdtest/gut/test_player_movement_signals.gd.uid
📜 Review details
⏰ 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: Sourcery review
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-04-10T00:07:53.308Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:53.308Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
test/gut/test_player_fuel_logic.gd.uidtest/gut/test_player_movement_signals.gdtest/gut/test_player_fuel_logic.gd
📚 Learning: 2026-04-10T00:07:53.308Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:53.308Z
Learning: For Godot 4 GDScript files scripts/globals.gd and scripts/player.gd, treat `current_fuel` as volatile session state: do not read from or write `current_fuel` to `settings.cfg`, and do not attempt mid-run persistence via settings. `current_fuel` should always be reset to `max_fuel` unconditionally in `player._ready()`. Only `max_fuel` is a persistent setting; persisting `current_fuel` in settings.cfg is considered an architectural mistake by the project maintainer. Mid-run fuel persistence should be handled via a separate `SaveGameResource` (planned/tracked in PR `#535`), not `settings.cfg`.
Applied to files:
scripts/player.gd
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_player_movement_signals.gdtest/gut/test_player_fuel_logic.gd
🪛 GitHub Actions: Pull Request Pipeline
scripts/player.gd
[error] 216-216: gdformat --diff --check would reformat ./scripts/player.gd (whitespace/formatting change: removed trailing whitespace on blank line).
[error] 249-249: gdformat --diff --check would reformat ./scripts/player.gd (whitespace/formatting change: removed trailing whitespace on blank line).
[error] 296-296: gdformat --diff --check would reformat ./scripts/player.gd (whitespace/formatting change: removed trailing whitespace on blank line).
[error] 547-547: gdformat --diff --check would reformat ./scripts/player.gd (whitespace/formatting change: removed trailing whitespace on blank line).
[error] 564-564: gdformat --diff --check would reformat ./scripts/player.gd (whitespace/formatting change: removed trailing whitespace on blank line).
🔇 Additional comments (5)
test/gut/test_player_fuel_logic.gd.uid (1)
1-1: UID mapping entry is valid.This looks correct for registering the new GUT test resource.
test/gut/test_player_movement_signals.gd.uid (1)
1-1: UID mapping entry is valid.Looks good for the new movement/signals GUT test file.
scripts/player.gd (1)
297-301: Good emission guard on flameout speed reset.The
old_speedcomparison avoids redundantspeed_changedemissions when speed is already zero.test/gut/test_player_movement_signals.gd (1)
18-40: Test isolation setup/cleanup is solid.Good job restoring
Globals.settingsand cleaning temporaryInputMapactions per test.test/gut/test_player_fuel_logic.gd (1)
41-96: Fuel/engine state transition coverage is strong.These tests exercise observer-driven UI sync, flameout behavior, reignition, and no-fuel movement constraints well.
…s checked). Test name and assertions are inconsistent (max_and_min but only max is checked). Line 104 says max and min constraints, but this test only validates max clamp. Either rename it or add a min-clamp assertion.
The PR adds only a single signal speed_changed(new_speed: float) to Player.gd. It does not define speed_low(threshold: float) or speed_maxed(), and the speed_changed signature does not include max_speed as specified in the issue.
Add a standalone HUD (scripts/hud.gd + uid) and attach it to PlayerStatsPanel in main_scene.tscn; wire the HUD to the Player in scripts/main_scene.gd (stats_panel.setup_hud(player)). Refactor GameSettingsResource (scripts/game_settings_resource.gd) to expose speed-related exported properties (max/min speed, lateral speed, accel/decel, yellow fractions) with backing fields and emit setting_changed when updated. Clean up and decouple UI logic from player.gd: remove direct ProgressBar/Label manipulation and UI timers, use Globals.settings for fuel/speed values, emit speed_changed with correct max_speed, use settings values for physics (acceleration, limits, consumption), and improve rotor/weapon null-safety and logging. Other: update scene file load_steps and add hud script as an external resource in main_scene.tscn. These changes separate UI from game logic and centralize tuning in the settings resource.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/game_settings_resource.gd`:
- Around line 47-72: The exported motion properties lateral_speed, acceleration,
and deceleration can be set negative; modify their setters in
scripts/game_settings_resource.gd (the `@export` var lateral_speed, `@export` var
acceleration, and `@export` var deceleration properties) to clamp incoming value
to a non-negative floor (>= 0.0) before comparing/assigning and emitting
setting_changed (e.g., compute a sanitized_value = max(value, 0.0) or use
clamp(value, 0.0, INF) and then use sanitized_value in the equality check,
assignment to _lateral_speed/_acceleration/_deceleration and the
setting_changed.emit call).
- Around line 27-45: The max_speed and min_speed setters must enforce min_speed
<= max_speed: in max_speed.set, after clamping new_val, if new_val < _min_speed
update _min_speed = new_val and emit setting_changed.emit("min_speed",
_min_speed) before assigning _max_speed and emitting the max_speed change; in
min_speed.set, after clamping new_val, if new_val > _max_speed update _max_speed
= new_val and emit setting_changed.emit("max_speed", _max_speed) before
assigning _min_speed and emitting the min_speed change; keep the existing
early-return checks and use the same private fields (_max_speed, _min_speed) and
event (setting_changed.emit) so external listeners see both adjusted values.
In `@scripts/hud.gd`:
- Around line 11-23: Replace the hard-coded speed constants in hud.gd
(MAX_SPEED, MIN_SPEED, HIGH_YELLOW_FRACTION, LOW_YELLOW_FRACTION,
HIGH_RED_THRESHOLD, HIGH_YELLOW_THRESHOLD, LOW_YELLOW_THRESHOLD,
LOW_RED_THRESHOLD) with values derived from the shared GameSettingsResource used
by player.gd: fetch or inject the GameSettingsResource instance (e.g., an
exported or singleton _settings) and compute thresholds at runtime from
_settings.max_speed, _settings.min_speed and _settings.low_yellow_fraction (and
high fractions if present), and update HUD color/blink calculations to use those
computed thresholds; ensure the HUD recalculates thresholds when the settings
change so runtime config updates stay in sync with player.gd behavior.
- Around line 52-61: The HUD's _ready() currently returns early if
Globals.settings is missing, leaving styles/timers/observers uninitialized;
instead ensure initialization always runs and only skip the connection to
settings when _settings is null: in _ready() initialize styles, timers and
observers unconditionally, set _settings = Globals.settings if available, and if
_settings is valid connect its signals (_on_setting_changed and
_on_player_out_of_fuel); if _settings is null, don’t return—either register to
watch for Globals being instantiated (or poll/queue a deferred check) and call
the same connection code when a fallback GameSettingsResource appears (e.g.,
created by Player), so PlayerStatsPanel and HUD finish setup even when
Globals.settings is created later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c419a249-1e41-4d1f-b6bf-43ece284ba3a
📒 Files selected for processing (7)
scenes/main_scene.tscnscripts/game_settings_resource.gdscripts/hud.gdscripts/hud.gd.uidscripts/main_scene.gdscripts/player.gdtest/gut/test_player_movement_signals.gd
✅ Files skipped from review due to trivial changes (2)
- scripts/hud.gd.uid
- test/gut/test_player_movement_signals.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-04-10T00:07:55.427Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
scripts/hud.gdscripts/game_settings_resource.gd
📚 Learning: 2026-04-10T00:07:53.308Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:53.308Z
Learning: For Godot 4 GDScript files scripts/globals.gd and scripts/player.gd, treat `current_fuel` as volatile session state: do not read from or write `current_fuel` to `settings.cfg`, and do not attempt mid-run persistence via settings. `current_fuel` should always be reset to `max_fuel` unconditionally in `player._ready()`. Only `max_fuel` is a persistent setting; persisting `current_fuel` in settings.cfg is considered an architectural mistake by the project maintainer. Mid-run fuel persistence should be handled via a separate `SaveGameResource` (planned/tracked in PR `#535`), not `settings.cfg`.
Applied to files:
scripts/player.gd
🪛 GitHub Actions: Pull Request Pipeline
scripts/player.gd
[warning] 79-79: WARNING: Player sprite texture missing! Using fallback size. (push_warning core/variant/variant_utility.cpp:1034)
🔇 Additional comments (1)
scripts/player.gd (1)
233-247: Nice event gating aroundspeed_changed.Emitting only after the post-clamp value actually changes keeps the HUD decoupled without reintroducing the old per-frame update churn.
Update unit tests to match refactored game resources and HUD wiring: use Globals.settings.min_speed/max_speed instead of local MIN/MAX constants, emit speed_changed with resource values, and change _player typing to Variant for dynamic access. Wire mock PlayerStatsPanel to the Player and assign hud.gd script to the mock panel so UI elements (fuel/speed bars) are retrieved from the HUD. Fix AnimatedSpriteFrames setup for Godot 4 by removing add_animation("default") and adding a dummy frame to avoid crashes. Also ensure input actions are explicitly released after tests and clean up minor mocked node/commented code redundancies.
Adjust unit and integration tests to match recent refactors that moved player constants/logic into Globals.settings and separated the HUD into PlayerStatsPanel. Replaces usages of player.MAX_SPEED, MIN_SPEED, base_fuel_drain, and local fuel bar references with Globals.settings.max_speed/min_speed/base_consumption_rate/current_fuel and hud.fuel_bar / hud.speed_bar. Also adapts blinking/state APIs and label accessors to the new HUD structure and updates assertions to derive values from dynamic max_fuel/max_speed rather than hardcoded 100/Player constants.
The speed change + signal emission logic is duplicated in both _physics_process and _on_player_out_of_fuel; consider centralizing this into a small helper (e.g., set_speed(new_speed: float) or _update_speed(new_speed: float)) that handles clamping and emitting speed_changed when the value actually changes.
Add test/gut/gut_test_helper.gd (and uid) providing build_mock_player_scene() to construct a reusable mock Player HUD/scene for GUT tests. Update test_player_fuel_logic.gd and test_player_movement_signals.gd to preload the helper and call the shared builder, wire the HUD to the Player, and remove the duplicated inline mock-builder code. Also include small test cleanup: ensure mocked input actions are released, keep player as Variant for dynamic access, and minor comment/clarity tweaks.
Owner
Author
All addressed |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The HUD tests reach into internal fields like
_fuel_state,_speed_state,_fuel_bar_style, and_current_speed; consider adding small accessor methods or public query helpers onhud.gdso tests don’t depend on private implementation details that may change. - In
setup_hudyou connectplayer_node.speed_changedto the HUD but never disconnect it; if players or HUDs are re-instantiated during a session (scene reloads, level transitions), consider adding a corresponding disconnect (or using a weak reference / one-shot connection) to avoid dangling connections.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The HUD tests reach into internal fields like `_fuel_state`, `_speed_state`, `_fuel_bar_style`, and `_current_speed`; consider adding small accessor methods or public query helpers on `hud.gd` so tests don’t depend on private implementation details that may change.
- In `setup_hud` you connect `player_node.speed_changed` to the HUD but never disconnect it; if players or HUDs are re-instantiated during a session (scene reloads, level transitions), consider adding a corresponding disconnect (or using a weak reference / one-shot connection) to avoid dangling connections.
## Individual Comments
### Comment 1
<location path="scripts/game_settings_resource.gd" line_range="84-93" />
<code_context>
+ get:
+ return _deceleration
+
+@export var high_yellow_fraction: float = 0.80:
+ set(value):
+ var new_val: float = clamp(value, 0.0, 1.0)
+ if _high_yellow_fraction == new_val:
+ return
+ _high_yellow_fraction = new_val
+ setting_changed.emit("high_yellow_fraction", _high_yellow_fraction)
+ get:
+ return _high_yellow_fraction
+
+@export var low_yellow_fraction: float = 0.10:
+ set(value):
+ var new_val: float = clamp(value, 0.0, 1.0)
</code_context>
<issue_to_address>
**issue (bug_risk):** high_yellow_fraction and low_yellow_fraction can be configured into an inverted or overlapping range, which leads to ambiguous threshold logic in the HUD.
Both values are clamped to `[0, 1]`, but nothing enforces `low_yellow_fraction <= high_yellow_fraction`. If `low` is set above `high`, the thresholds in `hud.gd` (`high_yellow_thresh`, `low_yellow_thresh`) can invert or collapse, making the yellow zone behavior unpredictable. Consider enforcing the ordering (e.g., each setter clamps against the other field or normalizes the pair so `low <= high`) to avoid these visual glitches when changed from the inspector.
</issue_to_address>
### Comment 2
<location path="scripts/hud.gd" line_range="115-124" />
<code_context>
+## Call this from your main level script when instantiating the player and UI.
+## @param player_node: The Player Node2D instance.
+## @return: void
+func setup_hud(player_node: Node2D) -> void:
+ if not is_instance_valid(player_node):
+ push_error("HUD setup failed: Invalid player node.")
+ return
+
+ # Connection guard for external wiring
+ if not player_node.speed_changed.is_connected(_on_player_speed_changed):
+ player_node.speed_changed.connect(_on_player_speed_changed)
+
+ Globals.log_message("HUD successfully wired to Player signals.", Globals.LogLevel.DEBUG)
+
+
</code_context>
<issue_to_address>
**issue (bug_risk):** setup_hud assumes the player_node has a speed_changed signal but the signature is only Node2D, which can lead to runtime errors if miswired.
Because `setup_hud` takes a generic `Node2D` but immediately accesses `player_node.speed_changed`, passing the wrong node type (or later renaming/removing the signal) will cause a runtime error. To make this safer, either narrow the parameter type to your concrete `Player` script that defines `speed_changed`, or check `player_node.has_signal("speed_changed")` and report a clear error instead of crashing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Won't fix, low priority and entirely safe to ignore for now |
Owner
Author
While Godot 4.x does a fantastic job of automatically cleaning up signal connections when a node is completely destroyed, CodeRabbit is raising a valid architectural concern: if the player respawns or the Player node is hot-swapped while the PlayerStatsPanel remains active in the scene tree, the HUD will hold onto a dangling reference and try to listen to a ghost. To resolve this, we need to cache a reference to the player when setup_hud is called, and explicitly sever that connection in _exit_tree(). |
While Godot 4.x does a fantastic job of automatically cleaning up signal connections when a node is completely destroyed, CodeRabbit is raising a valid architectural concern: if the player respawns or the Player node is hot-swapped while the PlayerStatsPanel remains active in the scene tree, the HUD will hold onto a dangling reference and try to listen to a ghost. To resolve this, we need to cache a reference to the player when setup_hud is called, and explicitly sever that connection in _exit_tree().
… configured into an inverted or overlapping range, which leads to ambiguous threshold logic in the HUD. issue (bug_risk): high_yellow_fraction and low_yellow_fraction can be configured into an inverted or overlapping range, which leads to ambiguous threshold logic in the HUD. Both values are clamped to [0, 1], but nothing enforces low_yellow_fraction <= high_yellow_fraction. If low is set above high, the thresholds in hud.gd (high_yellow_thresh, low_yellow_thresh) can invert or collapse, making the yellow zone behavior unpredictable. Consider enforcing the ordering (e.g., each setter clamps against the other field or normalizes the pair so low <= high) to avoid these visual glitches when changed from the inspector.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/gdunit4/test_difficulty.gd (1)
51-56: ReuseTestHelpers.calculate_expected_depletion()here.These three blocks now duplicate the same
max_speed/base_consumption_ratemath that already lives intest/gdunit4/test_helpers.gd. Calling the helper would keep the expectations aligned if the depletion formula changes again.Also applies to: 69-74, 87-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gdunit4/test_difficulty.gd` around lines 51 - 56, The test duplicates the depletion math instead of calling the shared helper; replace the manual normalized_speed and dep_1 calculations in test_difficulty.gd with calls to TestHelpers.calculate_expected_depletion(player_inst.speed["speed"], Globals.settings.max_speed, Globals.settings.base_consumption_rate, Globals.settings.difficulty) (or the helper's appropriate parameter order) so the test reuses the centralized formula; apply the same replacement to the other duplicate blocks that compute the same values (the blocks starting after the first one and the one later in the file) to keep expectations consistent with TestHelpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/gut/test_player_movement_signals.gd`:
- Around line 35-42: Cleanup currently erases temporary InputMap entries before
releasing simulated inputs which can call Input.action_release on actions that
no longer exist; change the order in the teardown to call
Input.action_release("speed_up") and Input.action_release("speed_down") (and any
other simulated actions) before iterating over _added_actions and calling
InputMap.erase_action(action), then clear _added_actions and restore
Globals.settings as currently done so Input.action_release runs against defined
actions; update the teardown around Input.action_release, InputMap.erase_action,
and _added_actions accordingly.
- Around line 84-91: The test is ambiguous because setting
Globals.settings.current_fuel = 0.0 already emits fuel_depleted and may trigger
the player's handler before you manually call _player._on_player_out_of_fuel(),
so change the setup to avoid the auto-emission: either set
Globals.settings.current_fuel to a small positive value (e.g., 0.1) so no
fuel_depleted signal fires, then call _player._on_player_out_of_fuel() and
assert on float(_player.speed["speed"]) and assert_signal_emitted(_player,
"speed_changed"); or explicitly disconnect the fuel_depleted connection from the
player before setting current_fuel to 0.0, then reconnect after the manual
_player._on_player_out_of_fuel() call, ensuring the test isolates the handler
named in the assertions.
---
Nitpick comments:
In `@test/gdunit4/test_difficulty.gd`:
- Around line 51-56: The test duplicates the depletion math instead of calling
the shared helper; replace the manual normalized_speed and dep_1 calculations in
test_difficulty.gd with calls to
TestHelpers.calculate_expected_depletion(player_inst.speed["speed"],
Globals.settings.max_speed, Globals.settings.base_consumption_rate,
Globals.settings.difficulty) (or the helper's appropriate parameter order) so
the test reuses the centralized formula; apply the same replacement to the other
duplicate blocks that compute the same values (the blocks starting after the
first one and the one later in the file) to keep expectations consistent with
TestHelpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea6f9c5d-4f49-4324-97ed-3fa471408deb
📒 Files selected for processing (15)
scripts/game_settings_resource.gdscripts/globals.gdscripts/hud.gdscripts/player.gdtest/gdunit4/test_difficulty.gdtest/gdunit4/test_difficulty_integration.gdtest/gdunit4/test_helpers.gdtest/gdunit4/test_player.gdtest/gut/gut_test_helper.gdtest/gut/gut_test_helper.gd.uidtest/gut/test_fuel_additional_edge_cases.gdtest/gut/test_hud.gdtest/gut/test_hud.gd.uidtest/gut/test_player_fuel_logic.gdtest/gut/test_player_movement_signals.gd
✅ Files skipped from review due to trivial changes (2)
- test/gut/gut_test_helper.gd.uid
- test/gut/test_hud.gd.uid
🚧 Files skipped from review as they are similar to previous changes (5)
- test/gut/test_fuel_additional_edge_cases.gd
- test/gut/test_player_fuel_logic.gd
- scripts/hud.gd
- scripts/game_settings_resource.gd
- scripts/player.gd
📜 Review details
⏰ 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 context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
📚 Learning: 2026-04-10T00:07:55.427Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:55.427Z
Learning: In `scripts/globals.gd` and `scripts/player.gd` (GDScript, Godot 4), `current_fuel` is intentionally treated as volatile session data and should NOT be saved to or loaded from `settings.cfg`. Only `max_fuel` (tank capacity) is a persistent setting. `current_fuel` is always reset to `max_fuel` unconditionally in `player._ready()`. Persisting `current_fuel` is considered an architectural mistake by the project maintainer (ikostan). Mid-run fuel state persistence is planned via a separate `SaveGameResource` in PR `#535`.
Applied to files:
test/gdunit4/test_difficulty_integration.gdtest/gdunit4/test_difficulty.gdtest/gdunit4/test_helpers.gdtest/gut/test_hud.gdtest/gdunit4/test_player.gd
📚 Learning: 2026-03-30T04:02:23.747Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 500
File: test/gut/test_audio_web_bridge.gd:131-145
Timestamp: 2026-03-30T04:02:23.747Z
Learning: In GUT (Godot Unit Test) for Godot 4, when using `assert_called` / `assert_called_count` with parameter matching, include *every* argument the mocked method accepts, including parameters with default values. GUT does not auto-fill default arguments during call matching. For example, if `JavaScriptBridgeWrapper.eval(script: String, global_exec: bool = false)` is invoked as `eval(js_string)`, the actual call recorded by GUT includes the default (`eval(js_string, false)`), so your assertion must match both arguments (e.g., `.bind(js_string, false)`, not `.bind(js_string)`). Apply this rule to GUT assertions in `test/gut` tests.
Applied to files:
test/gut/test_player_movement_signals.gdtest/gut/gut_test_helper.gdtest/gut/test_hud.gd
📚 Learning: 2026-04-10T00:07:53.308Z
Learnt from: ikostan
Repo: ikostan/SkyLockAssault PR: 534
File: scripts/player.gd:146-149
Timestamp: 2026-04-10T00:07:53.308Z
Learning: For Godot 4 GDScript files scripts/globals.gd and scripts/player.gd, treat `current_fuel` as volatile session state: do not read from or write `current_fuel` to `settings.cfg`, and do not attempt mid-run persistence via settings. `current_fuel` should always be reset to `max_fuel` unconditionally in `player._ready()`. Only `max_fuel` is a persistent setting; persisting `current_fuel` in settings.cfg is considered an architectural mistake by the project maintainer. Mid-run fuel persistence should be handled via a separate `SaveGameResource` (planned/tracked in PR `#535`), not `settings.cfg`.
Applied to files:
scripts/globals.gd
🔇 Additional comments (1)
scripts/globals.gd (1)
329-330: Nice guard for test-time logging.This keeps log output available when
Globals.settingsis intentionally null/freed, without changing the normal threshold behavior once settings exists.
…ed signal but the signature is only Node2D, which can lead to runtime errors if miswired.
issue (bug_risk): setup_hud assumes the player_node has a speed_changed signal but the signature is only Node2D, which can lead to runtime errors if miswired.
Because setup_hud takes a generic Node2D but immediately accesses player_node.speed_changed, passing the wrong node type (or later renaming/removing the signal) will cause a runtime error. To make this safer, either narrow the parameter type to your concrete Player script that defines speed_changed, or check player_node.has_signal("speed_changed") and report a clear error instead of crashing.
Release simulated actions before erasing them. If speed_up / speed_down were added in before_each(), erasing them first can make the later Input.action_release(...) calls hit undefined actions during cleanup. Release first, then remove any temporary InputMap entries.
This flameout assertion is currently ambiguous. Globals.settings.current_fuel = 0.0 already emits fuel_depleted, and scripts/player.gd handles that by calling _on_player_out_of_fuel() through the live connection. That means assert_signal_emitted() can succeed before the explicit manual call on Line 88, so the test does not isolate the handler it is naming.
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:
- Several comments in the new scripts (e.g.,
NEW FIX, long banner comments inhud.gd, etc.) read like change logs rather than maintenance documentation; consider trimming or rephrasing them to focus on current behavior to keep the codebase concise and easier to scan. - The new
speed_lowandspeed_maxedsignals on the player are emitted but not consumed anywhere in this PR; either wire them into the HUD/other systems or remove them until they have a concrete use to avoid dead APIs. - Some tests (e.g.,
test_flameout_resets_speed_and_emits_signal) reach into private backing fields likeGlobals.settings._current_fuel, which tightly couples tests to implementation details; consider exposing a small public API or helper on the resource for setting up these states instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several comments in the new scripts (e.g., `NEW FIX`, long banner comments in `hud.gd`, etc.) read like change logs rather than maintenance documentation; consider trimming or rephrasing them to focus on current behavior to keep the codebase concise and easier to scan.
- The new `speed_low` and `speed_maxed` signals on the player are emitted but not consumed anywhere in this PR; either wire them into the HUD/other systems or remove them until they have a concrete use to avoid dead APIs.
- Some tests (e.g., `test_flameout_resets_speed_and_emits_signal`) reach into private backing fields like `Globals.settings._current_fuel`, which tightly couples tests to implementation details; consider exposing a small public API or helper on the resource for setting up these states instead.
## Individual Comments
### Comment 1
<location path="scripts/hud.gd" line_range="176-185" />
<code_context>
+## @param setting_name: The name of the property that was modified.
+## @param _new_value: The updated value of the property (unused directly here).
+## @return: void
+func _on_setting_changed(setting_name: String, _new_value: Variant) -> void:
+ if not is_instance_valid(_settings):
+ return
+
+ if (
+ setting_name
+ in [
+ "current_fuel",
+ "max_fuel",
+ "high_fuel_threshold",
+ "medium_fuel_threshold",
+ "low_fuel_threshold",
+ "no_fuel_threshold"
+ ]
+ ):
+ if setting_name == "max_fuel":
+ fuel_bar.max_value = _settings.max_fuel
+
+ update_fuel_bar()
+ check_fuel_warning()
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** HUD does not react to speed-related setting changes, so speed UI may be stale until the next speed signal.
`_on_setting_changed` currently only responds to fuel-related properties. If `max_speed`, `min_speed`, or the yellow/red fractions are changed while speed is constant, the HUD thresholds and `speed_bar.max_value` won’t update until the next `speed_changed` signal. If runtime tuning is intended, consider handling speed-related settings here as well so the HUD updates immediately even when speed doesn’t change.
Suggested implementation:
```
## Observer pattern callback to react to updates from the global settings resource.
## @param setting_name: The name of the property that was modified.
## @param _new_value: The updated value of the property (unused directly here).
## @return: void
func _on_setting_changed(setting_name: String, _new_value: Variant) -> void:
if not is_instance_valid(_settings):
return
# React to both fuel- and speed-related settings so the HUD updates immediately,
# even when speed/fuel values themselves have not changed.
if (
setting_name
in [
# Fuel-related properties
"current_fuel",
"max_fuel",
"high_fuel_threshold",
"medium_fuel_threshold",
"low_fuel_threshold",
"no_fuel_threshold",
# Speed-related properties
"max_speed",
"min_speed"
]
):
match setting_name:
# --- Fuel-related updates ---
"max_fuel":
fuel_bar.max_value = _settings.max_fuel
update_fuel_bar()
check_fuel_warning()
"current_fuel", "high_fuel_threshold", "medium_fuel_threshold", "low_fuel_threshold", "no_fuel_threshold":
update_fuel_bar()
check_fuel_warning()
# --- Speed-related updates ---
"max_speed":
# Keep the HUD's max consistent with settings immediately.
speed_bar.max_value = _settings.max_speed
update_speed_bar()
check_speed_warning()
"min_speed":
# Thresholds or visual zones may depend on min_speed.
update_speed_bar()
check_speed_warning()
```
If your speed warning thresholds are controlled by additional settings (such as yellow/red fractions or multiple speed threshold properties), you should also:
1. Add those property names to the `in [ ... ]` list in `_on_setting_changed`.
2. Include them in the appropriate `match` branch (most likely alongside `"min_speed"`), so that `update_speed_bar()` and `check_speed_warning()` are called when they change.
For example, if you have properties like `"speed_yellow_fraction"` or `"speed_red_fraction"`, they should be added to the list and mapped to the same `update_speed_bar()` / `check_speed_warning()` logic as `"min_speed"`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
Won't fix. |
Owner
Author
Won't fix. |
…anges, so speed UI may be stale until the next speed signal. suggestion (bug_risk): HUD does not react to speed-related setting changes, so speed UI may be stale until the next speed signal. _on_setting_changed currently only responds to fuel-related properties. If max_speed, min_speed, or the yellow/red fractions are changed while speed is constant, the HUD thresholds and speed_bar.max_value won’t update until the next speed_changed signal. If runtime tuning is intended, consider handling speed-related settings here as well so the HUD updates immediately even when speed doesn’t change.
Add several public accessor methods to scripts/hud.gd (get_current_speed, get_settings, get_fuel_bar_color, get_speed_bar_color, is_fuel_warning_active, is_speed_warning_active, is_speed_timer_running) to allow safe external inspection of HUD state for testing and encapsulation. Refactor test/gut/test_hud.gd to use these new accessors and to drive HUD behavior via public interfaces: emit Player speed_changed signals and modify Globals.settings for fuel changes instead of poking private HUD fields or calling internal methods. Tests now verify visual colors and blinker/timer state through the public API, improving encapsulation and making tests more realistic.
Owner
Author
|
@sourcery-ai guide |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
8 tasks
5 tasks
8 tasks
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.
To resolve issue #279, we need to decouple the UI speed updates from the _physics_process loop. By introducing a speed_changed signal, the UI will only update when the speed value actually changes, rather than calculating warnings and bar sizes every single frame.
Introduce a new speed_changed(float) signal in player.gd and hook it up to update UI and warnings. Connect/disconnect the handler in _ready/_exit_tree to avoid dangling connections, and emit the signal when speed actually changes (in _physics_process and on flameout). Also add guards for _settings validity and ensure flameout resets speed and notifies listeners. Add comprehensive GUT tests (test_player_fuel_logic.gd, test_player_movement_signals.gd and their uid files) that validate fuel behavior, rotor/timer reactions, lateral movement constraints, signal emission semantics, UI reactivity, and speed clamping.
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
Decouple player speed and fuel UI from the player controller by introducing observer-based signals and a dedicated HUD script, and extend tests and settings to validate the new behavior.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
speed_changedsignal inplayer.gdand created a dedicated, standalonehud.gdscript that relies entirely on the Observer Pattern to manage progress bars and warning blinkers natively.GameSettingsResource._exit_treecleanups to prevent dangling signal connections and memory leaks. Added robust invariant enforcements in the resource setters (ensuringmin_speedcannot exceedmax_speed, etc.) and built safe fallbacks to handle race conditions ifGlobals.settingsfails to load. Refined lateral movement to lock when the plane runs out of fuel.test_player_fuel_logic.gd,test_player_movement_signals.gd,test_hud.gd, andtest_player.gd). Built reusable static mock scene builders to perfectly isolate UI rendering, signal emission semantics, engine state changes, and speed clamping without needing to load the full game scene.Here is the summary of the bot and AI contributions to pull request #542:
@sourcery-ai:
hud.gdto prevent tests from relying on private fields, and ensuring that signals are properly disconnected insetup_hudto avoid dangling connections.@coderabbitai:
@deepsource-io: