BatchedMesh: Implemented optimize, deleteGeometry and deleteInstance …

[Shakhriddin]

Related issue: #28462

Description
In this PR implemented deleteGeometry, deleteInstance, deleteAllInstances and optimize functions in the BatchedMesh class. Now you can delete the geometry or instance and then pack the data using the optimization function for further use. This helps eliminate gaps in geometry and instance data and remove unnecessary data. This makes it much easier to use BatchedMesh. The packing data in optimize function without re-creating copy of data.

To achieve this we had to change a lot of things, I will try describe them all below. This is my first PR, this PR is not completely ready, I needed a feedback, so I decided to draw up a PR draft.

• Separate Index from ID in the geometries and instances. Because when delete something we have mismatch with index. To solve use geometryId for geometries and instanceId for instances.

• Due to the fact that ID is now used, I had to replace _reservedRanges, _drawRanges, _bounds from a bracket array with Map for quick access by ID. Because find() method is slower than Map's get(), since the class does not iterate over these arrays, in this case I think it is better to use Map. If Map is not suitable, I can replace everything with a bracket array, but it will work slowly.

• For _drawInfo, I left a bracket array, because there is a lot of iteration over this array. As I know, iteration over Map is slower than iteration over a regular array. So now the instanceId is used for the instance so that there are no problems with matrices, I added another DrawIndex for the instance. This helps to get the instance matrix much faster.

Usage

const mesh = new BatchedMesh( 100, 300, 300 );

// If set true, optimize called after deleting geometry or instance
mesh.geometryAutoOptimize = false;

// add new geometry and one instance of a sphere and cube geometry each
const id0 = mesh.addGeometry( sphereGeometry );
mesh.setMatrixAt( id0, ... );

const id1 = mesh.addGeometry( boxGeometry );
mesh.setMatrixAt( id1, ... );

// add new instances based on the above items
const id2 = mesh.addInstance( id0 );
mesh.setMatrixAt( id2, ... );

const id3 = mesh.addInstance( id1 );
mesh.setMatrixAt( id3, ... );

// delete instance of sphere geometry
mesh.deleteInstance( id2 );

// delete sphere geometry data 
mesh.deleteGeometry( id0 );

// call optimize to pack the data
mesh.optimize();

API Changes

deleteGeometry( geometryId )
Deletes all data of this geometry from attributes and index of the batched mesh geometry. And delete all instances of this geometry. After needs to call optimize to pack the data.

deleteInstance( instanceId )
Deletes the instance data from matrices and colors if colors defined. After it needs to call optimize to update matrices and colors.

optimize()
This function packs and delete gaps the geometry data of the batched mesh. After updates matrices and colors texture and doing re-indexing any instance. This implemented using Array.copyWithin() to get rid of gaps by moving data without creating a copy of data.

Screenshots

Here I delete conus and sphere geometries, keep only box geometry. The screenshots shows how geometry can be optimized after optimize

Position attribute before optimize:

Position attribute after optimize:

Index before optimize:

Index after optimize

I need to get feedback about:

• Is It okay using Map instead bracket array?
• Does anyone have any ideas to do this differently?

As soon as I get the feedback, I will try to create a new PR and a documentation for this.
I tried to describe as best I could.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
679.4 kB (168.3 kB)	682.7 kB (169.1 kB)	+3.31 kB

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
457.4 kB (110.4 kB)	457.4 kB (110.4 kB)	+0 B

[Shakhriddin]

@mrdoob, @Mugen87, @gkjohnson
What do you think about this implementation of "optimize"?

[Mugen87]

TBH, I'm not much in favor of this change. BatchedMesh became a very complex component in the past months and this PR makes it even more complex. To me, the existing API of BatchedMesh is sufficient and addresses most use cases. Yes, you get more flexibility with this PR and in certain use cases more performance, but maintainability is also an important aspect as well and it seems it's suffering from this rewrite.

So I don't think we should accommodate use cases where BatchedMesh is used for many delete and insert operations. It's more common to setup the batch once and then transform/update geometries/instances or control their visibility. I would focus on that.

[gkjohnson]

maintainability is also an important aspect as well and it seems it's suffering from this rewrite.

I agree with this but I think a lot the apparent complexity is coming from so many changes happening in a single PR. I'd expect most of the added complexity to be from the addition of the "optimize" function. I do think we need more concrete use cases for this, though, before adding it. But generally I think it's best to make progressive changes towards something like this so it's easier to understand what's changing.

