mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.69k stars 35.37k forks source link

BatchedMesh: Plans for adding a "resize" function #29464

Closed gkjohnson closed 1 month ago

gkjohnson commented 1 month ago

Description

It's not always possible to know how much memory is required for a scene up front and you may need to expand the buffer geometry attributes to store more data (or shrink them). Similar to #29463, this could relevant for games, simulation visualizations, editors, or 3d tiles rendering.

Solution

Add support for a "resizeGeometry" and "resizeInstances" functions that takes a new index size, attribute size, and instance count. Passing any of them in as "null" or "-1" results in no changes to the sizes:

BatchedMesh.resizeGeometry( vertexCount = - 1, indexCount = - 1 );
BatchedMesh.resizeInstances( instanceCount );

Additionally we'd want functions or members indicating how "full" the capacity is so user code can determine whether and how much the BatchedMesh instances need to be adjusted.

cc @Mugen87

Alternatives

Similar to #29463, there aren't any great alternatives to this. Though a user could instead choose to create a new BatchedMesh instance and use both in the scene if this kind of expansion is needed, though it's not ideal.

Additional context

No response

donmccurdy commented 1 month ago

Add support for a "resizeGeometry" and "resizeInstances" functions that takes a new index size, attribute size, and instance count.

If the functions are substantially cheaper than creating a new, larger, BatchedMesh, and deleting the old one, then I'm in favor. I think that's true of resizeInstances but I'm not sure about resizeGeometry... I don't want convenience to obscure the cost of the operation, compared to...

const dst = new BatchedMesh( src.maxInstanceCount * 2, src.maxVertexCount * 2 ).copy( src );

... which is not much code, and makes it obvious this should be done in chunks, not in small increments.

gkjohnson commented 1 month ago

I don't want convenience to obscure the cost of the operation, compared to...

const dst = new BatchedMesh( src.maxInstanceCount * 2, src.maxVertexCount * 2 ).copy( src );

This would require that the "copy" function be changed to continue to respect the original constructor arguments rather than result in a BatchedMesh that shares the same number of instances and geometries as the copy source. But there are no other copy function that behave this way - BufferGeometry recreates all buffer attributes so they are sized the same, InstancedMesh clones textures and updates the max instance count, etc. I think changing the way copy works for this one case would be significantly more confusing than just documenting that the resize functions have to recreate and reallocate texture and geometry data.

donmccurdy commented 1 month ago

But there are no other copy function that behave this way...

I see. I suppose it's more comparable to BufferGeometry#merge, which overwrites the target geometry with the source geometry, up to the original size of the target geometry, but that's perhaps not intuitive either.

Alternatives aside, do you think these functions will be cheaper than creating a new BatchedMesh at the new size? I feel that should inform the API design, and I think it's part of why BufferGeometry is not resizable.

Or put another way, why is BatchedMesh different than BufferGeometry? It could be easier to resize a BufferGeometry, but in that case (I think) the slight friction usually steers people in better directions.

gkjohnson commented 1 month ago

do you think these functions will be cheaper than creating a new BatchedMesh at the new size?

It will be cheaper in both performance and user code complexity. If a brand new BatchedMesh is created the user has to do the following:

Also both geometry and instances structures will have to be recreated any time you want to just expand one or other other.

Rather than the following for just resizing instances if it were to happen internally:

And for BufferGeometry resize:

Both of these functions can also then check for validity when resizing - ie don't allow for resizing if the underly valid data is won't fit in the new size.

Or put another way, why is BatchedMesh different than BufferGeometry? It could be easier to resize a BufferGeometry, but in that case (I think) the slight friction usually steers people in better directions.

The design and nature of BatchedMesh abstracts maintenance of BufferGeometry completely away from the user by necessity. It is not the users job to add or modify underlying attribute buffers directly. As far as an average user is concerned BatchedMesh may not even have a geometry object because there's never a reason to interact with it (nor should they).

BufferGeometry, on the other hand, is designed to afford the user direct access to the underlying data. BufferGeometry can be easily resized in place by disposing of it and copying all data over to new buffer attributes. I've never felt the need for a "resize" function from a BufferGeometry since it's already very straight forward.

Makio64 commented 1 month ago

I'm now using batchedmesh a lot across projects ( games ) and it would be handly but I suggest instead of resizeX :

setGeometrySize(vertexCount = -1, indexCount = -1) setInstanceCount(instanceCount)

It feels more align with the rest of the class and Threejs

donmccurdy commented 1 month ago

Solution looks great in https://github.com/mrdoob/three.js/pull/29577, thank you @gkjohnson!