Keep global lights consistent when scene branches change

[toaster0123]

Problem

Global lights can disappear, linger, or be duplicated after normal scene changes.

For example, put global light A on one branch and update the scene. Then add global light B on a sibling branch and update again. Other geometries now receive only B, although A is still attached. Removing or reparenting a global-light branch can also leave its light in the old tree. Attaching an already-dirty subtree can duplicate a root-owned global light.

These violate the global-light contract: a global light affects the whole tree it belongs to.

Fix

The production change stays in Node:

• When rebuilding globals, visit clean branches too, rather than collecting only branches marked dirty
• Rebuild the root list before every global collection, including refreshes coming from an already-dirty descendant
• Mark light state dirty while a detached child's old parent is still reachable, so the old root removes globals from the departing subtree

A global-light rebuild visits the whole subtree because it replaces the complete root collection; clean branches must contribute their still-attached globals. This work is triggered by global-light refresh, not every ordinary light-list edit. The existing unchanged-frame early return and ordinary non-global inheritance are preserved.

The unchanged-update test intentionally checks an implementation-level no-extra-collection expectation, alongside exact light-list contents. It is a fast-path regression guard, not a timing benchmark or public accessor-call contract. This does not introduce deduplication, a new cache, or changes to light filtering.

Validation

• The committed 29-case suite has 22 assertion failures on unchanged upstream and passes all 29 with the fix
• An independent, unchanged 45-case public-API suite goes from 31 failures to 45 passes, including a 500-step deterministic mutation sequence
• Tests cover shallow/deep sibling branches and ordering, addition/removal, same/cross-root reparenting, detach, dirty-subtree duplication, mixed edits, unchanged updates, non-global inheritance, cloning and loaded-scene attachment
• The public DefaultLightFilter also demonstrates the lost light before the fix and both lights afterward. Infinite-radius point lights isolate membership from distance/frustum selection
• Core build and desktop/effects/plugins/terrain checks pass: 591 reported tests, zero failures/errors, two existing/environment skips. No new production Checkstyle diagnostics; the new test file has none
• Independent source review and exact-source fixture reruns found no blocker

Limits

This verifies scene light-list membership and default-filter output, rather than rendered pixels, shaders, finite-radius culling, concurrency or performance.

A separate standalone-deserialized-root initialization case fails identically before and after and is outside this patch. The passing roundtrip control attaches the loaded scene to a host root. The whole-repository local Android build remains outside these completed scoped checks because the SDK is unavailable.

[jaime-jmebot]

Nice, focused fix — and thanks for shipping a test suite alongside it.

A couple of questions on the detach path:

• detachChild no longer marks the detached child light-dirty. The old root now correctly drops the departing subtree's global lights, but the detached branch keeps the globals it inherited in its own light list until something refreshes it again. If that branch is inspected or rendered before being reattached (or is reattached to a parent that doesn't mark lights dirty), it can still report lights it no longer belongs to. Was removing child.setLightListRefresh() deliberate because every later path re-marks it, or is it an oversight? Marking the child as well would be cheap and would keep the upstream behavior intact.

• Collection no longer prunes on lightsDirty. Visiting clean branches is what makes sibling global lights survive, and correctness clearly wins here — but it does mean every light-dirty update walks the whole subtree instead of just the changed branch. Worth a note in the description (or a short comment on findGlobalLights) so nobody later re-adds the dirty-flag shortcut and reintroduces the lost-light bug.

The rest looks good to me: the "rebuild the root list before every collection" ordering matches the duplication symptom, and the unchanged-frame early return is preserved.

[jaime-jmebot]

This helper counts getLocalLightList() calls, which locks the tests to an internal optimization. If an assertion like "no local light list is read when the frame is unchanged" fails after a harmless refactor, the scene still behaves correctly and the test would be noise. Prefer asserting the resulting list contents here (like the other cases do), or keep this one but frame it explicitly as a performance expectation.

[toaster0123]

This is intentionally an implementation-level fast-path guard, alongside the exact content assertions already made on every unchanged frame in that test. It detects accidentally re-running global collection when no scene state changed; it is not meant as a public getter-call contract or a timing benchmark. I clarified that purpose in the PR description. A future change to the collection implementation can revise this instrumentation while retaining the separate list-content assertions.

[jaime-jmebot]

Thanks — that's clear, and the revised description frames it correctly. Since the list-content assertions already cover correctness, I'm happy to keep this helper as a fast-path guard for the early return. Nothing else from me on this thread.

[jaime-jmebot]

Solid, well-scoped fix with strong test coverage — the sibling-branch and dirty-subtree cases directly cover the reported symptoms.

• Collecting from clean branches (and rebuilding the root list first) is the right fix for lost/duplicated globals.
• One open question: dropping child.setLightListRefresh() in detachChild leaves the detached branch holding globals it inherited from the old root until the next light refresh. Worth confirming that's always refreshed later.
• Since lightsDirty no longer prunes the traversal, a light-dirty update now walks the whole subtree — correctness over speed is fine, but it's worth a comment so nobody re-adds the shortcut later.

Nothing blocking from my side; happy to re-review once the detach question is settled.

[toaster0123]

@jaime-jmebot The child refresh was moved, not removed: Node.java lines 488–493 calls child.setLightListRefresh() before clearing its parent. That method marks the receiver light-dirty as well as reaching the old ancestors, so the detached subtree does not rely on later reattachment to get refreshed.

The usual root geometric-state update is still needed before using its derived lists. The direct/deep detach tests update both resulting roots and assert that the detached tree has only its own global, not the sibling global it previously inherited. detachAllChildren additionally verifies that detached branches lose the old root-owned global. See tests at lines 161–179 and 246–259.

I also clarified the traversal cost in the description: a global-light rebuild replaces the complete root collection, so it must collect clean branches too. It is not a full scan on every ordinary local-light edit or unchanged frame. No timing/performance claim is made.

[jaime-jmebot]

Good — dropping the dirty-flag pruning here is the fix, but the line itself now looks like an oversight rather than a deliberate choice. The comments you added sit in updateGeometricState and detachChildAt; a one-liner here would stop someone "optimizing" this back into the lost-light bug later.

[jaime-jmebot]

Concretely:

Suggested change

	findGlobalLights(child, list);
	// No dirty-flag pruning: a global collection rebuilds the root's
	// complete list, so clean branches must contribute too.
	findGlobalLights(child, list);

Same behaviour, plus a note for whoever reads this next.

[toaster0123]

Added that explanation directly at the recursive call in 79f74e8. This is a comment-only change: Node's compiled bytecode without debug metadata is identical before and after, and the unchanged 45-case lifecycle suite still passes.

[jaime-jmebot]

Confirmed in the diff — the explanation sits right at the recursive call and says the right thing:

// No dirty-flag pruning: a global collection rebuilds the root's
// complete list, so clean branches must contribute too.
findGlobalLights(child, list);

That's a good spot for it, since it's the exact line someone would be tempted to re-guard with RF_GLOBAL_LIGHTS.

One small note on the "identical bytecode" check: comments are dropped by the compiler either way, so that's expected rather than surprising — but verifying the suite still passes is the part that matters, and you did that. All three of my earlier threads are now closed. Nice work.

[jaime-jmebot]

Checked the diff against the file — that settles it for me. detachChildAt calls child.setLightListRefresh() before clearing child.parent, and that method marks the receiver dirty itself, so the detached subtree gets corrected and isn't relying on later reattachment. Both concerns from my last pass are closed, and the findGlobalLights traversal is now commented so the shortcut doesn't creep back in.

Nothing blocking from my side — nice work on the test coverage around the detach and reparenting cases.

• Detach path verified: marking happens while the old parent is still reachable, so the old root drops the departing subtree's globals.
• Traversal cost rationale noted in-code; agreed it isn't a full scan on ordinary local edits or unchanged frames.
• The unchanged-frame instrumentation stays framed as a fast-path guard — agreed, with the list-content assertions as the real contract.

[jaime-jmebot]

Replied in the existing thread — acknowledged the clarification that the getLocalLightList() counter is an intentional fast-path guard for the unchanged-frame early return, with correctness already covered by the list-content assertions. No new findings, so no additional review submitted.

[jaime-jmebot]

All of my earlier threads are resolved — verified against the current diff.

• Detach path: detachChildAt marks the child's light state while the old parent is still reachable, so the old root drops the departing subtree's globals and the detached branch doesn't depend on later reattachment.
• Traversal cost: the rationale is now documented at the recursive call in findGlobalLights, which is the spot someone would be tempted to re-add the dirty-flag shortcut to. Good placement.
• Unchanged-frame guard: understood as an intentional fast-path assertion, with the list-content assertions carrying the actual correctness contract.

One note: no check runs are registered on 79f74e8 yet, so I haven't seen CI verify the new commit — worth letting it finish before merging.

Approving.