[toaster0123]

When a geometry inside an InstancedNode moves or its model bound changes, the rendered instance can move while its group's bounding volume stays at the old position. Light filtering can then discard nearby point or spot lights incorrectly.

Changes

Source-bound invalidation now notifies the associated GeometryGroupNode through a new onBoundChange() callback. Its default implementation is a no-op, preserving the existing behavior of other geometry-group implementations.

InstancedNode uses its existing geometry-to-group map to mark only the affected InstancedGeometry's bound dirty. The normal scene-update traversal or lazy getWorldBound() path then refreshes that group when needed. This replaces the unconditional refresh of every group and avoids rebuilding groups twice after whole-node transforms or membership changes.

Changes are coalesced within each group, unchanged groups remain cached, and moving a BatchHint.Never child does not rebuild instanced bounds. The notification covers model-bound-only changes as well as transforms, including when batches precede their source geometries.

Regression coverage

The original seven correctness tests are retained. The new tests count aggregate-bound rebuilds to detect redundant work. Nine additional tests cover cached groups, coalesced source changes, whole-node movement, uninstanced children, adding/removing an instance, bound-only invalidation, model-bound changes, direct batch-bound queries, and reordered children.

Validation

• All 16 focused instancing tests pass with assertions enabled.
• Negative control: the original PR implementation fails eight of the nine new tests; the unchanged-scene case and all seven original tests still pass.
• All 691 core main/plugins/tools Java sources compile with --release 8.
• The normal Gradle 9.7.1 build with the configured Adoptium toolchains compiled core Java/Groovy tests and required desktop/plugins modules. The full core run reports 499 tests: 497 passed, one skipped, and one external-resource failure in TestLocators.testManyLocators().
• That locator failure was reproduced using the original PR production classes. The test JVM initially could not resolve the external hosts; forwarding the environment's proxy exposed a proxy-certificate truststore failure instead. The same single test fails on both versions.

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

[jaime-jmebot]

Everything looks good to me!

• The onBoundChange() callback cleanly dirty-marks only the affected InstancedGeometry bounds, keeping bound updates lazy and avoiding redundant rebuilds.
• The 16 instancing test cases provide solid regression coverage for node movements, transform updates, and model-bound changes.