SelectionOutlineLayer: Fix clearSelection crash with instanced meshes#18235
SelectionOutlineLayer: Fix clearSelection crash with instanced meshes#18235sebavan merged 5 commits intoBabylonJS:masterfrom
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
The bug diagnosis is solid and the fix direction (tracking source meshes in a Set) is correct. However there are three issues that should be addressed before merging.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
All three concerns from the previous review have been addressed well. The shared cleanup method is clean, the tests now exercise the real API, and _disposeMesh handles the tracking Set.
One minor edge case to be aware of (non-blocking): if multiple instances from the same source are selected and one is disposed, _disposeMesh removes the source from _instancedBufferSources immediately. When clearSelection is later called for the remaining instances, the source won't be cleaned up because it's no longer in the Set. This leaves stale (but valid) instanceSelectionId data on the source — not a crash (the storage data is still intact), but a GPU resource leak until the next addSelection or mesh disposal. A possible refinement would be to only remove the source from the Set when no more selected instances from that source remain, but this is unlikely to matter in practice.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Popov72
left a comment
Popov72
left a comment
There was a problem hiding this comment.
The edge case is now properly handled -- _disposeMesh only removes the source from tracking when no other selected instance shares it, and the new test validates this scenario. LGTM.
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18235/merge/index.html#WGZLGJ#4600 Links to test your changes to core in the published versions of the Babylon tools (does not contain changes you made to the tools themselves): https://playground.babylonjs.com/?snapshot=refs/pull/18235/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/18235/merge#BCU1XR#0 If you made changes to the sandbox or playground in this PR, additional comments will be generated soon containing links to the dev versions of those tools. |
|
Visualization tests for WebGPU |
|
WebGL2 visualization test reporter: |
|
@marns any chance you can update and merge master back in ? |
…lines # Conflicts: # packages/dev/core/src/Layers/thinSelectionOutlineLayer.ts
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
WebGL2 visualization test reporter: |
|
Visualization tests for WebGPU |
4 participants
Summary
Fix
Repro
Select a node whose descendants include instanced meshes (e.g. multiple GLB instances from the same container), then trigger a re-selection. The outline layer's clearSelection + addSelection cycle leaves stale state on the source mesh.
Test plan