Skip to content

Initialize loaded scene transforms, lights and material overrides #3002

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 4 commits into
base: master
Choose a base branch
from
Open

Initialize loaded scene transforms, lights and material overrides #3002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfbc0c5
Keep global lights consistent across scene graph changes
toaster0123 Oct 2, 2026
5775cb4
Initialize derived scene state after deserialization
toaster0123 Oct 2, 2026
021fd5c
Explain read-time transform invalidation pruning
toaster0123 Oct 2, 2026
74ce32c
Remove duplicate loaded-scene binary roundtrip test
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
36 changes: 29 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,7 @@ 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);
}
findGlobalLights(child, list);
}
}
}
Expand All @@ -284,11 +282,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 +487,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 +501,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 Expand Up @@ -832,11 +833,32 @@ public void read(JmeImporter importer) throws IOException {
if (children != null) {
for (Spatial child : children.getArray()) {
child.parent = this;
invalidateReadTransform(child);
// Carry pending global-light discovery from loaded descendants toward the root.
refreshFlags |= child.refreshFlags & RF_GLOBAL_LIGHTS;
}
}
super.read(importer);
}

/**
* Invalidates transforms that a loaded control may have computed before its
* subtree acquired a parent. Lazy world-transform and bound getters update
* their dirty ancestor path first, so a still-transform-dirty branch needs
* no further traversal.
*/
private static void invalidateReadTransform(Spatial spatial) {
if ((spatial.refreshFlags & RF_TRANSFORM) != 0) {
return;
}
spatial.refreshFlags |= RF_TRANSFORM | RF_BOUND;
if (spatial instanceof Node) {
for (Spatial child : ((Node) spatial).children.getArray()) {
invalidateReadTransform(child);
}
}
}

@Override
public void setModelBound(BoundingVolume modelBound) {
assert SceneGraphThreadWarden.assertOnCorrectThread(this);
Expand Down
8 changes: 8 additions & 0 deletions jme3-core/src/main/java/com/jme3/scene/Spatial.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,14 @@ public void read(JmeImporter im) throws IOException {

localLights = (LightList) ic.readSavable("lights", null);
localLights.setOwner(this);
// Rebuild derived world state without invoking overridable refresh methods during loading.
refreshFlags |= RF_TRANSFORM | RF_BOUND | RF_LIGHTLIST | RF_MATPARAM_OVERRIDE;
for (Light light : localLights) {
Comment on lines +1722 to +1723

@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.

Nice and tidy — marking everything derived here avoids calling overridable refresh hooks on half-built subclasses. One thing to keep in mind: localLights comes straight from readSavable("lights", null), so the new for loop is the first thing that dereferences it. Everything jME writes includes lights, so this only matters for hand-built capsules — just a heads-up in case you want a guard for symmetry with the older file formats.

@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.

The preceding localLights.setOwner(this) already dereferences the result, before the new loop; that call is unchanged from the base. So this patch does not introduce the null dereference. I have kept the existing serialized-LightList contract unchanged rather than add an unrelated legacy-format behavior without a failing fixture.

@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.

Fair — and thanks for checking the surrounding line rather than taking my note at face value. localLights.setOwner(this) on the line above already dereferences it, so nothing here changes the existing contract. Dropping that point; agreed there's no reason to invent legacy-format behavior without a failing fixture.

That leaves invalidateReadTransform's short-circuit as the one open item — the P → C → D sequence is up in the PR conversation if you want a concrete sequence to replay.

if (light.isGlobal()) {
refreshFlags |= RF_GLOBAL_LIGHTS;
break;

@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.

Heads up on this flag: RF_GLOBAL_LIGHTS is transient — the first world-light-list recompute consumes it, and Node.read only forwards the flag value upward. So if a control asks for getWorldLightList() (or any updateWorldLightList) while it is being deserialized, the pending-discovery signal is already gone by the time the parent links it, and the ancestor chain never gets marked. A global light can then still be missing from world lists above, which is the same stale-state class this PR is fixing.

Cheap way to close it: have the read-time invalidation re-mark light state too (not just RF_TRANSFORM/RF_BOUND), so a read-time light query can't swallow the signal.

@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.

I checked this on 5775cb4. getWorldLightList only returns worldLights; it does not recompute anything or clear flags. The protected updateWorldLightList clears RF_LIGHTLIST, not RF_GLOBAL_LIGHTS.

I also replayed read-time controls across six getter scenarios and six separate protected-updater diagnostics (different global-light owners, settled/unsettled saves). RF_GLOBAL_LIGHTS remains pending in every case, and normal final root updates produce the expected ancestor and descendant light lists. A full updateGeometricState during partial deserialization is a different operation, outside the stated getter scenario and the limits documented in this PR.

@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.

You're right, and I'm withdrawing this one. I misread the flag lifecycles: getWorldLightList() is a pure getter, and the protected updater clears RF_LIGHTLIST, while RF_GLOBAL_LIGHTS is only consumed by the node's geometric-state update. So a read-time getter can't swallow the pending discovery — thanks for replaying six scenarios plus the protected-updater diagnostics rather than just arguing from the source.

One residual, clearly outside the stated getter scenario: a full updateGeometricState() during partial deserialization would consume RF_GLOBAL_LIGHTS on a subtree whose root is parentless. That's already documented in the PR limits, so I'm not asking for a change here.

Light point closed on my side. My remaining note is the invalidateReadTransform short-circuit — I've put a concrete getter sequence in a comment on the PR so it can be replayed.

}
}

ArrayList<MatParamOverride> localOverridesList = ic.readSavableArrayList("overrides", null);
if (localOverridesList == null) {
Expand Down
Loading