Even so, here couple quick thoughts on this from skimming the code:

• We shouldn't be exposing "drawIndex" as a third id that a user can provide (and will never have access to otherwise)
• Functions like setColorAt should not be slower when providing an instance id due to having to iterate over every item. This will significantly slow down use cases of updating items per frame. We'd need to store and maintain a two-way mapping between instance id and internal draw index.
• Functions and flags like deleteAllInstances and geometryAutoOptimize I don't think are needed.

But again - I think we should lay out what's this would be used for in practice and what's not possible right now before expanding on this too much more.

[Shakhriddin]

TBH, I'm not much in favor of this change. BatchedMesh became a very complex component in the past months and this PR makes it even more complex. To me, the existing API of BatchedMesh is sufficient and addresses most use cases. Yes, you get more flexibility with this PR and in certain use cases more performance, but maintainability is also an important aspect as well and it seems it's suffering from this rewrite.

I understand that in this PR there are a lot of changes and that’s why it’s a PR draft, the maintainability of the code can be made better, I can also improve it so that the use of the BatchedMesh class does not change.

So I don't think we should accommodate use cases where BatchedMesh is used for many delete and insert operations. It's more common to setup the batch once and then transform/update geometries/instances or control their visibility. I would focus on that.

I did all this based on the fact that we were talking about the “optimize” function and in the code in the comments it was written that we need to call this function to pack the data. Here I don’t really agree with the idea that you just need to manage "visibility" because I think that after deleting the geometry or instance it should not be stored.

[Shakhriddin]

• We shouldn't be exposing "drawIndex" as a third id that a user can provide (and will never have access to otherwise)

If there is no drawIndex there will be problems after optimize, here I can make the user work only with instanceId and inside the class BatchedMesh using this instanceId I can get DrawIndex without losing the performance.

• Functions like setColorAt should not be slower when providing an instance id due to having to iterate over every item. This will significantly slow down use cases of updating items per frame. We'd need to store and maintain a two-way mapping between instance id and internal draw index.

I agree with this, I have an idea on how to do this without losing performance. One of these days I'll push the changes.

• Functions and flags like deleteAllInstances and geometryAutoOptimize I don't think are needed.

I just added these things for convenience, they can be easily removed, they are not so critically important.

But again - I think we should lay out what's this would be used for in practice and what's not possible right now before expanding on this too much more.

In my project I use Tile system, I previously had problems with InstancedMesh, since I cannot delete unnecessary instances and add new places to deleted ones. It is very difficult to keep track of which indexes have become free. Now with BatchedMesh I will have the same problems, because I need to add geometry and at some point I need to delete it to free up space. This is necessary because next you will need to add new geometry.

[Shakhriddin]

Guys, thank you for your response, I thought it would be more useful to implement the optimization function because I worked on a similar case on my project and I decided to share. If you want to simple version of optimize function and with fewer changes, you can suggest how it should be. Based on this, I will try to implement it. Again, if necessary.

@gkjohnson, @Mugen87

[donmccurdy]

Having deleteInstance may be important for use cases like #28456 (comment). It'd be OK if we had to allocate instances up front and can't exceed that limit later, similar to InstancedMesh, but I do think we should be able to reduce the number of drawn instances without (for example) manually moving things out of the camera frustum.

I don't have much preference beyond that. Immutability can be a useful characteristic in an API, and if we feel that it shouldn't be possible to do removeGeometry, for simplicity and optimization, I have no objection. We could instead add a .toOptimized() method to create a new BatchedMesh, removing unused geometry.

[Shakhriddin]

I don't have much preference beyond that. Immutability can be a useful characteristic in an API, and if we feel that it shouldn't be possible to do removeGeometry, for simplicity and optimization, I have no objection. We could instead add a .toOptimized() method to create a new BatchedMesh, removing unused geometry.

I'm not really sure that it's better that BatchedMesh becomes similar to InstancedMesh. To increase the number of instances, recreate BatchedMesh with the same geometry, I think this is not correct and if some instances were deleted there, even more so.

[Shakhriddin]

I am not insisting that they accept this PR, simply based on the fact that I think it is necessary to implement optimize, I proposed this option. Here the main idea is to separate indexing from identification, so that there are no problems and no mismatches after optimize and use Array.copyWithin() to pack the data and remove gaps from the geometry.

@gkjohnson, @donmccurdy, @Mugen87

