Conversation
…eed #543 Implement a dynamic ParallaxBackground system that automatically adjusts its scrolling speed based on the player's current forward velocity.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…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).
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.
…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>
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.
The Connection Guard: If _ready() runs twice (e.g., moving nodes in and out of the tree, or in certain GUT testing environments), connect() will throw an error if it's already connected. Wrapping it in is_connected is a bulletproof safety net.
Globals is 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 guard Globals.settings (which can legitimately be null during transitions per the pattern in scripts/player.gd:50-57) would be cleaner. The reviewer is absolutely correct here. In Godot, an Autoload (like your Globals) is attached to the root of the SceneTree before any of your scenes even load, and it stays there until the application quits entirely. Because the engine guarantees its existence, checking is_instance_valid(Globals) is redundant "paranoid" coding. It clutters the script and makes it look like you expect the singleton to be destroyed. We only need to check if the property inside it (Globals.settings) is valid.
…at precision over long sessions. scroll_offset.y += scroll_amount accumulates 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 the ParallaxLayer children use motion_mirroring to tile, the offset can safely be taken modulo the mirroring period.
126-148: Nit: test name/assertion message is slightly misleading.
Since _process no longer reads Globals.settings at all (it uses cached _difficulty set by setup() in before_each), this test actually verifies that cached state still drives _process after globals are nulled — not that a 1.0 fallback kicks in. The math passes only because setup() happened to cache difficulty = 1.0 before the null. Consider renaming to something like test_process_safe_with_null_globals_after_setup and updating the comment/assertion so the intent ("doesn't crash / keeps using cached values") matches the actual behavior.
To truly exercise the _difficulty = 1.0 initial fallback path, you'd need to instantiate a fresh ParallaxManager without calling setup() and null globals before the first _process.
63-67: LGTM — refuel recovery path is correctly wired. The current_fuel > 0 branch clears _out_of_fuel, and the depletion transition is handled by the dedicated fuel_depleted signal emitted by game_settings_resource.gd (lines 147-148). No redundancy, and the observer chain avoids per-frame polling as intended by the PR. Minor suggestion for test coverage: there's currently no GUT test asserting that setting current_fuel back to a positive value after a flameout clears _out_of_fuel and allows scroll to resume on the next _process. Worth adding as a follow-up.
In main_scene.gd, _process(_delta) now only fetches Globals.settings and early-returns without using it; consider removing this method (or the unused settings_res lookup) entirely to avoid confusion and unnecessary work each frame. The reviewer has an eagle eye! They are absolutely right. That settings_res lookup inside main_scene.gd's _process loop is a leftover "ghost" from before we decoupled the parallax background. Back when main_scene.gd was handling the parallax scrolling directly, it needed the settings to check the fuel and difficulty every frame. Now that parallax_manager.gd handles all of that, main_scene.gd doesn't use settings_res in _process at all! Since we still need _process to check for the unbound controls warning, we shouldn't delete the whole function, but we should absolutely delete the useless settings_res fetch.
In parallax_manager.gd, the 0.8 scroll multiplier is a magic number used in _process; consider extracting it to a named constant (e.g., SCROLL_MULTIPLIER) so it is easier to tune and understand its purpose. The reviewer is bringing up a classic clean code principle here! "Magic numbers" (unexplained numbers floating in the middle of equations) make code harder to read and maintain. By extracting 0.8 into a named constant at the top of the script, anyone reading the file immediately knows what that number does, and if you ever need to tweak the background speed, you don't have to go hunting through the math in the _process loop to find it.
In main_scene.gd you now access Globals.settings directly in _ready() for parallax setup without the null/validity guard that existed in the old _process, which can reintroduce crashes during scene transitions or GUT tests; consider mirroring the previous is_instance_valid/null safety when grabbing settings_res. Yes, this is 100% valid feedback, and it highlights a very specific quirk about how Godot handles isolated unit testing! While it is true that Globals (as an Autoload) is supposed to exist for the entire lifetime of the game, GUT tests often instantiate scenes in a total vacuum. If a GUT test loads main_scene.gd directly without spinning up the full SceneTree and Autoloads, Globals will technically be a null instance. If your code says var settings_res: GameSettingsResource = Globals.settings, and Globals doesn't exist in that specific test, the engine will throw a fatal "Attempt to access property on a null instance" error and crash the test. To make your code bulletproof and match the exact safety pattern you already established in player.gd, we should restore that guard.
The global_bushes_period calculation in main_scene.gd uses a hard-coded 0.5 divisor; if this is tied to a specific layer’s motion_scale.y, it would be clearer and less fragile to derive it from that layer’s actual properties or a named constant rather than a magic number. This is another fantastic piece of feedback from your reviewer. It targets a concept called "Fragile Coupling." Right now, if you or another developer decide later to change the parallax scrolling speed to make the background move slower (e.g., changing the motion_scale to 0.2), you would have to remember to also scroll down to the bottom of the _ready() function and change that random 0.5 divisor. If you forget, the math breaks, and the background will glitch. By dynamically reading the motion_scale.y directly from the layer itself, the code becomes "self-healing." If the layer properties change, the math automatically adapts.
…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.
…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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Move the high-level game state and manager scripts out of the root folder into core/ and managers/.
The command assumes addons/gut/ already exists, but run_pipeline.sh does not install it. The existing run_gut_unit_tests.sh and .github/workflows/gut_tests.yml both include GUT installation logic that run_pipeline.sh lacks. Additionally, passing -gdir=res://test broadens test discovery beyond .gutconfig.json's configured res://test/gut/, so GUT may attempt to load unintended test suites (e.g., GDUnit4 tests under res://test/gdunit4/).
Move to managers/: audio_manager.gd, resource_preloader.gd.
RUN is a Dockerfile instruction and is not valid bash. Running this will cause a command not found error. It should be replaced with a conditional install bloc
This overrides the directory configured in .gutconfig.json (which points to res://test/gut/) and causes GUT to scan all subdirectories under res://test/, including GDUnit4 tests. These flags should be removed so -gconfig alone governs test discovery:
In run_pipeline.sh, removing the report.xml existence warning means a missing or failed pytest run will now fail silently; consider restoring a clear message or explicit failure if the report is absent to make pipeline debugging easier.
… the environment The npx --yes markdownlint-cli2@0.12.1 invocation couples the pipeline to an on-demand network install of a specific version on every run; consider using a locally installed tool (or caching) and/or moving the version pin to a central config so CI remains faster and less dependent on external availability.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
9-9: Remove or wire PW_TIMEOUT into the pytest command. Line 9 defines PW_TIMEOUT=10000, but the pytest command on line 96 doesn't use the --timeout parameter, so the variable has no effect. Either add --timeout=$PW_TIMEOUT to the pytest invocation or remove the unused variable.
Line 12 now renders as a standalone + bullet, which breaks the paragraph flow.
Remove Game Jolt from the manual phase or reclassify it consistently. The guide lists Game Jolt as 100% Auto on Line 79, but also includes it in the manual/occasional Phase 3 list on Line 101.
…tations issue (bug_risk): Clarify timeout units to match pytest-timeout expectations This value is now passed directly to pytest via --timeout=$PW_TIMEOUT, and pytest-timeout interprets it as seconds, not milliseconds. As written, 10000 is ~2.8 hours, not 10 seconds. Please either change the value to the intended number of seconds or clearly document that this timeout is in seconds to avoid accidental misconfiguration.
…y pipeline run for performance and reliability This step makes every CI run download and unzip GUT, adding avoidable network and I/O overhead and introducing a runtime dependency on GitHub being available. If GUT is a stable dependency, consider installing it in the Docker image (or caching it via a shared volume) so the pipeline avoids repeated downloads and file moves. To fully implement the suggestion: Update your Dockerfile (or base image build) to clone/unpack GUT into $PROJECT_DIR/addons/gut at image build time, so it is available for the pipeline without network access. If you are using a cache/volume approach instead, ensure that the cache populates $PROJECT_DIR/addons/gut before run_pipeline.sh executes. If there is (or will be) a command that actually runs the GUT tests (e.g. godot --headless ...), place it immediately after this existence check and wrap it with check_exit as done for the other steps.
…eton Migrate Core and Manager Scripts. Align CI pipeline and tests with new script layout, add GUT.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Reviewer's GuideRefactors parallax background scrolling into a dedicated ParallaxManager with DI and observer wiring, replaces the player speed dictionary with a typed current_speed property across code and tests, and hardens the CI pipeline, Docker image, and documentation around linting, testing, and deployment. Sequence diagram for parallax setup and runtime updatessequenceDiagram
participant MainScene
participant Globals
participant GameSettingsResource as Settings
participant Player
participant ParallaxManager as Background
MainScene->>Globals: read settings
alt Globals and settings valid
Globals-->>MainScene: GameSettingsResource
MainScene->>Background: setup(Settings)
activate Background
Background->>Background: cache difficulty and fuel
Background->>Settings: connect setting_changed
Background->>Settings: connect fuel_depleted
deactivate Background
else invalid or null
Globals-->>MainScene: null
MainScene->>Background: setup(null)
end
MainScene->>Background: prime_speed(Player.current_speed)
MainScene->>Background: auto_calculate_wrap_period()
Note over Player,Background: Runtime: player speed change
Player->>Player: _set_speed(target_speed)
Player->>Settings: read difficulty, min_speed, max_speed, current_fuel
Player->>Player: update current_speed (clamped)
alt speed changed
Player-->>Background: speed_changed(new_speed, max_speed)
Background->>Background: update_speed(new_speed, max_speed)
end
Note over Settings,Background: Runtime: difficulty or fuel change
Settings-->>Background: setting_changed(setting_name, new_value)
Background->>Background: _on_setting_changed(setting_name, new_value)
Settings-->>Background: fuel_depleted()
Background->>Background: _on_fuel_depleted()
Background->>Background: scroll_offset = Vector2.ZERO
loop each frame
Background->>Background: _process(delta)
alt out_of_fuel
Background->>Background: scroll_offset = Vector2.ZERO
else has fuel
Background->>Background: scroll_offset.y += current_speed * delta * difficulty * SCROLL_MULTIPLIER
alt wrap_period > 0
Background->>Background: scroll_offset.y = wrapf(scroll_offset.y, 0, wrap_period)
end
end
end
Class diagram for ParallaxManager and Player speed refactorclassDiagram
class Player {
+float current_speed
+GameSettingsResource _settings
+signal speed_changed(speed: float, max_speed: float)
+signal speed_maxed()
+signal speed_low(threshold: float)
+void _set_speed(target_speed: float)
+void _physics_process(delta: float)
+void _on_fuel_timer_timeout()
}
class GameSettingsResource {
+float difficulty
+float current_fuel
+float min_speed
+float max_speed
+float low_yellow_fraction
+float base_consumption_rate
+signal setting_changed(setting_name: String, new_value: Variant)
+signal fuel_depleted()
}
class ParallaxBackground {
+Vector2 scroll_offset
}
class ParallaxManager {
<<ParallaxBackground>>
+float SCROLL_MULTIPLIER
+float wrap_period
-float _current_speed
-float _difficulty
-bool _out_of_fuel
+void setup(settings: GameSettingsResource)
+void prime_speed(initial_speed: float)
+void auto_calculate_wrap_period()
+void update_speed(new_speed: float, max_speed: float)
-int _gcd(a: int, b: int)
-int _lcm(a: int, b: int)
-void _on_setting_changed(setting_name: String, new_value: Variant)
-void _on_fuel_depleted()
+void _process(delta: float)
}
class MainScene {
+Player player
+ParallaxManager background
+void _ready()
+void _process(delta: float)
+void setup_decor_layer(viewport: Vector2)
}
Player ..> GameSettingsResource : uses
Player ..> ParallaxManager : emits speed_changed
ParallaxManager --|> ParallaxBackground
ParallaxManager ..> GameSettingsResource : observes setting_changed
ParallaxManager ..> GameSettingsResource : observes fuel_depleted
MainScene ..> GameSettingsResource : reads via Globals.settings
MainScene *-- Player
MainScene *-- ParallaxManager
MainScene ..> ParallaxManager : setup, prime_speed, update_speed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Apr 21, 2026 2:31a.m. | Review ↗ |
| JavaScript | |
Apr 21, 2026 2:31a.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.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
![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:
- In
ParallaxManager, tests reach into the_current_speedinternal field directly; consider either promoting it to a public property (with documentation) or adding a getter so tests and other systems don’t rely on a private implementation detail. - The
_gcd/_lcm-basedauto_calculate_wrap_periodcan produce very large wrap values when layer periods are large or poorly chosen; it may be worth clamping the computed LCM or logging a stronger warning when the result exceeds a reasonable threshold to avoid overflow and hard-to-debug wrapping behavior. - In
setuponParallaxManager, when an invalid or nullGameSettingsResourceis passed in the method silently returns; if this indicates a misconfiguration, consider logging a warning so missing observer hookups are easier to diagnose at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ParallaxManager`, tests reach into the `_current_speed` internal field directly; consider either promoting it to a public property (with documentation) or adding a getter so tests and other systems don’t rely on a private implementation detail.
- The `_gcd`/`_lcm`-based `auto_calculate_wrap_period` can produce very large wrap values when layer periods are large or poorly chosen; it may be worth clamping the computed LCM or logging a stronger warning when the result exceeds a reasonable threshold to avoid overflow and hard-to-debug wrapping behavior.
- In `setup` on `ParallaxManager`, when an invalid or null `GameSettingsResource` is passed in the method silently returns; if this indicates a misconfiguration, consider logging a warning so missing observer hookups are easier to diagnose at runtime.
## Individual Comments
### Comment 1
<location path="scripts/parallax_manager.gd" line_range="26-35" />
<code_context>
+## Prevents tight coupling to global singletons in the process loop.
+## @param settings: GameSettingsResource - The configuration resource.
+## @return: void
+func setup(settings: GameSettingsResource) -> void:
+ if not is_instance_valid(settings):
+ return
+
+ _difficulty = settings.difficulty
+ _out_of_fuel = (settings.current_fuel <= 0.0)
+
+ if not settings.setting_changed.is_connected(_on_setting_changed):
+ settings.setting_changed.connect(_on_setting_changed)
+ if not settings.fuel_depleted.is_connected(_on_fuel_depleted):
+ settings.fuel_depleted.connect(_on_fuel_depleted)
+
</code_context>
<issue_to_address>
**suggestion:** Guard against multiple setup() calls with different settings instances to avoid duplicated signal connections.
If `setup()` can be called again (e.g., in tests or scene reloads) with a different `GameSettingsResource`, the old instance stays connected to `_on_setting_changed`/`_on_fuel_depleted`. Consider tracking the current settings in a private field and either disconnecting its signals before switching, or early‑returning when `setup()` is called with the same instance to avoid duplicate callbacks and leaks.
Suggested implementation:
```
var _current_speed: float = 0.0
var _difficulty: float = 1.0
var _out_of_fuel: bool = false
var _settings: GameSettingsResource
```
```
## Injects the game settings resource and wires up observer signals.
## Prevents tight coupling to global singletons in the process loop.
## @param settings: GameSettingsResource - The configuration resource.
## @return: void
func setup(settings: GameSettingsResource) -> void:
if not is_instance_valid(settings):
return
# If we already use this instance, just refresh cached values and avoid reconnecting.
if is_instance_valid(_settings) and _settings == settings:
_difficulty = settings.difficulty
_out_of_fuel = (settings.current_fuel <= 0.0)
return
# Disconnect old settings signals if we are switching to a new instance.
if is_instance_valid(_settings):
if _settings.setting_changed.is_connected(_on_setting_changed):
_settings.setting_changed.disconnect(_on_setting_changed)
if _settings.fuel_depleted.is_connected(_on_fuel_depleted):
_settings.fuel_depleted.disconnect(_on_fuel_depleted)
_settings = settings
_difficulty = settings.difficulty
_out_of_fuel = (settings.current_fuel <= 0.0)
if not settings.setting_changed.is_connected(_on_setting_changed):
settings.setting_changed.connect(_on_setting_changed)
if not settings.fuel_depleted.is_connected(_on_fuel_depleted):
settings.fuel_depleted.connect(_on_fuel_depleted)
```
</issue_to_address>
### Comment 2
<location path="run_pipeline.sh" line_range="9" />
<code_context>
EXPORT_DIR="$PROJECT_DIR/export/web_thread_off"
SERVER_PORT=8080
-PW_TIMEOUT=10000 # Default timeout in ms; adjustable
+PW_TIMEOUT=10 # Value is in SECONDS for pytest-timeout compatibility
# Function to check if a step failed
</code_context>
<issue_to_address>
**suggestion (testing):** A 10-second global pytest timeout may be too aggressive for Playwright end-to-end runs.
On slower or loaded CI runners, Playwright tests often run longer than 10 seconds, so this global timeout is likely to cause flaky failures rather than catch real regressions. Consider a higher default (e.g., 60–120 seconds) and only tighten timeouts for specific tests that must be short-lived.
```suggestion
PW_TIMEOUT=60 # Default in SECONDS for pytest-timeout; higher to avoid flaky Playwright e2e runs on slower CI
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+26
to
+35
| func setup(settings: GameSettingsResource) -> void: | ||
| if not is_instance_valid(settings): | ||
| return | ||
|
|
||
| _difficulty = settings.difficulty | ||
| _out_of_fuel = (settings.current_fuel <= 0.0) | ||
|
|
||
| if not settings.setting_changed.is_connected(_on_setting_changed): | ||
| settings.setting_changed.connect(_on_setting_changed) | ||
| if not settings.fuel_depleted.is_connected(_on_fuel_depleted): |
Contributor
There was a problem hiding this comment.
suggestion: Guard against multiple setup() calls with different settings instances to avoid duplicated signal connections.
If setup() can be called again (e.g., in tests or scene reloads) with a different GameSettingsResource, the old instance stays connected to _on_setting_changed/_on_fuel_depleted. Consider tracking the current settings in a private field and either disconnecting its signals before switching, or early‑returning when setup() is called with the same instance to avoid duplicate callbacks and leaks.
Suggested implementation:
var _current_speed: float = 0.0
var _difficulty: float = 1.0
var _out_of_fuel: bool = false
var _settings: GameSettingsResource
## Injects the game settings resource and wires up observer signals.
## Prevents tight coupling to global singletons in the process loop.
## @param settings: GameSettingsResource - The configuration resource.
## @return: void
func setup(settings: GameSettingsResource) -> void:
if not is_instance_valid(settings):
return
# If we already use this instance, just refresh cached values and avoid reconnecting.
if is_instance_valid(_settings) and _settings == settings:
_difficulty = settings.difficulty
_out_of_fuel = (settings.current_fuel <= 0.0)
return
# Disconnect old settings signals if we are switching to a new instance.
if is_instance_valid(_settings):
if _settings.setting_changed.is_connected(_on_setting_changed):
_settings.setting_changed.disconnect(_on_setting_changed)
if _settings.fuel_depleted.is_connected(_on_fuel_depleted):
_settings.fuel_depleted.disconnect(_on_fuel_depleted)
_settings = settings
_difficulty = settings.difficulty
_out_of_fuel = (settings.current_fuel <= 0.0)
if not settings.setting_changed.is_connected(_on_setting_changed):
settings.setting_changed.connect(_on_setting_changed)
if not settings.fuel_depleted.is_connected(_on_fuel_depleted):
settings.fuel_depleted.connect(_on_fuel_depleted)
| EXPORT_DIR="$PROJECT_DIR/export/web_thread_off" | ||
| SERVER_PORT=8080 | ||
| PW_TIMEOUT=10000 # Default timeout in ms; adjustable | ||
| PW_TIMEOUT=10 # Value is in SECONDS for pytest-timeout compatibility |
Contributor
There was a problem hiding this comment.
suggestion (testing): A 10-second global pytest timeout may be too aggressive for Playwright end-to-end runs.
On slower or loaded CI runners, Playwright tests often run longer than 10 seconds, so this global timeout is likely to cause flaky failures rather than catch real regressions. Consider a higher default (e.g., 60–120 seconds) and only tighten timeouts for specific tests that must be short-lived.
Suggested change
| PW_TIMEOUT=10 # Value is in SECONDS for pytest-timeout compatibility | |
| PW_TIMEOUT=60 # Default in SECONDS for pytest-timeout; higher to avoid flaky Playwright e2e runs on slower CI |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Decouple parallax background scrolling from the main scene via a dedicated ParallaxManager and update related systems, tests, and pipeline tooling to support the new architecture and test stack.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Tests: