Initialize loaded scene transforms, lights and material overrides

[toaster0123]

Dependency and review scope

This is stacked on #2997, at commit dfbc0c5a3407fa3f7a1f1290568a2068c28716dd. The upstream PR targets master, so its full diff includes that dependency. Review only the new initialization change here: commit 5775cb4.

The dependency matters: correctly restoring pending global-light state exposes the dirty-host attachment bug fixed by #2997. The initialization-only change does not pass that control without #2997. This branch tests both together and does not modify the #2997 branch or either repository's master.

Problem

A binary-loaded scene can retain its saved local values while its world state stays wrong after the normal scene update.

For example, save a root translated by (3, 4, 5), with a child translated by (1, 0, 0), lights, and a material override. Load it as a standalone scene root and update it. The local values are present, but the world translations can remain (0, 0, 0), the world light lists can be empty, and descendants can miss the inherited override. Bounds derived from those transforms can also be wrong.

Attaching the loaded scene to another root often hides the problem because attachment refreshes state. A loaded root should also work on its own.

Fix

The change is limited to initialization during reading:

• Mark derived transforms, bounds, light lists, and material overrides for refresh after restoring local state
• Detect loaded local global lights and carry that pending discovery upward in the existing child-link loop
• Invalidate transforms/bounds consumed by a control's lazy getter while its subtree was temporarily parentless, once the actual parent is linked

That last step uses a private helper which stops at already-dirty branches. It avoids overridable refresh callbacks on partially loaded subclasses. It ensures a temporary read-time calculation cannot survive as stale state after the final ordinary update; it does not promise final ancestor-relative coordinates in the middle of reading.

No serialized fields, file format, public API, or per-frame update traversal are changed by this initialization commit.

Validation

The same 48 new regression cases were replayed against exact source variants:

• Original upstream and Keep global lights consistent when scene branches change #2997 alone: 44 failures, 4 passes
• Initialization without Keep global lights consistent when scene branches change #2997: 47 passes, 1 failure in the explicit dirty-host dependency control
• Final stack: 48/48 pass, plus all 29 inherited Keep global lights consistent when scene branches change #2997 cases

Coverage includes standalone and attached roots, nested lights, persisted enabled/global flags, ordinary inheritance, public light-filter output, transforms/scale/rotation/bounds, override inheritance and sibling isolation, cloning, repeat roundtrips, read-time control getters, and unchanged updates.

Core build and desktop/effects/plugins/terrain checks pass: 639 reported tests, zero failures/errors, two existing/environment skips. Both new test files have zero Checkstyle warnings; the affected production files add no diagnostics.

Independent verification passes 185 cases, including preserved baseline-generated binary fixtures and lifecycle controls. A separate source review and 36 additional analytic bound-getter cases found no blocker.

Limits

This verifies CPU scene-derived state and binary persistence. It is not a GPU, pixel, shader, XML/all-loader, concurrency, or performance certification. Depth and unchanged-update controls check specific work counts, not general throughput. Arbitrary full scene updates or graph mutations during custom deserialization are not covered.

The two scoped-test skips are the existing FastMath counterclockwise test and a desktop test requiring a writable noexec filesystem. The whole-repository local Android build remains outside these checks because the Android SDK is unavailable.

[jaime-jmebot]

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]

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]

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.

[jaime-jmebot]

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]

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]

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.

[jaime-jmebot]

Really solid work — the change stays inside deserialization, avoids the overridable-refresh trap on partially loaded subclasses, and the test coverage (standalone vs. attached roots, nested lights, overrides, roundtrips) maps well onto the bug it fixes.

Two things before this lands:

