Skip to content

Keep global lights consistent when scene branches change #2997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
toaster0123 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Keep global lights consistent when scene branches change #2997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dfbc0c5
Keep global lights consistent across scene graph changes
toaster0123 Oct 2, 2026
79f74e8
Explain complete global-light collection during rebuild
toaster0123 Oct 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions jme3-core/src/main/java/com/jme3/scene/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,9 @@ private void findGlobalLights(Spatial sp, LightList list) {
List<Spatial> children = n.getChildren();
for (int i = 0; i < children.size(); i++) {
Spatial child = children.get(i);
if ((child.refreshFlags & RF_GLOBAL_LIGHTS)!= 0) {
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);

@jaime-jmebot jaime-jmebot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jaime-jmebot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 toaster0123 Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jaime-jmebot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand All @@ -284,11 +284,13 @@ public void updateGeometricState() {
// This branch has no geometric state that requires updates.
return;
}
if ((refreshFlags & RF_LIGHTLIST) != 0) {
boolean updateGlobalLights = (refreshFlags & RF_GLOBAL_LIGHTS) != 0;
if ((refreshFlags & RF_LIGHTLIST) != 0 || (updateGlobalLights && parent == null)) {
// Global collection replaces the root's previous list, even when
// the global refresh came from an already-dirty descendant.
updateWorldLightList();
}

boolean updateGlobalLights = (refreshFlags & RF_GLOBAL_LIGHTS) != 0;
if (updateGlobalLights){
// if root node, we collect the global lights
if (getParent() == null){
Expand Down Expand Up @@ -487,6 +489,9 @@ public Spatial detachChildAt(int index) {
assert SceneGraphThreadWarden.assertOnCorrectThread(this);
Spatial child = children.remove(index);
if (child != null) {
// Refresh while the old parent is still reachable, so global lights
// anywhere in the detached subtree also invalidate the old root.
child.setLightListRefresh();
child.setParent(null);
logger.log(Level.FINE, "{0}: Child removed.", this);

Expand All @@ -498,8 +503,6 @@ public Spatial detachChildAt(int index) {
// XXX: Not necessary? Since child will have transform updated
// when attached anyway.
child.setTransformRefresh();
// lights are also inherited from parent
child.setLightListRefresh();
child.setMatParamOverrideRefresh();

invalidateUpdateList();
Expand Down
Loading