[Shakhriddin]

• We shouldn't be exposing "drawIndex" as a third id that a user can provide (and will never have access to otherwise)
• Functions like setColorAt should not be slower when providing an instance id due to having to iterate over every item. This will significantly slow down use cases of updating items per frame. We'd need to store and maintain a two-way mapping between instance id and internal draw index.
• Functions and flags like deleteAllInstances and geometryAutoOptimize I don't think are needed.

I removed third drawIndex argument in the setColor and setMatrix functions, now there will be no performance drop during animation. I also removed the method deleteAllInstances and geometryAutoOptimize property. You will also run the mesh/batch and you will see how the BatchedMesh changes when the instance count changes. There the BatchedMesh created on maxCount and the instances are just removed or added.

@gkjohnson

[gkjohnson]

If there is no drawIndex there will be problems after optimize, here I can make the user work only with instanceId and inside the class BatchedMesh using this instanceId I can get DrawIndex without losing the performance.

Yes I mean that drawIndex shouldn't be user facing, which I think had been exposed as an optional function argument in a few places.

In my project I use Tile system, I previously had problems with InstancedMesh, since I cannot delete unnecessary instances and add new places to deleted ones.

These are the kind use cases that I think are good to elaborate on in more detail. The maintainers of this project are not familiar with your use case so it's up to you to make sure we understand why the currently implementation isn't adequate, what's not currently possible, and what issues it's causing.

@donmccurdy

It'd be OK if we had to allocate instances up front and can't exceed that limit later, similar to InstancedMesh

I think having to shift all colors and matrices when you want to "remove" an instance in the middle of the list is a bit unnecessarily complicated an, in my opinion, an artifact of the underlying InstancedMesh implementation. BatchedMesh is more complicated but generally a significantly more powerful class with support for object sorting and frustum culling. The underlying implementation here would more easily enable removing intermediate elements.

We could instead add a .toOptimized() method to create a new BatchedMesh, removing unused geometry.

Even if we did this the new BatchedMesh instance would still be expected to respect the previously provided geometryId and instanceIds. If we're going to look at a solution to "repacking" the batched mesh I think an "optimize" function that retains id references is a more reasonable solution to consider.

[Shakhriddin]

Yes I mean that drawIndex shouldn't be user facing, which I think had been exposed as an optional function argument in a few places.

Yes, I fixed it, removed all drawIndex arguments on methods.

Even if we did this the new BatchedMesh instance would still be expected to respect the previously provided geometryId and instanceIds. If we're going to look at a solution to "repacking" the batched mesh I think an "optimize" function that retains id references is a more reasonable solution to consider.

I agree with you here. I thought the same thing and implemented this optimize option.
Anyway, I finished this PR, if you need you merge it.

@gkjohnson

[donmccurdy]

It'd be OK if we had to allocate instances up front and can't exceed that limit later, similar to InstancedMesh ...

I think having to shift all colors and matrices when you want to "remove" an instance in the middle of the list is a bit unnecessarily complicated ...

Yes, I would like for BatchedMesh to allow deleting instances, i.e. returning a previously-used instance from the maxInstanceCount pool. There will still be an upper limit on instance count, allocated up front, assuming that makes code simpler and/or performance more predictable.

@gkjohnson I'm happy with whatever you prefer here. 👍🏻

[Makio64]

Hi guys,

Here a use case for deleteInstance / updateGeometry I'm facing :

I'm currently using BatchedMesh to create new rooms in a dungeon game.
My BatchedMesh is about 200 geometries with 2000 instances. When the user go to a new room, i would like to use the "old" instance from previous room to create unique atmosphere, for exemple a room full of skullhead and another one with a lot of weapons.

In this case I can't really only toggle visibility cause each rooms needs the 2000 instances to use a different set of geometry. Also it didnt seems right to recreate a new BatchedMesh each time as I already have everything loaded and ready in the gpu.

For this case I guest the best would be one of those, the first one being my favorite:

• updateGeometry(instanceID, geometryID)
• removeInstance(instanceID) + addInstance(instanceID) + update

I think this case come often when BatchedMesh is use to create large generative environment in game as I face the same problem in a runner game too.

[Shakhriddin]

I agree with you @Makio64. I have already improved BatchedMesh class in my project, except deleteGeometry, deleteInstance and optimize functions custom BatchedMesh class can change the overall size of the geometry if it is filled and increases the instance if it reaches the maximum