Conversation
Implement child.free(), the "phantom orphan" edge case vanishes entirely. It instantly incinerates the placeholders, meaning you won't even need to add await get_tree().process_frame to your GUT tests. The memory will be clean the moment the setup function finishes executing.
This test plan validates the fix for orphan node leaks caused by improper node cleanup in setup_bushes_layer() and setup_decor_layer().
Contributor
Reviewer's GuideFixes orphan node leaks and enforces performance/visual constraints for the main scene’s parallax background while tightening player lifecycle cleanup and adding targeted GUT coverage. Class diagram for main_scene parallax setup and decor spritesclassDiagram
class MainScene {
- bool _showing_unbound_warning
- bool _showing_unbound_key_message
+ Node2D player
+ Panel stats_panel
+ ParallaxBackground background
+ ParallaxLayer bushes_layer
+ ParallaxLayer decor_layer
+ void setup_bushes_layer(viewport: Vector2)
+ void setup_decor_layer(viewport: Vector2)
+ void show_message(text: String, type: MessageType)
}
class ParallaxBackground {
+ Vector2 scroll_base_offset
+ Vector2 scroll_base_scale
}
class ParallaxLayer {
+ Vector2 motion_scale
+ Vector2 motion_mirroring
+ Array get_children()
+ void add_child(node: Node)
+ void remove_child(node: Node)
}
class Sprite2D {
+ Texture2D texture
+ bool centered
+ Vector2 scale
+ bool flip_h
+ bool flip_v
+ float rotation
+ Vector2 position
}
class TexturePreloader {
+ Array get_resource_list()
+ Texture2D get_resource(id: String)
}
MainScene --> ParallaxBackground : has
MainScene --> ParallaxLayer : uses_bushes_layer
MainScene --> ParallaxLayer : uses_decor_layer
MainScene --> TexturePreloader : uses
MainScene --> Sprite2D : creates
ParallaxBackground --> ParallaxLayer : contains
Flow diagram for setup_decor_layer decor generation and cleanupflowchart TD
A["setup_decor_layer(viewport)"] --> B{decor_layer exists?}
B -- "no" --> Z["return"]
B -- "yes" --> C["for each child in decor_layer.get_children()"]
C --> D["decor_layer.remove_child(child)"]
D --> E["child.free()"]
E --> F["collect decor_ids from texture_preloader where id begins_with decor_"]
F --> G{decor_ids is_empty?}
G -- "yes" --> Z
G -- "no" --> H["screens_tall = 8.0"]
H --> I["layer_height = viewport.y * screens_tall"]
I --> J["num_decors = decor_ids.size() * 2"]
J --> K["allowed_rotations = [0.0, 90.0, 180.0, -90.0]"]
K --> L["for i in range(num_decors)"]
L --> M["decor = Sprite2D.new()"]
M --> N["id = decor_ids.pick_random()"]
N --> O["decor.texture = texture_preloader.get_resource(id)"]
O --> P["decor.centered = false"]
P --> Q["scale_factor = randf_range(0.5, 1.5)"]
Q --> R["decor.scale = Vector2(scale_factor, scale_factor)"]
R --> S["decor.flip_h = randf() < 0.5"]
S --> T["decor.flip_v = randf() < 0.5"]
T --> U["random_degrees = allowed_rotations.pick_random()"]
U --> V["decor.rotation = deg_to_rad(random_degrees)"]
V --> W["decor.position.x = randf_range(0, viewport.x - decor.texture.get_width() * scale_factor)"]
W --> X["decor.position.y = randf_range(0, layer_height - decor.texture.get_height() * scale_factor)"]
X --> Y["decor_layer.add_child(decor)"]
Y --> L
L --> AA["decor_layer.motion_mirroring = Vector2(0, layer_height)"]
AA --> Z
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
📝 WalkthroughWalkthroughReplaced deferred frees with immediate Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
ikostan
changed the title
![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 switch from
remove_child(child); child.queue_free()toif is_instance_valid(child): child.free()is a bit risky; in Godot scenes it’s usually safer and more idiomatic to just callchild.queue_free()on each child without manually removing it, rather than usingfree()directly while iterating the node tree. - There’s now a fair amount of duplicated
Performance.get_monitor(Performance.OBJECT_ORPHAN_NODE_COUNT)baseline/compare logic across the new tests; consider extracting a small helper or utility assertion to keep each test focused on the scenario rather than the boilerplate leak-check code. - The commented-out
remove_childandprintlines insetup_bushes_layer/setup_decor_layeradd noise; if they’re no longer needed, it would be cleaner to remove them or gate the logging behind a debug flag instead of leaving them commented out.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The switch from `remove_child(child); child.queue_free()` to `if is_instance_valid(child): child.free()` is a bit risky; in Godot scenes it’s usually safer and more idiomatic to just call `child.queue_free()` on each child without manually removing it, rather than using `free()` directly while iterating the node tree.
- There’s now a fair amount of duplicated `Performance.get_monitor(Performance.OBJECT_ORPHAN_NODE_COUNT)` baseline/compare logic across the new tests; consider extracting a small helper or utility assertion to keep each test focused on the scenario rather than the boilerplate leak-check code.
- The commented-out `remove_child` and `print` lines in `setup_bushes_layer`/`setup_decor_layer` add noise; if they’re no longer needed, it would be cleaner to remove them or gate the logging behind a debug flag instead of leaving them commented out.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Python |  |
Apr 16, 2026 10:43p.m. | Review ↗ |
| JavaScript | |
Apr 16, 2026 10:43p.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.
![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: 1
🧹 Nitpick comments (4)
scripts/main_scene.gd (2)
164-173: Same dead-code cleanup applies here.Lines 165 and 173 mirror the commented-out fragments in
setup_bushes_layer. Apply the same removal for consistency.♻️ Proposed cleanup
# Clear existing children for child in decor_layer.get_children(): - # decor_layer.remove_child(child) if is_instance_valid(child): child.free() # Get decor IDs from preloader (Array[String]) var decor_ids: Array = Array(texture_preloader.get_resource_list()).filter( func(id: String) -> bool: return id.begins_with("decor_") ) - # print("Loaded ", decor_ids.size(), " decor textures")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/main_scene.gd` around lines 164 - 173, Remove the dead commented lines in the decor cleanup block to match the cleanup performed in setup_bushes_layer: delete the commented-out decor_layer.remove_child(child) inside the for loop and the commented-out print("Loaded ", decor_ids.size(), " decor textures") after building decor_ids; keep the is_instance_valid(child) check and child.free() logic and the decor_ids construction using texture_preloader so behavior remains unchanged.
120-129: Remove dead/commented-out code rather than leaving it behind.The commented-out
bushes_layer.remove_child(child)(Line 121) andprint("Loaded ", bush_ids.size(), ...)(Line 129) are now dead code. Version control preserves history — leaving them as comments adds noise and invites confusion about whether they’re intentional placeholders. Either delete them or replace with a short rationale comment.♻️ Proposed cleanup
# Clear existing children for child in bushes_layer.get_children(): - # bushes_layer.remove_child(child) if is_instance_valid(child): child.free() # Get bush IDs from preloader (Array[String]) var bush_ids: Array = Array(texture_preloader.get_resource_list()).filter( func(id: String) -> bool: return id.begins_with("bush_") ) - # print("Loaded ", bush_ids.size(), " bush textures")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/main_scene.gd` around lines 120 - 129, The commented-out lines in the block that iterates over bushes_layer and gathers bush_ids are dead code; remove the commented-out bushes_layer.remove_child(child) and the print statement (or replace each with a one-line rationale comment if you intend to keep the intent) to reduce noise. Update the section around the for loop that frees children (referencing bushes_layer and is_instance_valid) and the bush texture collection that uses texture_preloader and bush_ids so the code contains only active logic or a brief justification comment.test/gut/test_main_scene_orphan_nodes.gd (2)
42-43: Explicitqueue_free()on an autofree-tracked node — minor redundancy.
before_eachalready registersmain_scenewithadd_child_autofree, so the node will be freed at test teardown regardless. Manually callingqueue_free()here is fine (GUT checks validity before auto-freeing), but it couples the test to internal autofree behavior. For lifecycle tests that specifically want to observe post-free state, consider usingadd_child(main_scene)(non-autofree) in these tests and managing the lifecycle explicitly, so the intent is clear and there’s no possibility of double-free across GUT versions.Same pattern on Line 124.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_main_scene_orphan_nodes.gd` around lines 42 - 43, The test explicitly calls main_scene.queue_free() then awaits get_tree().process_frame while before_each already registers main_scene with add_child_autofree, causing redundant coupling to autofree behavior; change tests that intend to observe post-free state to use add_child(main_scene) instead of add_child_autofree in the before_each (or in the specific test setup) so you manage the node lifecycle explicitly, remove the redundant reliance on autofree, and keep the queue_free() + await get_tree().process_frame sequence only in tests that own the node’s lifetime; update the setup referencing before_each, add_child_autofree, add_child, main_scene, queue_free, and get_tree().process_frame accordingly.
29-46: Useassert_lteinstead ofassert_eqfor engine-wide orphan counter.
Performance.OBJECT_ORPHAN_NODE_COUNTis engine-wide and reports orphans across the entire engine, not just nodes in this test. During the awaited frames, unrelated systems may transiently create or free orphan nodes, causing flaky assertions with strict equality. Leak regression checks are better expressed as "did not grow" rather than "exactly equal".Apply to lines 46, 65, 136, and 156:
Diff
- assert_eq(current_orphans, baseline_orphans, "Expected orphan nodes to return to baseline after frame sync and teardown.") + assert_lte(current_orphans, baseline_orphans, "Orphan node count must not grow after frame sync and teardown.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_main_scene_orphan_nodes.gd` around lines 29 - 46, Replace strict equality checks against the engine-wide orphan counter with a non-increasing assertion: where the test captures baseline_orphans via Performance.get_monitor(Performance.OBJECT_ORPHAN_NODE_COUNT) and later computes current_orphans, change the assertion from assert_eq(current_orphans, baseline_orphans, ...) to assert_lte(current_orphans, baseline_orphans, ...) (apply the same replacement for the other occurrences that compare Performance.OBJECT_ORPHAN_NODE_COUNT at the indicated spots). This ensures the test verifies the orphan count did not grow rather than requiring exact equality across engine-wide, transient activity.
🤖 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_main_scene_orphan_nodes.gd`:
- Around line 38-39: The comment next to "await get_tree().process_frame" is
outdated because production now uses immediate removal via child.free() not
queue_free(); update the test comment to reflect that the frame flush is no
longer required and explain that get_tree().process_frame was previously used to
wait for queue_free() but is unnecessary after main_scene.gd was changed to call
child.free() — locate the await in test/gut/test_main_scene_orphan_nodes.gd and
replace the rationale text to state that immediate freeing is used and the await
can be removed or kept only for clarity, referencing get_tree().process_frame,
queue_free(), child.free(), and main_scene.gd so reviewers understand the
change.
---
Nitpick comments:
In `@scripts/main_scene.gd`:
- Around line 164-173: Remove the dead commented lines in the decor cleanup
block to match the cleanup performed in setup_bushes_layer: delete the
commented-out decor_layer.remove_child(child) inside the for loop and the
commented-out print("Loaded ", decor_ids.size(), " decor textures") after
building decor_ids; keep the is_instance_valid(child) check and child.free()
logic and the decor_ids construction using texture_preloader so behavior remains
unchanged.
- Around line 120-129: The commented-out lines in the block that iterates over
bushes_layer and gathers bush_ids are dead code; remove the commented-out
bushes_layer.remove_child(child) and the print statement (or replace each with a
one-line rationale comment if you intend to keep the intent) to reduce noise.
Update the section around the for loop that frees children (referencing
bushes_layer and is_instance_valid) and the bush texture collection that uses
texture_preloader and bush_ids so the code contains only active logic or a brief
justification comment.
In `@test/gut/test_main_scene_orphan_nodes.gd`:
- Around line 42-43: The test explicitly calls main_scene.queue_free() then
awaits get_tree().process_frame while before_each already registers main_scene
with add_child_autofree, causing redundant coupling to autofree behavior; change
tests that intend to observe post-free state to use add_child(main_scene)
instead of add_child_autofree in the before_each (or in the specific test setup)
so you manage the node lifecycle explicitly, remove the redundant reliance on
autofree, and keep the queue_free() + await get_tree().process_frame sequence
only in tests that own the node’s lifetime; update the setup referencing
before_each, add_child_autofree, add_child, main_scene, queue_free, and
get_tree().process_frame accordingly.
- Around line 29-46: Replace strict equality checks against the engine-wide
orphan counter with a non-increasing assertion: where the test captures
baseline_orphans via
Performance.get_monitor(Performance.OBJECT_ORPHAN_NODE_COUNT) and later computes
current_orphans, change the assertion from assert_eq(current_orphans,
baseline_orphans, ...) to assert_lte(current_orphans, baseline_orphans, ...)
(apply the same replacement for the other occurrences that compare
Performance.OBJECT_ORPHAN_NODE_COUNT at the indicated spots). This ensures the
test verifies the orphan count did not grow rather than requiring exact equality
across engine-wide, transient activity.
🪄 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: 741764e1-e1d9-4e45-9f22-dbe5e8b69598
📒 Files selected for processing (3)
scripts/main_scene.gdtest/gut/test_main_scene_orphan_nodes.gdtest/gut/test_main_scene_orphan_nodes.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). (3)
- GitHub Check: GDUnit4 Unit Tests / unit-test
- GitHub Check: GUT Unit Tests / unit-test
- GitHub Check: Sourcery review
🧰 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_main_scene_orphan_nodes.gd
🔇 Additional comments (2)
test/gut/test_main_scene_orphan_nodes.gd (1)
16-21: LGTM — asyncbefore_eachwith frame flush is the right pattern.Using
add_child_autofreeplus a one-frame await lets_ready()run (including the initialsetup_bushes_layer/setup_decor_layercalls) before each test exercises the cleanup path. Good setup.test/gut/test_main_scene_orphan_nodes.gd.uid (1)
1-1: Auto-generated UID — no review needed.Standard Godot 4 resource UID mapping for the new test script.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This is the idiomatic and safe pattern in Godot because: queue_free() defers deletion → avoids modifying the scene tree mid-iteration Prevents: iterator invalidation hard-to-reproduce crashes undefined behavior in complex trees Lets Godot handle safe teardown order internally
Owner
Author
Addressed |
Introduce verify_no_orphan_leaks() to centralize orphan-node assertions and replace scattered Performance.get_monitor checks across tests. Harden tests by flushing frames at key points (to allow queue_free()/_ready() work to settle), filter out nodes queued for deletion when comparing child counts, and add minor assertions/log output to improve clarity. These changes reduce flakiness and better detect true orphan-node leaks in test/gut/test_main_scene_orphan_nodes.gd.
Owner
Author
Addressed |
The commented-out remove_child and print lines in setup_bushes_layer/setup_decor_layer add noise; if they’re no longer needed, it would be cleaner to remove them or gate the logging behind a debug flag instead of leaving them commented out.
Owner
Author
Addressed |
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:
- Consider giving
stats_panela concrete type (e.g.,Panelor a specific custom type) instead ofVariantnow that the leak issue is fixed, so you retain static checking and editor hints on that node. - The orphan-node tests depend on the global
Performance.OBJECT_ORPHAN_NODE_COUNTand the shared SceneTree, which can be polluted by other tests or scenes; you may want to isolate these tests (e.g., with a dedicated SceneTree or by ensuring no other test scenes are active) to avoid flaky baselines.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider giving `stats_panel` a concrete type (e.g., `Panel` or a specific custom type) instead of `Variant` now that the leak issue is fixed, so you retain static checking and editor hints on that node.
- The orphan-node tests depend on the global `Performance.OBJECT_ORPHAN_NODE_COUNT` and the shared SceneTree, which can be polluted by other tests or scenes; you may want to isolate these tests (e.g., with a dedicated SceneTree or by ensuring no other test scenes are active) to avoid flaky baselines.
## Individual Comments
### Comment 1
<location path="scripts/main_scene.gd" line_range="15-18" />
<code_context>
@onready var player: Node2D = $Player
-# @onready var stats_panel: Panel = $PlayerStatsPanel
@onready var stats_panel: Variant = $PlayerStatsPanel
@onready var background: ParallaxBackground = $Background
@onready var bushes_layer: ParallaxLayer = $Background/Bushes # Reference to the bushes layer
</code_context>
<issue_to_address>
**suggestion:** Consider tightening the type of `stats_panel` instead of using `Variant`.
If this is always a specific node type, prefer a concrete type to retain static checks and editor autocompletion. If you need to support multiple node types, consider typing it to a shared base class (e.g., `Control`) or an interface-like abstraction instead of falling back to `Variant`.
```suggestion
@onready var player: Node2D = $Player
@onready var stats_panel: Panel = $PlayerStatsPanel
@onready var background: ParallaxBackground = $Background
@onready var bushes_layer: ParallaxLayer = $Background/Bushes # Reference to the bushes layer
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ing Variant. If this is always a specific node type, prefer a concrete type to retain static checks and editor autocompletion. If you need to support multiple node types, consider typing it to a shared base class (e.g., Control) or an interface-like abstraction instead of falling back to Variant.
Owner
Author
Addressed |
Reduce repeating layer height from 20 screens to 8 for bushes and decor to balance the infinite-scrolling illusion with CPU overhead. Lower the density multipliers from 5x to 2x (num_bushes and num_decors) so perceived density matches the smaller repeated area. Comments updated to explain the rationale; no changes to rotation logic.
The Structural Asserts: test_parallax_chunk_size_is_optimized and test_parallax_sprite_density_is_optimized explicitly enforce the 8-screen limit and the 2x multiplier we just set. They act as automated guardrails against future developers accidentally re-introducing the BVH bloat that caused your FPS drop. The Execution Proxy: test_process_script_execution_time is a neat trick. It fires the _process() function 60 times in a row as fast as possible and measures the raw CPU time in microseconds. While it doesn't measure the GPU rendering the sprites, it does measure the GDScript overhead, ensuring your math and checks inside _process stay incredibly fast.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
test_decor_sprites_have_boolean_flipstest only checks thatflip_h/flip_vare of typebool(which they always are by default) rather than that random flips are actually being applied; consider asserting that at least some sprites havetrueand some havefalseto make this test meaningful. - Parallax chunk size and density behaviors (8-screen height and 2x density) are asserted in multiple suites (
test_main_scene_parallax_chunks.gd,test_main_scene_performance_limits.gd, etc.); consolidating these checks into a single focused test file would reduce duplication and future maintenance overhead if those parameters change. - Test teardown strategies vary between files (some rely on
autofreeand frame flushes while others callfree()explicitly); standardizing on a single, clearly documented cleanup pattern for MainScene tests would make lifecycle behavior easier to reason about and reduce the risk of subtle orphan-node issues reappearing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_decor_sprites_have_boolean_flips` test only checks that `flip_h`/`flip_v` are of type `bool` (which they always are by default) rather than that random flips are actually being applied; consider asserting that at least some sprites have `true` and some have `false` to make this test meaningful.
- Parallax chunk size and density behaviors (8-screen height and 2x density) are asserted in multiple suites (`test_main_scene_parallax_chunks.gd`, `test_main_scene_performance_limits.gd`, etc.); consolidating these checks into a single focused test file would reduce duplication and future maintenance overhead if those parameters change.
- Test teardown strategies vary between files (some rely on `autofree` and frame flushes while others call `free()` explicitly); standardizing on a single, clearly documented cleanup pattern for MainScene tests would make lifecycle behavior easier to reason about and reduce the risk of subtle orphan-node issues reappearing.
## Individual Comments
### Comment 1
<location path="scripts/main_scene.gd" line_range="200-202" />
<code_context>
+ var scale_factor: float = randf_range(0.5, 1.5)
decor.scale = Vector2(scale_factor, scale_factor)
+ # SCALING TRICK 2: Randomly mirror the sprite horizontally and/or vertically
+ decor.flip_h = [true, false].pick_random()
+ decor.flip_v = [true, false].pick_random()
+
+ # Apply random cardinal rotation to ALL decor sprites
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid reallocating the `[true, false]` array on every sprite when randomizing flips.
`[true, false].pick_random()` inside the loop allocates a new array for each call, which adds overhead for large `num_decors`. Prefer either a shared constant (e.g. `const BOOL_CHOICES := [true, false]`) or use `randf() < 0.5` for each flip flag to avoid per-iteration allocations while keeping the same behavior.
```suggestion
# SCALING TRICK 2: Randomly mirror the sprite horizontally and/or vertically
decor.flip_h = randf() < 0.5
decor.flip_v = randf() < 0.5
```
</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>
The PR does not change the child cleanup semantics to eliminate the orphan window. setup_bushes_layer() still has no explicit clear loop shown in the diff, and setup_decor_layer() continues to clear children using decor_layer.remove_child(child) followed by child.queue_free(), rather than calling free() directly. This preserves the timing window where removed children exist as orphans until the end of the frame, which is the core bug described in the issue.
…gd' of https://github.com/ikostan/SkyLockAssault into orphan-node-leak-from-placeholder-sprites-in-main_scenegd
The test_decor_sprites_have_boolean_flips test only checks that flip_h/flip_v are of type bool (which they always are by default) rather than that random flips are actually being applied; consider asserting that at least some sprites have true and some have false to make this test meaningful.
Owner
Author
Addressed |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/main_scene.gd (1)
194-211:⚠️ Potential issue | 🟡 MinorRotation with
centered = falsecan push decor sprites outside the intended bounds.
decor.centered = falsekeeps the pivot at the sprite's top-left. Random rotations of 90°, 180°, and -90° are then applied around that top-left pivot, but the position clamp on lines 208–211 only accounts for the unrotated extent (texture.get_width() * scale_factoron X, height on Y). Concretely:
- 90°: after rotation the sprite occupies
[x, x + h*scale] × [y - w*scale, y], so it can extend above the layer (negative Y).- 180°: it occupies
[x - w*scale, x] × [y - h*scale, y], so it can extend left/above.- -90°: it occupies
[x - h*scale, x] × [y, y + w*scale], so it can extend left.For non-square textures this also means horizontal-fit assumes
width*scalebut a ±90° rotation makes the effective horizontal extentheight*scale. Result: decor can render outside the randomized safe area (and, with 180°, even into negative coordinates near the top of the layer).Simplest fix is to
centered = trueon decor sprites so rotation pivots around the sprite center, then shrink the clamp by half-extents; alternatively compute the rotated AABB and clamp accordingly.♻️ Proposed fix (centered pivot)
- decor.texture = texture_preloader.get_resource(id) - decor.centered = false + decor.texture = texture_preloader.get_resource(id) + decor.centered = true @@ - decor.position.x = randf_range(0, viewport.x - (decor.texture.get_width() * scale_factor)) - decor.position.y = randf_range( - 0, layer_height - (decor.texture.get_height() * scale_factor) - ) + # With centered pivot, use the max of width/height for the half-extent so any cardinal rotation stays in bounds. + var tex_size: Vector2 = decor.texture.get_size() + var half_extent: float = max(tex_size.x, tex_size.y) * 0.5 * scale_factor + decor.position.x = randf_range(half_extent, viewport.x - half_extent) + decor.position.y = randf_range(half_extent, layer_height - half_extent)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/main_scene.gd` around lines 194 - 211, The decor sprites are rotated around the top-left because decor.centered = false, so applying allowed_rotations.pick_random() and setting decor.rotation can push sprites outside the intended bounds when you clamp using texture.get_width()/get_height() * scale_factor; fix this by setting the sprite pivot to its center (set decor.centered = true) and adjust the position clamps to account for half-extents (use (texture.get_width() * scale_factor) / 2 and (texture.get_height() * scale_factor) / 2) before assigning decor.position.x and decor.position.y so rotations around center stay within the layer.
🧹 Nitpick comments (2)
test/gut/test_main_scene_parallax_chunks.gd (1)
70-102: Consider consolidating the two near-duplicate tests.
test_bushes_layer_chunk_size_and_densityandtest_decor_layer_chunk_size_and_densitydiffer only by layer reference, ID prefix, and setup method. A small parameterized helper (orgut'sparameterize_test) would remove the duplication and keep future mirroring-height / density changes to a single edit. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_main_scene_parallax_chunks.gd` around lines 70 - 102, The two near-duplicate tests test_bushes_layer_chunk_size_and_density and test_decor_layer_chunk_size_and_density should be consolidated by extracting the common assertions into a single parameterized helper (or using gut's parameterize_test) that accepts the layer setup method (main_scene.setup_decor_layer / main_scene.setup_bushes_layer), the layer accessor (main_scene.decor_layer / main_scene.bushes_layer) and the ID prefix ("decor_" / "bushes_"); re-run the appropriate setup via the passed setup function, compute expected_height using viewport_mock.y * 8.0, get resource IDs via texture_preloader.get_resource_list() filtered by the given prefix, compute expected_count = ids.size() * 2, and assert the active child count (filtering out is_queued_for_deletion) and motion_mirroring.y against those expected values so both layer tests are covered by one parameterized test.test/gut/test_decor_layer_transformations.gd (1)
16-28: Redundant lifecycle:add_child_autofreeplus explicitmain_scene.free()inafter_each.
add_child_autofree()registersmain_scenefor automatic freeing at test teardown. Callingmain_scene.free()first inafter_each()destroys the node synchronously; GUT's autofree pass then runs against an already-invalid instance. Theis_instance_valid()check prevents a crash, but this pattern is redundant and obscures intent. The same pattern appears intest_main_scene_parallax_chunks.gdandtest_main_scene_performance_limits.gd.Pick one strategy:
- Keep autofree, drop manual
free(): Let autofree own lifecycle; GUT will handle cleanup.- Drop autofree, keep manual
free(): Useadd_child(main_scene)instead and callfree()explicitly if immediate teardown is required.Option A: keep autofree, drop manual free
func after_each() -> void: - if is_instance_valid(main_scene): - main_scene.free() - await get_tree().process_frame + # main_scene is released by add_child_autofree; flush a frame to settle deferred work. + await get_tree().process_frame🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/gut/test_decor_layer_transformations.gd` around lines 16 - 28, The teardown currently redundantly calls main_scene.free() in after_each() even though before_each() uses add_child_autofree(main_scene); remove the manual free to let GUT's autofree handle lifecycle: delete the is_instance_valid(...) { main_scene.free() } block from after_each() (leave the await get_tree().process_frame) so add_child_autofree and main_scene remain the single lifecycle mechanism; alternatively, if you prefer manual freeing, replace add_child_autofree(main_scene) in before_each() with add_child(main_scene) and keep the explicit free() in after_each()—pick one strategy and apply the change consistently across test_decor_layer_transformations.gd, test_main_scene_parallax_chunks.gd, and test_main_scene_performance_limits.gd.
🤖 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_main_scene_performance_limits.gd`:
- Around line 101-120: The test times main_scene._process but is flaky and
polluted by side effects: widen the threshold and/or increase the sample (e.g.,
>60 frames) or mark the test for a dedicated perf job, and eliminate the
first-call side effects by pre-warming or stubbing the branch that mutates
background.scroll_offset and calls show_message; specifically, call
main_scene._process once before timing (or set
Settings.has_unbound_critical_actions_for_current_device() to return false) to
avoid scheduling get_tree().create_timer and label updates, and ensure any
created timer/Label nodes are removed/cleaned up after the test so the
measurement only captures steady-state _process cost.
---
Outside diff comments:
In `@scripts/main_scene.gd`:
- Around line 194-211: The decor sprites are rotated around the top-left because
decor.centered = false, so applying allowed_rotations.pick_random() and setting
decor.rotation can push sprites outside the intended bounds when you clamp using
texture.get_width()/get_height() * scale_factor; fix this by setting the sprite
pivot to its center (set decor.centered = true) and adjust the position clamps
to account for half-extents (use (texture.get_width() * scale_factor) / 2 and
(texture.get_height() * scale_factor) / 2) before assigning decor.position.x and
decor.position.y so rotations around center stay within the layer.
---
Nitpick comments:
In `@test/gut/test_decor_layer_transformations.gd`:
- Around line 16-28: The teardown currently redundantly calls main_scene.free()
in after_each() even though before_each() uses add_child_autofree(main_scene);
remove the manual free to let GUT's autofree handle lifecycle: delete the
is_instance_valid(...) { main_scene.free() } block from after_each() (leave the
await get_tree().process_frame) so add_child_autofree and main_scene remain the
single lifecycle mechanism; alternatively, if you prefer manual freeing, replace
add_child_autofree(main_scene) in before_each() with add_child(main_scene) and
keep the explicit free() in after_each()—pick one strategy and apply the change
consistently across test_decor_layer_transformations.gd,
test_main_scene_parallax_chunks.gd, and test_main_scene_performance_limits.gd.
In `@test/gut/test_main_scene_parallax_chunks.gd`:
- Around line 70-102: The two near-duplicate tests
test_bushes_layer_chunk_size_and_density and
test_decor_layer_chunk_size_and_density should be consolidated by extracting the
common assertions into a single parameterized helper (or using gut's
parameterize_test) that accepts the layer setup method
(main_scene.setup_decor_layer / main_scene.setup_bushes_layer), the layer
accessor (main_scene.decor_layer / main_scene.bushes_layer) and the ID prefix
("decor_" / "bushes_"); re-run the appropriate setup via the passed setup
function, compute expected_height using viewport_mock.y * 8.0, get resource IDs
via texture_preloader.get_resource_list() filtered by the given prefix, compute
expected_count = ids.size() * 2, and assert the active child count (filtering
out is_queued_for_deletion) and motion_mirroring.y against those expected values
so both layer tests are covered by one parameterized test.
🪄 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: 16d625e6-6894-43c2-8d0c-39becf6e6220
📒 Files selected for processing (7)
scripts/main_scene.gdtest/gut/test_decor_layer_transformations.gdtest/gut/test_decor_layer_transformations.gd.uidtest/gut/test_main_scene_parallax_chunks.gdtest/gut/test_main_scene_parallax_chunks.gd.uidtest/gut/test_main_scene_performance_limits.gdtest/gut/test_main_scene_performance_limits.gd.uid
✅ Files skipped from review due to trivial changes (3)
- test/gut/test_main_scene_parallax_chunks.gd.uid
- test/gut/test_decor_layer_transformations.gd.uid
- test/gut/test_main_scene_performance_limits.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: Browser Functional Tests / 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_decor_layer_transformations.gdtest/gut/test_main_scene_performance_limits.gdtest/gut/test_main_scene_parallax_chunks.gd
🔇 Additional comments (5)
scripts/main_scene.gd (1)
16-16: LGTM — concretePaneltype forstats_panel.Replacing the implicit
Variantannotation withPanelenables static checks onhas_method("setup_hud")/setup_hud(player)call sites and improves editor tooling.test/gut/test_decor_layer_transformations.gd (2)
34-65: LGTM — cardinal rotation assertion.
is_equal_approxagainstdeg_to_rad({0, 90, 180, -90})correctly accommodates the float representation of π/2 and π thatNode2D.rotationreturns viaatan2, including the 180° boundary case.
102-119: This review comment is based on outdated code.The test file has already been updated. Lines 102–138 now correctly validate that both
trueandfalseflip states appear across the decor sprites, using boolean tracking variables and proper assertions—exactly matching the suggested fix in this review comment. The tautologicaltypeof()check no longer exists in the codebase.> Likely an incorrect or invalid review comment.test/gut/test_main_scene_parallax_chunks.gd (1)
38-67: LGTM — chunk size and density assertions.Computing
expected_countfrom the actualtexture_preloaderresource list (rather than hardcoding a number) keeps the test resilient to content changes while still enforcing the 2x multiplier.motion_mirroring.y == viewport_mock.y * 8.0is an exact product soassert_eqon floats is safe here.test/gut/test_main_scene_performance_limits.gd (1)
34-95: LGTM — layer-height and density enforcement.Checking
motion_mirroring.yand 2x density on both layers guards against accidental tuning regressions. The inline comment calling out BVH/draw-call trade-offs is useful context for future maintainers.
Rename test/gut/test_main_scene_performance_limits.gd -> test_main_scene_parallax_and_performance.gd (and corresponding .uid) and remove the separate test_main_scene_parallax_chunks.gd file. Add two new GUT tests that verify parallax layers: bushes and decor should mirror at exactly 8 screens tall (motion_mirroring.y == viewport.y * 8) and spawn 2x the number of sprites based on the texture preloader. Also add a brief initialization comment and ensure tests filter out nodes queued for deletion when counting active children.
Add a standardized safe_hard_free helper and use it across GUT tests to avoid double-free/orphan-window issues. Replace add_child_autofree with add_child and make after_each perform guarded teardown via safe_hard_free. Tighten and simplify test_main_scene_orphan_nodes.gd: explicit manual frees and nullification, added frame syncs, lambda type annotations, streamlined assertions, and cleanup of reloaded instances. These changes make test teardown deterministic and prevent orphan-node leaks and intermittent flakiness.
Owner
Author
Addressed |
…ects leak into the measurement. Two concerns with this test: A hard < 1000 µs average over 60 script calls will occasionally fail on shared CI runners (GC pauses, other tests running concurrently, headless renderer under contention). A script-time performance bound this tight is better enforced as a benchmark trend rather than a pass/fail assertion. Consider widening the threshold substantially, running a larger sample, and/or tagging it so it only runs in a dedicated perf job. main_scene._process on line 108 has side effects: it mutates background.scroll_offset, and on the first call will trigger show_message(...) if Settings.has_unbound_critical_actions_for_current_device() is true, which schedules a get_tree().create_timer(4.0) and adds a label update. The first iteration is therefore not representative, and the timer node lingers past the test. Worth either pre-warming once outside the timed loop, or explicitly setting state to skip the unbound branch during the measurement.
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:
- Using
child.free()insetup_bushes_layer/setup_decor_layeris more aggressive thanqueue_free()and can be risky if any of those sprites ever end up with external references or signals; consider whetherqueue_free()plus a frame flush or a dedicated cleanup pass would be safer for runtime code outside of tests. - The performance test
test_process_script_execution_timedepends onTime.get_ticks_usec()and absolute timing thresholds, which can be flaky across machines/CI; consider asserting more structural conditions (e.g. no allocations, no heavy loops) or using a much looser bound or profiler-based checks instead. - The
safe_hard_freehelper is duplicated across several GUT test files; consider centralizing this into a shared test utility script to keep teardown behavior consistent and easier to adjust in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `child.free()` in `setup_bushes_layer`/`setup_decor_layer` is more aggressive than `queue_free()` and can be risky if any of those sprites ever end up with external references or signals; consider whether `queue_free()` plus a frame flush or a dedicated cleanup pass would be safer for runtime code outside of tests.
- The performance test `test_process_script_execution_time` depends on `Time.get_ticks_usec()` and absolute timing thresholds, which can be flaky across machines/CI; consider asserting more structural conditions (e.g. no allocations, no heavy loops) or using a much looser bound or profiler-based checks instead.
- The `safe_hard_free` helper is duplicated across several GUT test files; consider centralizing this into a shared test utility script to keep teardown behavior consistent and easier to adjust in one place.
## Individual Comments
### Comment 1
<location path="scripts/main_scene.gd" line_range="133" />
<code_context>
- var layer_height: float = viewport.y * 4
+ # THE GOLDILOCKS ZONE:
+ # 8 screens is the sweet spot for infinite illusion vs CPU overhead
+ var screens_tall: float = 8.0
+ var layer_height: float = viewport.y * screens_tall
+
</code_context>
<issue_to_address>
**suggestion:** The hardcoded `screens_tall = 8.0` is duplicated and would be easier to tune as a single constant/config.
Since both `setup_bushes_layer` and `setup_decor_layer` use this `screens_tall = 8.0` value, any future adjustment would require changing it in multiple places. Please extract it into a shared constant or exported property so the parallax layer height is defined once and stays consistent across both layers.
Suggested implementation:
```
# THE GOLDILOCKS ZONE:
# 8 screens is the sweet spot for infinite illusion vs CPU overhead
# Exposed so both bushes and decor parallax layers share the same height tuning.
@export var parallax_screens_tall: float = 8.0
@onready var player: Node2D = $Player
```
In addition to the edit above, you should:
1. In `setup_bushes_layer`, replace the local hardcoded value:
- Change: `var screens_tall: float = 8.0`
- To: `var screens_tall: float = parallax_screens_tall`
2. In `setup_decor_layer`, do the same replacement:
- Change: `var screens_tall: float = 8.0`
- To: `var screens_tall: float = parallax_screens_tall`
3. If there are any other places in this script that use a hardcoded `8.0` specifically for parallax layer height (e.g. `viewport.y * 8.0`), update them to multiply by `parallax_screens_tall` instead so all layers stay consistent.
</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
|
@sourcery-ai title |
sourcery-ai
Bot
changed the title
…be easier to tune as a single constant/config. Since both setup_bushes_layer and setup_decor_layer use this screens_tall = 8.0 value, any future adjustment would require changing it in multiple places. Please extract it into a shared constant or exported property so the parallax layer height is defined once and stays consistent across both layers.
Owner
Author
The use of free() here is an intentional architectural choice to satisfy the requirements of Issue #540 (Eliminating the orphan window). Since these are procedurally generated Sprite2D assets with no external signals or persistent references, a hard free() after remove_child() is the most performant and memory-accurate way to reset the parallax layers without accumulating transient orphans. |
Remove duplicate safe_hard_free implementations from test/gut/test_decor_layer_transformations.gd and test/gut/test_main_scene_orphan_nodes.gd, add a const GutHelper preload, and replace local calls with GutHelper.safe_hard_free(...). Centralizes the safe-free logic to avoid orphan windows/double-frees and reduces duplicated test code.
Owner
Author
Both points have been addressed. Here is the breakdown of how the implementation now aligns with that feedback: 1. Centralized
|
| Requirement | Status | Implementation Detail |
|---|---|---|
| Centralized Helper | COMPLETED | Moved to gut_test_helper.gd and preloaded in all suites. |
| Looser Timing Bound | COMPLETED | Increased to 2 ms in test_main_scene_parallax_and_performance.gd. |
| Type Safety | COMPLETED | All lambda filters now include explicit -> bool return types. |
| Parallax Constants | COMPLETED | Uses @export var parallax_screens_tall for consistent layer height. |
ikostan
deleted the
orphan-node-leak-from-placeholder-sprites-in-main_scenegd
branch
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Summary: Parallax Performance Optimization and Test Suite Refactor
This Pull Request addresses critical FPS drops related to background generation and standardizes the project's unit testing architecture to ensure long-term stability and memory integrity.
1. Performance Optimizations
@exportvariableparallax_screens_tallinmain_scene.gd. This allows for real-time performance tuning of all background layers via the Godot Inspector without modifying code.2. Memory & Stability Fixes
setup_bushes_layer()andsetup_decor_layer()to use an immediatefree()strategy. By detaching children withremove_child()and instantly incinerating them, we have eliminated the "orphan window" bug where nodes persisted until the end of a frame.safe_hard_freehelper to prevent "previously freed" object errors and engine crashes during rapid scene transitions and unit test teardowns.3. Test Suite Enhancements
test_main_scene_parallax_and_performance.gdto reduce maintenance overhead.gut_test_helper.gd. All test suites now utilize this shared helper for consistent teardown behavior.flip_handflip_vare actively producing varied results rather than just checking data types.-> boolreturn types to all anonymous lambda functions within the test suite to satisfy strict GDScript type-checking requirements.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
Resolve orphan node leaks and enforce performance and visual constraints for the main scene’s parallax background and player lifecycle.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by Sourcery
Resolve orphan node leaks in the main scene while enforcing performance and visual constraints on its parallax background layers and tightening player lifecycle cleanup.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit