Refresh instanced bounds after source geometry updates

[toaster0123]

When a geometry inside an InstancedNode moves, the rendered instance moves but its group's bounding box can stay at the old position. This can make nearby point or spot lights disappear because light filtering still uses the old bounds.

The scene-update traversal updates source children first. This override then refreshes the instanced groups before Node merges their bounds into its own bound. The lazy getWorldBound() path uses the same order.

Seven regression tests cover moving an instance, changing child order, moving a nested parent, updating an InstancedNode through an outer root, changing only the model bound, querying the node bounds, and keeping a point light after an instance moves into its range. All seven fail before the fix and pass afterward.

Validation: the current head passes the full core checks: 490 tests, no failures, one existing skip. The original core build (including Javadoc), plus desktop, effects and plugins checks also passed: 563 tests, no failures, two existing skips. Independent lifecycle review also covered multiple instances, adding/removing instances, mesh/material changes, transforms and Cloner copies.

[jaime-jmebot]

Quick coverage check on this new override: it only catches calls that go through the public no-arg updateWorldBound(). If Spatial still has the internal updateWorldBound(boolean recurse) entry point (used when a parent node walks its children), that call skips this override, so the groups could keep their stale bounds on that path.

Overriding the protected variant instead would cover both, since the public entry point delegates to it:

    @Override
    protected void updateWorldBound(boolean recurse) {
        for (InstancedGeometry ig : instancesMap.values()) {
            ig.updateWorldBound();
        }
        super.updateWorldBound(recurse);
    }

Not a blocker, just worth confirming which entry points actually reach this code in a normal frame.

[toaster0123]

I checked the current base and PR head: updateWorldBound() is already the protected override, and there is no updateWorldBound(boolean) overload in Spatial or Node.

Both paths reach it after updating the source children: Node.updateGeometricState() and the lazy Spatial.checkDoBoundUpdate(). The override then refreshes the instance groups before super.updateWorldBound() merges their bounds into the node bound.

The existing nested test covers a parent inside the InstancedNode. I also added the separate case you suggested, with the InstancedNode under an outer root and updates driven through that root. It fails on the original code and passes with this override. I’m running the full core checks before pushing that test and clarifying the PR wording.

[toaster0123]

The outer-root regression is now in f16b477. All seven bounds cases fail on the original code and pass with the fix; the full core checks also pass (490 tests, one existing skip). I clarified the PR description to distinguish updating children from merging their bounds. No production change was needed.

[jaime-jmebot]

Thanks — that clears it up on my end. I had a stale mental picture of an updateWorldBound(boolean recurse) variant; it doesn't exist in Spatial or Node in the current base, so this override really is the single hook and both paths (the Node.updateGeometricState() traversal and the lazy Spatial.checkDoBoundUpdate() bound query) funnel through it.

The outer-root test is a good addition — driving the update through a parent node is exactly the path I was unsure about, and the description wording now matches the actual ordering (refresh groups, then merge into the node bound).

One note: no CI checks are reported on f16b477 yet on my side. Once the core checks come back green on that SHA I'll approve. The code and the seven regression cases look right to me.

[jaime-jmebot]

Following up on my last note: GitHub isn't reporting any check runs for f16b477 (the combined status is still pending with zero checks), so waiting on CI here wouldn't tell either of us anything new — it may simply not have been kicked off for this branch yet.

Since my one substantive concern turned out to be a wrong assumption about the API, and the code plus the seven regression cases hold up to review, I'm going ahead and approving. Happy to revisit if a core run comes back red later.

[jaime-jmebot]

Nice and focused fix — refreshing each instanced group's bound right before the node aggregates its own bound is the right hook, and it makes light/culling filtering follow the moved geometry again.

A few small things:

• Which entry point is covered? Left an inline note: the new override only catches the public no-arg updateWorldBound(), so it's worth confirming the internal updateWorldBound(boolean recurse) path (used when a parent walks its children) also ends up refreshing the groups.
• Description wording: the PR text says the refresh happens "after the source children have been updated", but the code refreshes the groups before super.updateWorldBound(). Worth aligning the wording so future readers don't get confused about the ordering.
• Tests: good coverage for a regression suite (move, reordering, nested parent, model-bound-only change, direct getWorldBound(), point-light survival). If you get a chance, one test with the InstancedNode nested under another node would pin down the entry-point question above.

Overall this looks good to me; just the entry-point check and a docs tweak.

[jaime-jmebot]

I checked the claim rather than taking it at face value, and the author is correct — my earlier inline note was based on a wrong assumption.

What I found in the current base:

• Spatial and Node have no updateWorldBound(boolean recurse) overload, so there is no second entry point being missed.
• The new method in InstancedNode is the protected no-arg updateWorldBound() override, which is the single hook both paths reach: the Node.updateGeometricState() traversal and the lazy Spatial.checkDoBoundUpdate() bound query.
• The newly added outer-root test drives updates through a parent node, which is precisely the entry-point scenario I flagged — that closes the gap.

Since my one substantive concern turned out to be unfounded, there was nothing left to request changes on. I replied in the existing thread acknowledging the correction rather than opening a new one.

I left the review at COMMENT rather than APPROVE for one honest reason: CI reports zero checks on f16b477b0b26cb13d80dd73751488fb58284da2b. The PR body cites 490 passing core tests, but I could not independently verify that on this SHA — no check runs have been reported. Rather than approve based on self-reported numbers, I told the author I'll approve once the core checks come back green on that commit.

One process note worth flagging: my first inline comment asserted a specific API (updateWorldBound(boolean recurse)) that doesn't exist. Speculating about method signatures from memory instead of reading the file first is what caused it, and I should have read Spatial/Node before commenting.

[jaime-jmebot]

Approving — the fix and the regression coverage both check out.

• The 11-line override in InstancedNode is in the right place: each instanced group's bound is refreshed right before super.updateWorldBound() merges it into the node bound, which is what makes light/culling filtering follow a moved source geometry again.
• Both update paths (the Node.updateGeometricState() traversal and the lazy Spatial.checkDoBoundUpdate() bound query) funnel through this single hook, and the new outer-root test drives updates through a parent node, so that case is pinned down too.
• Seven regression tests covering move, reorder, nested parent, outer-root update, model-bound-only change, direct getWorldBound(), and point-light survival — a solid suite for a bug like this.

Thanks for the clean follow-ups and for correcting my earlier note.