• invalidateReadTransform short-circuits on RF_TRANSFORM being set, which hides the entire subtree below that node. Because Spatial.read now marks every loaded spatial dirty, the early return hits the common case, not an edge case — and a dirty flag on the parent says nothing about its children. Concrete gap: a control on node C calls getWorldBound() during its own read; that merges the children and clears their RF_BOUND while C's RF_TRANSFORM stays set. When C's parent later links it, the helper returns at C and never re-marks the children, whose bounds were computed against C's not-yet-known world transform. The next ordinary update only recomputes what is still dirty, so the wrong bound sticks. Walking the whole subtree unconditionally fixes it and costs O(subtree) once per load — cheap next to reading the graph:

private static void invalidateReadTransform(Spatial spatial) {
    spatial.refreshFlags |= RF_TRANSFORM | RF_BOUND;
    if (spatial instanceof Node) {
        for (Spatial child : ((Node) spatial).children.getArray()) {
            invalidateReadTransform(child);
        }
    }
}

If you'd rather keep the short-circuit for performance, an explicit note in the comment about that limitation would help the next reader.

• The pending global-light discovery is a transient flag (left a note on the RF_GLOBAL_LIGHTS line in Spatial.java) — a read-time world-light query consumes it before Node.read can forward it upward, so the ancestor chain stays unmarked. Marking light state in the read-time invalidation too would close it.

Otherwise: no serialized format, public API, or per-frame traversal changes, which keeps the risk profile nice and small. Happy to take another pass once these two are addressed.

[toaster0123]

@jaime-jmebot I replayed the concrete getWorldBound case from the review against exact head 5775cb4 and could not reproduce the proposed flag state.

getWorldBound calls checkDoBoundUpdate, which updates the queried node's transform before visiting child bounds. In the targeted read-time control, C's transform/bound flags change from dirty/dirty to clean/clean. When its parent subsequently links C, the helper therefore enters C and re-invalidates the consumed subtree. The final analytic bounds are correct.

The targeted case passes, as do all 36 independent bound-getter variants and the existing 185-case replay. The light-query point is addressed in its inline thread: neither the getter nor the protected light-list updater consumes RF_GLOBAL_LIGHTS.

I have left the production code unchanged. Unconditionally walking every linked subtree would also repeat the walk at each ancestor link, rather than once per load; for a chain, the visits would grow as the sum of subtree sizes. If there is another concrete getter sequence that produces a dirty parent with consumed descendants, please share it so I can test that exact sequence.

[jaime-jmebot]

Thanks for replaying the getWorldBound case — and yes, on the light thread I'm satisfied, your flag reading matches what I see in the code now.

Here's the one concrete getter sequence I'd like replayed, since your walk explanation and the short-circuit trade-off both hinge on it. It's the descendant variant: the read-time getter is pointed at the grandchild, not at the controlled node itself.

• Structure at save time: P → C → D, all Nodes, with a control on C whose read calls getSpatial().getChild(0).getWorldBound() (the "size myself to my child" pattern, which is a realistic read-time getter).
• Expected flag state after deserialization, by my reading:
  • Spatial.read marks C and D dirty (RF_TRANSFORM | RF_BOUND).
  • The control query hits D.getWorldBound(), which consumes D's own flags and evaluates its bound against C's current (still pre-link) world transform.
  • C keeps RF_TRANSFORM/RF_BOUND — nobody queried it.
  • Node.read for P links C, the helper enters at C, sees RF_TRANSFORM set, and returns.
• What I'd like to see: the bound of D after the final ordinary update, not just C's or P's. The root's refreshFromParents()-style flags don't currently reach that bound as far as I can tell, so the number computed in pre-link space would stay there.

Useful variants if the exact reproduction is fussy: point the control's getter at the grandchild and give D a non-identity local translation so a stale bound can't coincide with the correct one; and log D's worldBound plus C's transform right after linking and again after the final update.

To be clear about what settles this: if that sequence reproduces, the fix is the unconditional subtree walk; if it doesn't, I'll take the short-circuit as-is as long as the limitation is written down where the next reader will hit it, rather than rediscovering it from a wrong bound.

[jaime-jmebot]

Replied to the thread — agreed with the author that the null-deref point is a non-issue (the pre-existing setOwner already dereferences), so I dropped it and pointed back at the one item still open: replaying the P → C → D descendant case against invalidateReadTransform's short-circuit.

[jaime-jmebot]

Thanks for the replay — and the key difference is that your case queries the controlled node itself, whereas the state I need comes from the control pointing at a descendant. Nobody queries C in that variant, so C keeps RF_TRANSFORM/RF_BOUND and the short-circuit returns before it ever reaches D.

Concrete sequence (the P → C → D one from my earlier comment here, with exact values so it can be asserted rather than eyeballed):

Save-time graph — all Nodes, P → C → D

• P.localTranslation = (3,4,5), C.localTranslation = (2,0,0), D.localTranslation = (1,0,0)
• D.modelBound = BoundingSphere(center (0,0,0), radius 1)
• Control on C (not P): read() → getSpatial().getChild(0).getWorldBound() — the "size myself against my child" read-time getter

Flag path through Node.read / Spatial.read

1. D's Spatial.read marks it RF_TRANSFORM|RF_BOUND (plus light/override flags).
2. C's child-link loop links D → invalidateReadTransform(D): D is dirty → short-circuit. Harmless.
3. C's Spatial.read reads the control; the control calls D.getWorldBound(), which consumes D's RF_TRANSFORM and RF_BOUND and evaluates against C.worldTransform — still identity, because C has never been updated.
4. Only afterwards does C's Spatial.read mark C dirty. So C leaves the read with RF_TRANSFORM|RF_BOUND set.
5. P's child-link loop calls invalidateReadTransform(C) → C is dirty → early return. D is never re-marked.
6. Final ordinary root.updateGeometricState(): P and C are dirty so they refresh, but Node.updateGeometricState only calls child.updateWorldTransform(true) when RF_TRANSFORM is set, so clean D keeps the transform and bound computed in step 3.

What I'd like asserted after the final update

check	expected	with the short-circuit
C.getWorldTransform().getTranslation()	(5,4,5)	(5,4,5) ✅
D.getWorldTransform().getTranslation()	(6,4,5)	(1,0,0)
D.getWorldBound().getCenter()	(6,4,5)	(1,0,0)
C.getWorldBound().getCenter()	merged around (6,4,5)	merged around (1,0,0)

So the assertion that settles it is the grandchild's transform and bound after the final update — the root's and C's flags come out right either way. Non-identity translations on all three levels keep a stale value from coinciding with the correct one, and it also confirms the log points I suggested: D.worldBound + C transform right after linking and again after the update.

On the walk cost — agreed it repeats per ancestor link, and for a chain the visits grow as the sum of subtree sizes. Practically it's a couple of field writes per loaded spatial on the deserialization path only, so a 1000-deep chain is ~500k trivial visits (sub-millisecond) against a file read doing far more work per node. I don't think that's worth a correctness hole, so I'd take the unconditional walk. Worth noting for the design, though: a "this subtree was fully read" marker can't be tracked per-node to prune this, because the consumer that clears the flags can be an ancestor's getter — so the signal has to travel down from the parent, not up from the child.

If the assertions above come out clean on 5775cb4, I'll take the short-circuit as-is — but then the limitation belongs in the helper's Javadoc, since it produces a wrong bound rather than a merely un-refreshed one. Either way, that settles it from here.

[jaime-jmebot]

The light point is settled — thanks for replaying the flag lifecycles rather than just reading the source, and for the targeted getWorldBound case.

• Your replay queries the controlled node itself, which is why the helper re-enters it. The state I need comes from a control pointing at a descendant (P → C → D, control on C reading D.getWorldBound()): C is never queried, so it leaves the read dirty and the short-circuit returns before reaching D, whose transform and bound were consumed in pre-link space.
• I've posted that sequence with exact save-time values and the expected vs. actual post-update table in a conversation comment — the decisive assertions are the grandchild's transform and bound after the final update, not the root's.
• If those come out clean on 5775cb4 I'll take the short-circuit as-is, with the limitation written into the helper's Javadoc. The unconditional walk still fixes it, and on the deserialization path its cost is negligible next to the file read.

Everything else in this PR looks good to me — the change stays inside deserialization, avoids the overridable-refresh trap on half-built subclasses, and the coverage maps onto the bug being fixed.

[toaster0123]

@jaime-jmebot Replayed the exact descendant-Node query from your comment: P → C → D are Nodes, and C's control calls ((Node) getSpatial()).getChild(0).getWorldBound() during read. D has a non-identity transform and a Box geometry below it, so its bound is non-null.

The missing step is the dirty-ancestor walk in checkDoTransformUpdate. D's query reaches temporarily parentless C and clears C's transform flag, even though C was not queried directly.

The raw-state observations are:

• After the control queries D: C transform clean / bound dirty; D transform and bound clean
• After P links C, before any further lazy getter: C and D transform/bound flags are dirty again. D still holds its cached pre-link bound, correctly marked for recomputation
• After one ordinary root update: D's own cached bound has the expected center (204, 246, 288) and extents (24, 48, 72), with both flags clear. This assertion happens before calling D's public bound getter again

For those numbers, P has translation (10,20,30), scale 2; C has (1,2,3), scale 3; D has (4,5,6), scale 4; its Box(1,2,3) child has translation (7,8,9). The pre-link bound center is (97,113,129). All eight variants pass, covering settled/unsettled saves, an additional Node level, and an untouched sibling.

I added the requested explanation beside the helper in 021fd5c. It documents this ancestor-path invariant; no executable code changed. The eight new descendant cases, 36 prior bound-getter cases, and three earlier claim tests all pass against that source.

[jaime-jmebot]

That closes it — the dirty-ancestor walk in checkDoTransformUpdate was the step I was missing. A getter on D climbs to the still-parentless C, sees C's RF_TRANSFORM set, and calls updateWorldTransform(true) on it, so C leaves the read with its transform flag clean. At link time the helper therefore re-enters C and re-invalidates the subtree, which is exactly why your grandchild assertions come out right.

So the short-circuit's invariant holds, and it's stronger than I gave it credit for: RF_TRANSFORM still being set on a node is a positive signal that nothing in its subtree was consumed, because any getter that could have consumed something below it would have had to pass through it and cleared the flag on the way.

The explanation beside the helper is what I asked for — an invariant a future reader can check in one place, rather than a limitation rediscovered from a wrong bound. Keeping the short-circuit is the right call: it's O(dirty branches) instead of O(subtree) per ancestor link, and on the deserialization path the saving is real for wide graphs.

One residual, which I read as falling under the PR's documented limits rather than a new blocker: the invariant covers the lazy getters, which walk the ancestor path. A control calling updateWorldBound()/updateWorldTransform() directly during read bypasses that walk, so it could consume a descendant's flags without clearing the dirty ancestor's transform flag — same class as the updateGeometricState()-mid-read case you already scoped out.

Approving. Nice work on the replay — the raw-state observations (flags and cached bound asserted before the public getter is touched) are what made this checkable rather than arguable.

[jaime-jmebot]

Both earlier items are resolved, and the resolution is well-evidenced.

• The invalidateReadTransform short-circuit stands. The dirty-ancestor walk in checkDoTransformUpdate is the missing link: a read-time getter on a descendant walks up through the still-parentless ancestor and clears its RF_TRANSFORM, so by link time the helper re-enters and re-invalidates the consumed subtree. A still-dirty RF_TRANSFORM is therefore a positive signal that nothing below was consumed, which makes the early return safe rather than merely optimistic.
• The RF_GLOBAL_LIGHTS point is withdrawn — the getter and the protected updater never consume that flag, so the pending-discovery signal survives a read-time query.
• The invariant is documented at the helper, which is what the next reader needs to keep the short-circuit correct when this code is touched.
• No serialized fields, file format, public API, or per-frame traversal touched, and the coverage maps directly onto the stale-scene-state bug. Good to merge.