Add removeSelection() to SelectionOutlineLayer

[regnaio]

Description and code entirely from Claude:

SelectionOutlineLayer has addSelection() and clearSelection() but no way to remove individual meshes from the selection. The internal _disposeMesh() does partial removal but is @internal, incomplete (leaks GPU resources for instanced meshes, doesn't clean _meshUniqueIdToSelectionId), and only handles single meshes. Users currently must clearSelection() + re-add everything, which is wasteful.

This adds removeSelection() that properly cleans up GPU resources (VBOs, VAOs, instance buffers) for individual meshes, extracted into a shared _cleanupMeshResources() helper used by both methods.

I built the snapshot babylon.js and tested that removeSelection() works in my project.

@Popov72 , @noname0310 , @Fuyutami , when you have time, could you please review this PR? I'm unsure if Claude wrote the code the way you deem best, so please feel free to edit the code in any way.

Thank you so much for your time and help!

---

If it also sounds good with you all, a future optimization I'd like to make with the help of Claude is to have a Map search instead of an array search like this._selection.indexOf(mesh)

[noname0310]

The removeSelection method has not been implemented intentionally.
This is because the instance ID continues to increase as selections are added, and in a scenario where removeSelection and addSelection are used repeatedly, the ID limit could be exceeded.

And this is just my personal opinion, but even cutting-edge models like Opus 4.6 aren’t capable of writing code that fully understands the context of Babylon.js.
That’s why reviewing code generated by LLMs is quite exhausting for the reviewer.

This is because the generated code gives the impression that it understands the context, even though it doesn’t actually understand anything, making it more confusing to read than code written by a human.

[regnaio]

Ah thank you for your explanation, @noname0310! I now understand your concern about the selection instance ID, which is incremented for every mesh or mesh group added.

My apologies for the hard-to-read diff. I tried to not duplicate code by reusing your cleanup code in clearSelection() in removeSelection(), and that caused a messy diff. I have since reverted to clearSelection() being untouched, which has made the diff easier on the eyes.

If you would be so kind to quickly glance over the diff again to see if it's in the right direction, I'd be really thankful.

---

I was just curious: If we have hundreds of meshes in the SelectionOutlineLayer and want to frequently add and remove meshes (potentially every frame), will we have to clear it every time we want to remove a mesh?

[noname0310]

_disposeMesh already provides the same value as removeSelection.

However, to use this, users must understand that empty IDs are produced as they continue to use the remove and add operations.

To hide these internal details,
introducing a removeSelection function that counts reference counts and manages free IDs seems to unnecessarily increase complexity.

[Popov72]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[noname0310]

Under normal usage, to remove a specific element from the list, you must call clearSelection and then manually add the remaining items one by one.

As mentioned earlier, using _disposeMesh allows you to exclude specific meshes. However, the user must understand that this creates free IDs, and the code should be written to handle ID overflow by calling clearSelection at an appropriate time.

Before discussing the Selection Outline, I’d like to talk about the GPU Picker.

Initially, the GPU Picker only allowed setting the picking list via setPickingList, but after reviewing the code, I realized it was possible to implement addPickingList without incurring any additional overhead.

Therefore, I modified the GPU Picker to remove the restriction that previously only allowed setting the picking list, enabling the ability to append items as well.

I applied the same constraint when creating the Selection Outline, which is why it now supports the add operation.

Essentially, the Selection Outline is designed to operate by setting a list.

---

Also, the main point of my comment about AI-generated PRs isn’t that the diff is hard to read, but simply that reading something generated by AI is tiring in itself.

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Snapshot stored with reference name:
refs/pull/18233/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18233/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18233/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/18233/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/18233/merge
https://gui.babylonjs.com/?snapshot=refs/pull/18233/merge
https://nme.babylonjs.com/?snapshot=refs/pull/18233/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/18233/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.

[bjsplat]

Interaction tests

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18233/merge/testResults/interactionplaywright/index.html

[bjsplat]

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18233/merge/testResults/webgl2playwright/index.html

[bjsplat]

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/18233/merge/testResults/webgpuplaywright/index.html

[regnaio]

Thank you so much, @noname0310 , I completely understand where you're coming from. Let me close this PR, since it does seem to add unnecessary complexity. It's great to hear that _disposeMesh() does everything needed for the mesh cleanup except handling the ID overflow.

It seems that _nextSelectionId would overflow after reaching 2^24 (by default) or 2^8 (on devices lacking half-float support)? For users rapidly adding/removing a few meshes every frame to a SelectionOutlineLayer that holds hundreds/thousands of meshes on average, is it recommended that they listen to this private field _nextSelectionId and run clearSelection() if it starts to approach this number limit?

[noname0310]

To be more precise, i expect issues to arise once the value exceeds the maximum safe integer for 32-bit floating-point numbers. In this case, the outlines of two objects that should be distinct may merge and be rendered as a single object.

Therefore, users can implement the following strategy:

By storing the selection state externally, clearing it after the “add selection” operation has been performed a certain number of times, and then resynchronizing the Selection Outline Layer’s state with the external state, the system should function without major issues.

[regnaio]

Thank you, @noname0310 , for all your explanations 😄

If I ever do a PR in the future for SelectionOutlineLayer potentially related to using a Map for _selection to optimize splice() or reusing selection IDs, it'll definitely be by me instead of AI