Conversation
…eed #543 Implement a dynamic ParallaxBackground system that automatically adjusts its scrolling speed based on the player's current forward velocity.
5 tasks
Contributor
Reviewer's GuideIntroduces a dedicated ParallaxManager to own parallax scroll math and wiring, refactors main_scene to inject settings and connect player speed signals, and simplifies player speed state from a Dictionary to a scalar property with updated tests across GUT and GDUnit4. Sequence diagram for parallax background wiring and runtime updatessequenceDiagram
actor Player
participant MainScene
participant ParallaxManager
participant GameSettingsResource as Settings
participant Globals
participant Engine as GodotEngine
MainScene->>Globals: is_instance_valid(Globals)
alt Globals valid
MainScene->>Globals: settings
Globals-->>MainScene: Settings
else Globals invalid
MainScene-->>MainScene: settings_res = null
end
MainScene->>ParallaxManager: setup(settings_res)
alt settings_res valid
ParallaxManager->>Settings: connect(setting_changed, _on_setting_changed)
ParallaxManager->>Settings: connect(fuel_depleted, _on_fuel_depleted)
Settings-->>ParallaxManager: emit setting_changed(difficulty, value)
ParallaxManager-->>ParallaxManager: _difficulty updated
Settings-->>ParallaxManager: emit setting_changed(current_fuel, value)
ParallaxManager-->>ParallaxManager: _out_of_fuel updated
else settings_res invalid
ParallaxManager-->>ParallaxManager: return without wiring
end
MainScene->>Player: connect(speed_changed, ParallaxManager.update_speed)
MainScene->>ParallaxManager: prime_speed(player.current_speed)
MainScene->>ParallaxManager: auto_calculate_wrap_period()
loop Every_frame
Engine->>Player: _physics_process(delta)
Player-->>Player: _set_speed(target_speed)
Player-->>Player: current_speed updated
Player-->>Player: emit speed_changed(current_speed, max_speed)
Player->>ParallaxManager: speed_changed(current_speed, max_speed)
ParallaxManager-->>ParallaxManager: update_speed(new_speed)
Engine->>ParallaxManager: _process(delta)
alt _out_of_fuel true
ParallaxManager-->>ParallaxManager: scroll_offset = Vector2.ZERO
else _out_of_fuel false
ParallaxManager-->>ParallaxManager: scroll_offset.y += current_speed * delta * _difficulty * SCROLL_MULTIPLIER
alt wrap_period > 0
ParallaxManager-->>ParallaxManager: scroll_offset.y = wrapf(scroll_offset.y, 0, wrap_period)
end
end
end
Settings-->>ParallaxManager: emit fuel_depleted()
ParallaxManager-->>ParallaxManager: _on_fuel_depleted() -> _out_of_fuel = true, scroll_offset = Vector2.ZERO
Class diagram for Player, ParallaxManager, and GameSettingsResourceclassDiagram
class Player {
+float current_speed
-GameSettingsResource _settings
+signal speed_changed(new_speed: float, max_speed: float)
+signal speed_maxed()
+signal speed_low(low_threshold: float)
+func _set_speed(target_speed: float) void
+func _physics_process(delta: float) void
+func _on_fuel_timer_timeout() void
}
class ParallaxManager {
<<ParallaxBackground>>
+const float SCROLL_MULTIPLIER
+float wrap_period
-float _current_speed
-float _difficulty
-bool _out_of_fuel
+func setup(settings: GameSettingsResource) void
+func prime_speed(initial_speed: float) void
-func _gcd(a: int, b: int) int
-func _lcm(a: int, b: int) int
+func auto_calculate_wrap_period() void
+func update_speed(new_speed: float, max_speed: float) void
-func _on_setting_changed(setting_name: String, new_value: Variant) void
-func _on_fuel_depleted() void
+func _process(delta: float) void
}
class GameSettingsResource {
+float difficulty
+float current_fuel
+float min_speed
+float max_speed
+float low_yellow_fraction
+float base_consumption_rate
+float lateral_speed
+signal setting_changed(setting_name: String, new_value: Variant)
+signal fuel_depleted()
}
Player --> GameSettingsResource : uses
ParallaxManager --> GameSettingsResource : injected_via_setup
Player ..> ParallaxManager : emits speed_changed
GameSettingsResource ..> ParallaxManager : emits setting_changed
GameSettingsResource ..> ParallaxManager : emits fuel_depleted
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughParallax scrolling logic was moved into a new Changes
Sequence Diagram(s)sequenceDiagram
participant Main as MainScene
participant Player as Player
participant Parallax as ParallaxManager
participant Globals as Globals.settings
Main->>Parallax: attach script to Background (scene load)
Main->>Parallax: setup(Globals.settings)
Main->>Player: connect player.speed_changed -> Parallax.update_speed
Player-->>Parallax: speed_changed(new_speed, max_speed)
Parallax->>Parallax: update _current_speed (prime/update)
loop every frame
Parallax->>Globals: read difficulty, current_fuel
alt current_fuel <= 0
Parallax-->>Parallax: set scroll_offset = Vector2.ZERO
else
Parallax-->>Parallax: scroll_offset.y += _current_speed * delta * difficulty * SCROLL_MULTIPLIER
alt wrap_period > 0
Parallax-->>Parallax: scroll_offset.y = wrapf(scroll_offset.y, 0, wrap_period)
end
end
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 19, 2026 2:42a.m. | Review ↗ |
| JavaScript | |
Apr 19, 2026 2:42a.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 found 2 issues, and left some high level feedback:
- The new ParallaxManager drops the previous
current_fuel <= 0handling that resetscroll_offset, so if that behavior is still desired you should migrate that logic into the manager instead of just commenting it out inmain_scene.gd. - ParallaxManager pulls difficulty from the global
Globals.settingseach frame, which introduces tight coupling and repeated lookups; consider injecting difficulty (or settings) via a property or signal from the main scene instead. - In
_readyyou callbackground._on_player_speed_changed(player.speed["speed"], 0.0)to prime the parallax, but the second argument is always0.0and unused—either remove the unused parameter from the signal/handler or pass the actual max speed if it is intended to be meaningful.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new ParallaxManager drops the previous `current_fuel <= 0` handling that reset `scroll_offset`, so if that behavior is still desired you should migrate that logic into the manager instead of just commenting it out in `main_scene.gd`.
- ParallaxManager pulls difficulty from the global `Globals.settings` each frame, which introduces tight coupling and repeated lookups; consider injecting difficulty (or settings) via a property or signal from the main scene instead.
- In `_ready` you call `background._on_player_speed_changed(player.speed["speed"], 0.0)` to prime the parallax, but the second argument is always `0.0` and unused—either remove the unused parameter from the signal/handler or pass the actual max speed if it is intended to be meaningful.
## Individual Comments
### Comment 1
<location path="scripts/main_scene.gd" line_range="55-59" />
<code_context>
# Setup decor layer with random instances
setup_decor_layer(viewport_size)
+ # Wire up the signal architecture for the parallax background
+ if player.has_signal("speed_changed") and background.has_method("_on_player_speed_changed"):
+ player.speed_changed.connect(background._on_player_speed_changed)
+ # Prime the background with the player's initial starting speed
+ background._on_player_speed_changed(player.speed["speed"], 0.0)
+
</code_context>
<issue_to_address>
**suggestion:** Consider handling the case where the signal or method is missing more explicitly.
If `player` lacks `speed_changed` or `background` lacks `_on_player_speed_changed`, the wiring is skipped and scrolling never happens, with no visible signal of the problem. Consider adding a clear fallback (e.g., reuse the previous `_process`-based scrolling) or at least a `push_warning` so misconfigurations are easier to detect and debug.
```suggestion
# Wire up the signal architecture for the parallax background
if player.has_signal("speed_changed") and background.has_method("_on_player_speed_changed"):
player.speed_changed.connect(background._on_player_speed_changed)
# Prime the background with the player's initial starting speed
background._on_player_speed_changed(player.speed["speed"], 0.0)
elif not player.has_signal("speed_changed"):
push_warning("Parallax background not wired: player is missing the `speed_changed` signal. "
+ "Verify that the Player node defines and emits `speed_changed`.")
elif not background.has_method("_on_player_speed_changed"):
push_warning("Parallax background not wired: background is missing `_on_player_speed_changed` method. "
+ "Ensure the background script implements `_on_player_speed_changed(speed: float, delta: float)`.")
```
</issue_to_address>
### Comment 2
<location path="scripts/main_scene.gd" line_range="236-241" />
<code_context>
# Use the safe local reference for difficulty
- var scroll_speed: float = player.speed["speed"] * delta * settings_res.difficulty * 0.8
- background.scroll_offset.y += scroll_speed
+ # var scroll_speed: float = player.speed["speed"] * delta * settings_res.difficulty * 0.8
+ # background.scroll_offset.y += scroll_speed
# Use the safe local reference for current_fuel
- if settings_res.current_fuel <= 0:
- background.scroll_offset = Vector2(0, 0)
+ # if settings_res.current_fuel <= 0:
+ # background.scroll_offset = Vector2(0, 0)
# 1. Critical unbound controls warning (shown ONCE per session)
</code_context>
<issue_to_address>
**question (bug_risk):** Previous fuel-depletion behavior for the background is now disabled; ensure this gameplay change is intentional.
This used to reset `background.scroll_offset` to `(0, 0)` when `settings_res.current_fuel <= 0`, giving a clear visual end-of-run cue. With the scroll now handled by `ParallaxManager` and this check commented out, the background will keep scrolling from its last offset. If that cue is still desired, consider adding an equivalent fuel check in `ParallaxManager` (or signalling it when fuel hits zero) to restore the previous behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/main_scene.gd (1)
55-60: Guard against duplicate signal connections and avoid invoking a private-convention method from outside.Two minor concerns:
player.speed_changed.connect(background._on_player_speed_changed)has nois_connectedguard. If_ready()is ever executed again (scene reload, tests), you'll get a "Signal already connected" error. Consider usingCONNECT_REFERENCE_COUNTEDor guarding withis_connected.- Methods prefixed with
_are a GDScript convention for "private/virtual". Callingbackground._on_player_speed_changed(...)from another script leaks that convention. Consider exposing a publicprime(initial_speed: float)(or similar) method onParallaxManagerthat sets_current_speedinternally — the signal handler stays private.Proposed fix
- if player.has_signal("speed_changed") and background.has_method("_on_player_speed_changed"): - player.speed_changed.connect(background._on_player_speed_changed) - # Prime the background with the player's initial starting speed - background._on_player_speed_changed(player.speed["speed"], 0.0) + if player.has_signal("speed_changed") and background.has_method("_on_player_speed_changed"): + if not player.speed_changed.is_connected(background._on_player_speed_changed): + player.speed_changed.connect(background._on_player_speed_changed) + # Prime the background with the player's initial starting speed + if background.has_method("prime"): + background.prime(player.speed["speed"]) + else: + background._on_player_speed_changed(player.speed["speed"], 0.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/main_scene.gd` around lines 55 - 60, Guard the signal connection to avoid duplicate connects and stop calling a private method directly: before calling player.speed_changed.connect(background._on_player_speed_changed) check if player.speed_changed.is_connected(background, "_on_player_speed_changed") (or connect with CONNECT_REFERENCE_COUNTED) to prevent "Signal already connected" errors; and instead of invoking background._on_player_speed_changed(...) from this script, add a public method on the ParallaxManager like prime(initial_speed: float) that sets the internal _current_speed and call background.prime(player.speed["speed"]) so the private handler _on_player_speed_changed remains internal.scripts/parallax_manager.gd (2)
29-30:is_instance_valid(Globals)on an autoload singleton is always true.
Globalsis a Godot autoload, so the engine guarantees it exists for the lifetime of the scene tree. The check adds no safety and just makes the intent less clear — simplifying to only guardGlobals.settings(which can legitimately be null during transitions per the pattern inscripts/player.gd:50-57) would be cleaner.Proposed refactor
- if is_instance_valid(Globals) and is_instance_valid(Globals.settings): - difficulty = Globals.settings.difficulty + if is_instance_valid(Globals.settings): + difficulty = Globals.settings.difficulty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/parallax_manager.gd` around lines 29 - 30, The check `is_instance_valid(Globals)` is redundant for an autoload singleton; remove it and only guard access to `Globals.settings` so `difficulty = Globals.settings.difficulty` runs only when `Globals.settings` is non-null. Update the conditional around the assignment (where `difficulty` is set) to `if is_instance_valid(Globals.settings):` (or equivalent null check) to match the pattern used around `Globals.settings` elsewhere (e.g., in the player code) and keep intent clear.
32-33:scroll_offset.ygrows unboundedly — consider wrapping to preserve float precision over long sessions.
scroll_offset.y += scroll_amountaccumulates indefinitely. At ~250 px/s × 0.8 × difficulty, the value can reach millions after extended play, at which point float32 precision degrades visibly (jitter/stutter in the parallax scroll). Since theParallaxLayerchildren usemotion_mirroringto tile, the offset can safely be taken modulo the mirroring period.- var scroll_amount: float = _current_speed * delta * difficulty * 0.8 - scroll_offset.y += scroll_amount + var scroll_amount: float = _current_speed * delta * difficulty * 0.8 + scroll_offset.y = fposmod(scroll_offset.y + scroll_amount, 100000.0)Pick a wrap value that's a common multiple of your layers'
motion_mirroring.yperiods so visible tiling isn't disrupted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/parallax_manager.gd` around lines 32 - 33, The scroll_offset.y accumulation in the parallax update (using _current_speed, delta, difficulty -> scroll_amount) must be wrapped to avoid float32 precision loss; replace the unbounded increment of scroll_offset.y with a modulo/wrap using a chosen wrap_period (a common multiple of your ParallaxLayer.motion_mirroring.y values) and set scroll_offset.y = positive_mod(scroll_offset.y + scroll_amount, wrap_period) (use GDScript fposmod or equivalent) so tiling stays correct and the value stays small.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/main_scene.gd`:
- Around line 55-60: Guard the signal connection to avoid duplicate connects and
stop calling a private method directly: before calling
player.speed_changed.connect(background._on_player_speed_changed) check if
player.speed_changed.is_connected(background, "_on_player_speed_changed") (or
connect with CONNECT_REFERENCE_COUNTED) to prevent "Signal already connected"
errors; and instead of invoking background._on_player_speed_changed(...) from
this script, add a public method on the ParallaxManager like
prime(initial_speed: float) that sets the internal _current_speed and call
background.prime(player.speed["speed"]) so the private handler
_on_player_speed_changed remains internal.
In `@scripts/parallax_manager.gd`:
- Around line 29-30: The check `is_instance_valid(Globals)` is redundant for an
autoload singleton; remove it and only guard access to `Globals.settings` so
`difficulty = Globals.settings.difficulty` runs only when `Globals.settings` is
non-null. Update the conditional around the assignment (where `difficulty` is
set) to `if is_instance_valid(Globals.settings):` (or equivalent null check) to
match the pattern used around `Globals.settings` elsewhere (e.g., in the player
code) and keep intent clear.
- Around line 32-33: The scroll_offset.y accumulation in the parallax update
(using _current_speed, delta, difficulty -> scroll_amount) must be wrapped to
avoid float32 precision loss; replace the unbounded increment of scroll_offset.y
with a modulo/wrap using a chosen wrap_period (a common multiple of your
ParallaxLayer.motion_mirroring.y values) and set scroll_offset.y =
positive_mod(scroll_offset.y + scroll_amount, wrap_period) (use GDScript fposmod
or equivalent) so tiling stays correct and the value stays small.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 01b56c05-7c47-4a98-824f-3671cdd53b90
📒 Files selected for processing (4)
scenes/main_scene.tscnscripts/main_scene.gdscripts/parallax_manager.gdscripts/parallax_manager.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 (1)
📚 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/main_scene.gd
🪛 GitHub Actions: Pull Request Pipeline
scripts/main_scene.gd
[error] 226-226: gdlint reported: unused function argument 'delta' (unused-argument).
🔇 Additional comments (3)
scripts/parallax_manager.gd.uid (1)
1-1: LGTM — UID metadata file for the new script.scenes/main_scene.tscn (1)
80-80: LGTM — external resource added and correctly assigned to theBackground(ParallaxBackground) node viascript = ExtResource("76_qj6t7"). Matches the newParallaxManagerclass which extendsParallaxBackground.Also applies to: 207-208
scripts/main_scene.gd (1)
226-242: The lint issue and dead code removal have already been completed in the current code. The_processmethod correctly uses_deltaas the parameter, and the commented-out lines (236-241) have been removed.Regarding the parallax behavior during fuel depletion: when fuel depletes,
_on_player_out_of_fuel()calls_set_speed(0.0), which emits thespeed_changedsignal with speed 0. The ParallaxManager receives this via its observer callback and sets_current_speed = 0.0, causing the background to freeze at its current position on the next frame. While this differs from the old behavior (which resetscroll_offsetto zero), it is the intended architectural design and not a problem.> Likely an incorrect or invalid review comment.
…ger.gd file. Here is a comprehensive GUT unit test suite for the new parallax_manager.gd file. This test file mirrors the high standards of your existing tests. It includes full static typing, proper setup/teardown encapsulation, file headers, and specific scenario testing (Observer updates, math calculations, and null safety).
Owner
Author
|
Working on:
To satisfy the PR reviewer's feedback while maintaining the clean architecture we established, we need to reintroduce the current_fuel <= 0 check into the ParallaxManager. Since we are already safely accessing Globals.settings inside the _process loop to grab the game's difficulty, we can cleanly grab the current_fuel state in that exact same block and enforce the Vector2.ZERO offset reset. |
The new ParallaxManager drops the previous current_fuel <= 0 handling that reset scroll_offset, so if that behavior is still desired you should migrate that logic into the manager instead of just commenting it out in main_scene.gd. To satisfy the PR reviewer's feedback while maintaining the clean architecture we established, we need to reintroduce the current_fuel <= 0 check into the ParallaxManager. Since we are already safely accessing Globals.settings inside the _process loop to grab the game's difficulty, we can cleanly grab the current_fuel state in that exact same block and enforce the Vector2.ZERO offset reset.
Owner
Author
|
Working on:
We can completely eliminate the _process polling by using Dependency Injection alongside the Observer pattern we already set up. main_scene.gd will pass the settings resource into the manager exactly once, and the manager will listen for the difficulty and fuel changes via signals. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_parallax_manager.gd`:
- Around line 20-21: The test attaches a ParallaxManager instance via
add_child_autofree(_parallax_manager), which lets Godot call its _process()
between setup and assertions and can change scroll_offset.y; disable automatic
processing on the node under test by calling set_process(false) (or
set_physics_process(false) if appropriate) on the _parallax_manager after
creation and before any assertions so only the manual _process(delta)
invocations in tests mutate state; locate the ParallaxManager.new() /
add_child_autofree(_parallax_manager) lines and add a call to
_parallax_manager.set_process(false) (and set_physics_process(false) if needed)
immediately after adding the child.
- Around line 15-128: Tests currently rely on an implicit default for
GameSettingsResource.current_fuel which couples unrelated tests to fuel state;
explicitly set Globals.settings.current_fuel to a positive value (e.g., 100.0)
in before_each or at the start of tests that expect normal scrolling
(referencing GameSettingsResource and ParallaxManager instances used in
before_each, and the methods _process and _on_player_speed_changed and
properties _current_speed and scroll_offset), and add a new dedicated test
(e.g., test_flameout_resets_scroll) that sets Globals.settings.current_fuel to
0.0 (or a negative value), calls ParallaxManager._process(delta) and asserts
that ParallaxManager.scroll_offset == Vector2.ZERO to cover the fuel-depletion
reset behavior; also update test_scroll_offset_math and
test_zero_speed_stops_scroll to explicitly set current_fuel > 0 before
exercising _process so they no longer depend on the resource default.
🪄 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: 0d0f3e1b-74d7-47f6-8abe-311ff9df751e
📒 Files selected for processing (4)
scripts/main_scene.gdscripts/parallax_manager.gdtest/gut/test_parallax_manager.gdtest/gut/test_parallax_manager.gd.uid
✅ Files skipped from review due to trivial changes (1)
- test/gut/test_parallax_manager.gd.uid
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/main_scene.gd
- scripts/parallax_manager.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). (2)
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: GDUnit4 Unit Tests / unit-test
🧰 Additional context used
🧠 Learnings (1)
📚 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_parallax_manager.gd
…ion alongside the Observer pattern ParallaxManager pulls difficulty from the global Globals.settings each frame, which introduces tight coupling and repeated lookups; consider injecting difficulty (or settings) via a property or signal from the main scene instead. We can completely eliminate the _process polling by using Dependency Injection alongside the Observer pattern we already set up. main_scene.gd will pass the settings resource into the manager exactly once, and the manager will listen for the difficulty and fuel changes via signals.
…epend on current_fuel's default. The whole point of migrating logic into ParallaxManager (per the PR discussion) was to preserve the current_fuel <= 0 → scroll_offset = Vector2.ZERO behavior — but there is no test asserting this flameout reset. In addition, before_each() constructs a fresh GameSettingsResource without ever setting current_fuel, so: test_scroll_offset_math only passes if GameSettingsResource.current_fuel happens to default to > 0. If the default is ever changed to 0.0 (common for "fuel to be filled at game start"), the production code would hit the current_fuel <= 0 branch, reset scroll_offset to Vector2.ZERO, and the 80.0 expectation would fail — for reasons unrelated to the math being tested. test_zero_speed_stops_scroll is similarly coupled: with a fuel default of 0, scroll_offset.y would be reset to 0 rather than staying at 125.5, producing a misleading failure.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Owner
Author
|
Working on:
The reviewer caught a great detail. We have two options here, but only one is architecturally safe:
|
In _ready you call background._on_player_speed_changed(player.speed["speed"], 0.0) to prime the parallax, but the second argument is always 0.0 and unused—either remove the unused parameter from the signal/handler or pass the actual max speed if it is intended to be meaningful. The reviewer caught a great detail. We have two options here, but only one is architecturally safe: Remove the parameter: We cannot do this. The speed_changed(new_speed, max_speed) signal is broadcast by player.gd, and the HUD relies heavily on that max_speed parameter to calculate the color lerping (Green -> Yellow -> Red) for the UI progress bars. If we change the signal signature, the HUD breaks. Pass the actual max speed: This is the correct approach. Since parallax_manager.gd connects to that signal, it must accept both parameters to match the signature (even if it only uses the first one). Therefore, when we manually prime it in main_scene.gd, we should pass the real max_speed instead of a lazy 0.0.
The Encapsulation Leak: Calling _on_player_speed_changed from main_scene.gd forces the main scene to "pretend" to be a signal emitter, which is dirty architecture. Creating a public prime_speed() method fixes this, and as a bonus, it means we don't have to clumsily pass the max_speed parameter just to satisfy the signal signature.
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider exposing a public signal or method on
ParallaxManagerinstead of wiring the player directly to the internal_on_player_speed_changedcallback frommain_scene.gd, so callers don’t need knowledge of an underscored/private-style handler signature. - The wrap period computation in
main_scene.gd(based onbushes_layer.motion_scale.yandviewport_size) might be better encapsulated insideParallaxManageror driven by a configuration value, so main_scene doesn’t need to know about the specific parallax layer structure. - When priming the manager with
player.speed["speed"]inmain_scene.gd, consider exposing a typed property or getter on the player instead of pulling from a dictionary key to avoid silent runtime errors if the key or data structure changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider exposing a public signal or method on `ParallaxManager` instead of wiring the player directly to the internal `_on_player_speed_changed` callback from `main_scene.gd`, so callers don’t need knowledge of an underscored/private-style handler signature.
- The wrap period computation in `main_scene.gd` (based on `bushes_layer.motion_scale.y` and `viewport_size`) might be better encapsulated inside `ParallaxManager` or driven by a configuration value, so main_scene doesn’t need to know about the specific parallax layer structure.
- When priming the manager with `player.speed["speed"]` in `main_scene.gd`, consider exposing a typed property or getter on the player instead of pulling from a dictionary key to avoid silent runtime errors if the key or data structure changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ic API for the Parallax Manager! This is another excellent catch by your reviewer, focusing on encapsulation and API design. In GDScript, methods starting with an underscore (like _on_player_speed_changed) are universally understood as "private" or "internal" methods. When main_scene.gd connects directly to a private method on background, it is breaking encapsulation—it "knows too much" about how the background works internally. By replacing the private handler with a public update_speed method, we create a clean, public API that main_scene.gd is allowed to use.
Owner
Author
This is another excellent catch by the reviewer, focusing on encapsulation and API design. In GDScript, methods starting with an underscore (like _on_player_speed_changed) are universally understood as "private" or "internal" methods. When main_scene.gd connects directly to a private method on background, it is breaking encapsulation—it "knows too much" about how the background works internally. By replacing the private handler with a public update_speed method, we create a clean, public API that main_scene.gd is allowed to use. |
Owner
Author
This is a fantastic architectural critique from the reviewer. It touches on the principle of Encapsulation. Right now, main_scene.gd is reaching into the background, grabbing a specific layer (bushes_layer), extracting its properties, doing the math, and handing the answer back to the background. main_scene.gd is "knowing too much" about how the background is structured. The most elegant way to solve this is to let the ParallaxManager inspect its own children and calculate the maximum wrap period automatically. main_scene.gd just needs to say: "Hey, I'm done setting up the layers. Please calculate your wrap limits now." |
…ximum wrap period automatically. The wrap period computation in main_scene.gd (based on bushes_layer.motion_scale.y and viewport_size) might be better encapsulated inside ParallaxManager or driven by a configuration value, so main_scene doesn’t need to know about the specific parallax layer structure. This is a fantastic architectural critique from the reviewer. It touches on the principle of Encapsulation. Right now, main_scene.gd is reaching into the background, grabbing a specific layer (bushes_layer), extracting its properties, doing the math, and handing the answer back to the background. main_scene.gd is "knowing too much" about how the background is structured. The most elegant way to solve this is to let the ParallaxManager inspect its own children and calculate the maximum wrap period automatically. main_scene.gd just needs to say: "Hey, I'm done setting up the layers. Please calculate your wrap limits now." Why this is the perfect solution: Zero Coupling: main_scene.gd no longer knows what a motion_scale is, nor does it care. Future Proof: If you add 5 more parallax layers tomorrow with totally different speeds, you won't have to change any math. The ParallaxManager will automatically scan them all, find the one that requires the longest wrap period, and apply it seamlessly!
When priming the manager with player.speed["speed"] in main_scene.gd, consider exposing a typed property or getter on the player instead of pulling from a dictionary key to avoid silent runtime errors if the key or data structure changes. This is an excellent catch by the reviewer. Using a Dictionary (speed["speed"]) for a single numerical value is a "code smell" known as stringly-typed data. It defeats Godot's static typing system. If someone accidentally renames the key or drops the dictionary, the compiler won't warn you, but the game will crash at runtime. By refactoring this into a strongly typed float property (e.g., current_speed: float = 250.0), we gain full autocomplete, compiler safety, and we protect main_scene.gd from silent errors.
Owner
Author
This is an excellent catch by the reviewer. Using a Dictionary (speed["speed"]) for a single numerical value is a "code smell" known as stringly-typed data. It defeats Godot's static typing system. If someone accidentally renames the key or drops the dictionary, the compiler won't warn you, but the game will crash at runtime. By refactoring this into a strongly typed float property (e.g., current_speed: float = 250.0), we gain full autocomplete, compiler safety, and we protect main_scene.gd from silent errors. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/parallax_manager.gd (2)
51-65:auto_calculate_wrap_period()silently disables wrap when no layers are found.If
get_children()contains noParallaxLayers (or all of them have zeromotion_scale.y/motion_mirroring.y),max_periodstays0.0and Line 65 overwrites the exportedwrap_period, silently disabling the float-precision safeguard — even when a caller configured it via the inspector. Guard the assignment:♻️ Proposed fix
- wrap_period = max_period + if max_period > 0.0: + wrap_period = max_period + else: + push_warning( + "ParallaxManager.auto_calculate_wrap_period: no valid ParallaxLayer children; " + + "keeping existing wrap_period=%s." % wrap_period + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/parallax_manager.gd` around lines 51 - 65, auto_calculate_wrap_period currently sets wrap_period to 0.0 if no ParallaxLayer children (or all have zero motion_scale.y/motion_mirroring.y), silently overwriting an inspector-configured value; change the function (auto_calculate_wrap_period) to only assign wrap_period when max_period > 0.0 (i.e., after iterating get_children() and computing period from each ParallaxLayer using motion_scale.y and motion_mirroring.y), otherwise leave the existing exported wrap_period untouched so the inspector/default precision safeguard is preserved.
51-65: Optional: max-of-periods is only safe when layer periods are commensurate.
wrap_periodneeds to be a common multiple of every layer'smotion_mirroring.y / motion_scale.yfor all layers to wrap seamlessly. Picking the maximum works only if the other layers' periods evenly divide it. Inmain_scene.gd, the sand layer usesmotion_mirroring.y = 2*tiles_y*tex_size.ywhilebushes_layer/decor_layerusemotion_mirroring.y = viewport.y * parallax_screens_tall, so the periods are generally non-commensurate and a reset atmaxwill cause a visible jump on the shorter-period layer. Consider computing an LCM (over integer-quantized periods) instead, or padmax_periodto an integer multiple of each layer's period.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/parallax_manager.gd` around lines 51 - 65, The current auto_calculate_wrap_period() sets wrap_period to the maximum period which can cause visible jumps when layer periods are non-commensurate; instead compute a common multiple of every layer's period (period = motion_mirroring.y / motion_scale.y) by quantizing periods to integers (choose a precision multiplier, e.g. factor = 1000), convert each period to an integer n = round(period * factor), compute the integer LCM across all n values, then set wrap_period = lcm / factor; update auto_calculate_wrap_period() to iterate ParallaxLayer children, collect quantized periods, compute LCM, and fall back to max_period if LCM would overflow or no layers found.
🤖 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_parallax_manager.gd`:
- Around line 198-199: The test creates an uninitialized ParallaxManager
instance (uninitialized_manager) and adds it via add_child_autofree without
disabling its automatic processing, allowing Godot to run _process() and mutate
scroll_offset.y; call set_process(false) on uninitialized_manager immediately
after construction (before add_child_autofree) to match the guard used for
_parallax_manager so the test's manual process invocation and subsequent
assertions remain deterministic.
---
Nitpick comments:
In `@scripts/parallax_manager.gd`:
- Around line 51-65: auto_calculate_wrap_period currently sets wrap_period to
0.0 if no ParallaxLayer children (or all have zero
motion_scale.y/motion_mirroring.y), silently overwriting an inspector-configured
value; change the function (auto_calculate_wrap_period) to only assign
wrap_period when max_period > 0.0 (i.e., after iterating get_children() and
computing period from each ParallaxLayer using motion_scale.y and
motion_mirroring.y), otherwise leave the existing exported wrap_period untouched
so the inspector/default precision safeguard is preserved.
- Around line 51-65: The current auto_calculate_wrap_period() sets wrap_period
to the maximum period which can cause visible jumps when layer periods are
non-commensurate; instead compute a common multiple of every layer's period
(period = motion_mirroring.y / motion_scale.y) by quantizing periods to integers
(choose a precision multiplier, e.g. factor = 1000), convert each period to an
integer n = round(period * factor), compute the integer LCM across all n values,
then set wrap_period = lcm / factor; update auto_calculate_wrap_period() to
iterate ParallaxLayer children, collect quantized periods, compute LCM, and fall
back to max_period if LCM would overflow or no layers found.
🪄 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: b568be64-b54d-441e-aba3-44bdd597f58b
📒 Files selected for processing (5)
scripts/main_scene.gdscripts/parallax_manager.gdscripts/player.gdtest/gut/test_parallax_manager.gdtest/gut/test_player_fuel_logic.gd
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 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/gut/test_player_fuel_logic.gdscripts/main_scene.gdscripts/parallax_manager.gdtest/gut/test_parallax_manager.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_fuel_logic.gdtest/gut/test_parallax_manager.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
🔇 Additional comments (4)
test/gut/test_player_fuel_logic.gd (1)
97-97: LGTM.Scalar
current_speedwrite matches the new field inscripts/player.gd(Line 33) and preserves the original test intent.scripts/player.gd (1)
30-33: LGTM on scalar speed migration.Replacing the
speed["speed"]dictionary with a typedcurrent_speed: floatis cleaner, avoids repeated dict lookups in the hot path (_physics_process,_on_fuel_timer_timeout), and preserves the 250.0 default. All downstream reads/writes (Lines 142, 146-148, 151-155, 163, 228, 244, 259) are consistently updated.scripts/main_scene.gd (1)
55-107: Parallax DI/observer wiring LGTM.The settings injection, duplicate-connect guard via
is_connected, publicprime_speed/update_speedAPI usage, and delegatedauto_calculate_wrap_periodall align with the ParallaxManager contract. Theis_instance_valid(Globals)guard (Line 62) keeps this path GUT-friendly, matching the pattern already used inscripts/player.gd(Line 54). Warning messages correctly distinguish missing-signal vs. missing-method failure modes.Minor nit (non-blocking): if both
speed_changedandupdate_speedare missing, only the firstelifwarning fires — acceptable since either message indicates breakage.test/gut/test_parallax_manager.gd (1)
105-158: Good coverage for flameout and recovery paths.
test_flameout_resets_offsetandtest_flameout_recovery_resumes_scrolldirectly exercise the_out_of_fuelstate machine including the_on_setting_changed("current_fuel", >0)clearing path and the subsequent scroll math — this closes the gap called out earlier about preserving thecurrent_fuel <= 0 → Vector2.ZEROcontract insideParallaxManager.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…r-speed' of https://github.com/ikostan/SkyLockAssault into implement-parallax-scrolling-background-linked-to-player-speed
…e found. 51-65: auto_calculate_wrap_period() silently disables wrap when no layers are found. If get_children() contains no ParallaxLayers (or all of them have zero motion_scale.y/motion_mirroring.y), max_period stays 0.0 and Line 65 overwrites the exported wrap_period, silently disabling the float-precision safeguard — even when a caller configured it via the inspector.
…urate. 51-65: Optional: max-of-periods is only safe when layer periods are commensurate. wrap_period needs to be a common multiple of every layer's motion_mirroring.y / motion_scale.y for all layers to wrap seamlessly. Picking the maximum works only if the other layers' periods evenly divide it. In main_scene.gd, the sand layer uses motion_mirroring.y = 2*tiles_y*tex_size.y while bushes_layer/decor_layer use motion_mirroring.y = viewport.y * parallax_screens_tall, so the periods are generally non-commensurate and a reset at max will cause a visible jump on the shorter-period layer. Consider computing an LCM (over integer-quantized periods) instead, or pad max_period to an integer multiple of each layer's period.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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:
- In
ParallaxManager.auto_calculate_wrap_period()you're rounding float periods to ints and computing an LCM; ifmotion_mirroring.y / motion_scale.yisn't an exact integer this can introduce subtle drift—consider either documenting the requirement that those ratios be integral or using a float-based approach with an epsilon instead of integer rounding. - Several tests reach into
_parallax_manager._current_speedand other underscored members; if these are intended to stay private, it may be more robust to expose minimal public accessors or to assert against observable behavior (e.g.,scroll_offset) to avoid coupling tests to internal implementation details.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ParallaxManager.auto_calculate_wrap_period()` you're rounding float periods to ints and computing an LCM; if `motion_mirroring.y / motion_scale.y` isn't an exact integer this can introduce subtle drift—consider either documenting the requirement that those ratios be integral or using a float-based approach with an epsilon instead of integer rounding.
- Several tests reach into `_parallax_manager._current_speed` and other underscored members; if these are intended to stay private, it may be more robust to expose minimal public accessors or to assert against observable behavior (e.g., `scroll_offset`) to avoid coupling tests to internal implementation details.
## Individual Comments
### Comment 1
<location path="scripts/main_scene.gd" line_range="58-63" />
<code_context>
+ # =========================================================
+ # DEPENDENCY INJECTION: Parallax Background
+ # =========================================================
+ # Safely extract settings once to use for both injection and priming.
+ # The is_instance_valid(Globals) guard prevents hard crashes during
+ # isolated GUT tests where Autoloads may not be fully initialized.
+ var settings_res: GameSettingsResource = (
+ Globals.settings if is_instance_valid(Globals) else null
+ )
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handle the case where `Globals` exists but `Globals.settings` is null or invalid.
As written, `settings_res` can reference `Globals.settings` even when the singleton exists but `Globals.settings` is null or freed, so `background.setup(settings_res)` may receive an invalid resource. Consider also checking `Globals.settings` itself (e.g., `is_instance_valid(Globals.settings)` or a null check) so `settings_res` is guaranteed to be either a valid `GameSettingsResource` or `null`.
```suggestion
# Safely extract settings once to use for both injection and priming.
# The is_instance_valid(Globals) guard prevents hard crashes during
# isolated GUT tests where Autoloads may not be fully initialized.
# Also guard against a null or freed Globals.settings so background.setup
# only ever receives a valid GameSettingsResource or null.
var settings_res: GameSettingsResource = (
Globals.settings
if is_instance_valid(Globals)
and Globals.settings != null
and is_instance_valid(Globals.settings)
else null
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
> * In `ParallaxManager.auto_calculate_wrap_period()` you're rounding float periods to ints and computing an LCM; if `motion_mirroring.y / motion_scale.y` isn't an exact integer this can introduce subtle drift—consider either documenting the requirement that those ratios be integral or using a float-based approach with an epsilon instead of integer rounding. This feedback is incredibly sharp. The reviewer is pointing out a "silent failure" that happens when math precision slowly degrades the visual quality of the game over time, known as "sub-pixel drift." Here is why it happens: Imagine one of your layers has a calculated period of 1000.5. By using roundi(), we force it to 1001. The LCM math works perfectly using 1001, but the actual graphic on the screen is still wrapping every 1000.5 pixels. Every time the background loops, that layer will visually "slip" by 0.5 pixels. After 10 loops, the graphic has drifted 5 full pixels out of alignment, causing noticeable stuttering or seams. In 2D game development, the best practice is to always use exact integers for texture mirroring to prevent artifacting anyway. Therefore, instead of rewriting the LCM math to use complex float-epsilons, we should do exactly what the reviewer suggested as their first option: document the requirement and throw a warning if the rule is broken.
Owner
Author
This feedback is incredibly sharp. The reviewer is pointing out a "silent failure" that happens when math precision slowly degrades the visual quality of the game over time, known as "sub-pixel drift." Here is why it happens: In 2D game development, the best practice is to always use exact integers for texture mirroring to prevent artifacting anyway. Therefore, instead of rewriting the LCM math to use complex float-epsilons, we should do exactly what the reviewer suggested as their first option: document the requirement and throw a warning if the rule is broken. |
Owner
Author
It is a "clean code" suggestion rather than a logic bug or a performance risk. Won't fix. |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
deleted the
implement-parallax-scrolling-background-linked-to-player-speed
branch
ikostan
changed the title
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.
This PR implements a comprehensive architectural overhaul of the player movement and parallax systems to improve maintainability, stability, and type safety. Key changes focus on decoupling systems through the Observer pattern and resolving long-term float-precision risks.
Summary of Changes
Architectural Improvements
ParallaxManagerand UI systems away from polling global singletons. They now react dynamically to signals (e.g.,speed_changed,fuel_depleted) emitted by the player.main_scene.gdto inject theGameSettingsResourceinto subordinates at initialization, reducing coupling to theGlobalsautoload and improving testability.ParallaxManager, hiding implementation details from theMainScene.Technical Safeguards
wrapfmechanism in the parallax loop to prevent "stuttering" during long-duration play sessions by resetting the scroll offset at a calculated period.motion_scalevalues wrap seamlessly without visual jumping.speed["speed"]) to a strictly typed float (current_speed), eliminating silent runtime errors.UI & Input
Testing & Quality Assurance
Contributors
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")
@ikostan Contributions Summary
current_speed) to ensure compile-time safety across the game engine.wrapfmethod, protecting the game's visual integrity during extended play sessions.main_scene.gdinitialization logic to use safe Dependency Injection, protecting the scene from crashes during isolated testing or scene transitions.This Pull Request (PR) #574 has benefited from significant contributions and automated reviews from several AI-driven tools and bots. Below is a summary of their contributions and their correctly formatted GitHub handles for inclusion in the contributors list.
Bots & AI Contribution Summary
Contributor List (GitHub Format)
To ensure these are properly recognized in your GitHub contributors list or
README.md:Summary by Sourcery
Introduce a dedicated ParallaxManager to drive background scrolling from the player’s speed signal, decoupling parallax logic from the main scene and player while preserving fuel-based behavior and precision safeguards.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit