BatchedMesh: Reorganization discussion

[gkjohnson]

Description

Over the last few releases BatchedMesh has had a number of features added to support optimization, resizing, instancing, etc which has made the class very powerful and flexible. But there have been concerns about complexity and surface API, as well as requests for more features including point & line rendering (#29018), and multi-material support (#27930), etc. I'm happy to rework some of the API of the class but I'll request some more details on what exactly the concerns are and what the more tangible impact / problem is so we can discuss solutions.

cc @mrdoob @Mugen87

Solution(s)

• In the short term for this release we can comment the recently-added optimize, setGeometrySize, setGeometryCount functions to avoid future breaking changes if these are going to be moved.

• Introduce some kind of subclassing or flags for more complex rendering or other render types (like lines, points).

• One solution I would like to discuss is moving geometry management to a BatchedBufferGeometry class so that the logic complexity for the addition and removal of sub-geomeries would be encapsulated there. The BatchedMesh class could continue to pass-through the function calls to the batched mesh. Ultimately the goal being to separate some of the logic out from an otherwise monolithic file. This could also allow for more easily sharing geometry between multiple BatchedMeshes in the case that different shader are needed (#29018).

A class could be structured like so:

class BatchedBufferGeometry {

  constructor( indexCount, vertexCount );

  addGeometry( ... );
  setGeometryAt( ... );
  deleteGeometry( ... );

  getBoundingBoxAt( ... );
  getBoundingSphereAt( ... );
  getGeometryRangeAt( ... );

  setGeometrySize( ... );
  optimize();

}

BatchedMesh would otherwise still be responsible for managing instances.

Alternatives

Leave it as-is - other suggestions.

Additional context

No response

[Mugen87]

I was wondering if you could have a simplified BatchedMesh class which does only support adding geometries. So you prepare it once and use it for rendering. No optimize, no delete, no internal reorganization. It would be considered as a static structure. You can of course update and raycast individual geometries/instances similar to InstancedMesh.

This class should be placed in core. A more advanced module should be part of addons that derives the basic API but offers the advanced features from the current BatchedMesh implementation. In this way, we can keep the core module simple and manageable similar to InstancedMesh but provide more sophisticated alternative for more advanced use cases.

[gkjohnson]

Do you mind elaborating on the reason for that kind of restructuring? What problem is it solving?

Does it make sense to discuss what kind of features or changes might be needed for https://github.com/mrdoob/three.js/issues/29018 or https://github.com/mrdoob/three.js/issues/27930? In the interest that any changes for those two features may conflict with other plans.

[Mugen87]

Do you mind elaborating on the reason for that kind of restructuring? What problem is it solving?

It's a maintenance problem from my point of view since we see tendencies of over-engineering in BatchedMesh. A more straightforward implementation would still satisfy most use cases and could easier be maintained by more people. Hence, I suggest to implement a more basic version of BatchedMesh (like the initial one from the first PRs) in core by adhering the style of InstancedMesh. I would then derive an addon from BatchedMesh and implement more management related logic.

[Mugen87]

I would also consider a batch as a more static thing that you generate just once and then use it for rendering. If its structure changes, you throw it away and generate a new one. TBH, I've never liked the idea of modifying the batch via a delete operation and then optimize it again. It seems to me this is something that might be useful for specific use cases (like tiles rendering) but in my view it would better to ensure an optimized structure once when the batch is created (manually or with tools like #29782).

Could it be that the case that the API of BatchedMesh has been bended in the last months to accommodated the requirements of 3DTilesRendererJS?

[Makio64]

@Mugen87 I understand optimize / resize / delete is more specific use case but can we keep setGeometryAt( instanceID ) as a base function as its just changing one parameters and make it much more powerful.

I also re-read the current code today and I think we can simplify rewrite a lot of the code to make it simplier/shorter and easier to maintain while keeping the current features.

[Makio64]

I did a very quick attempt using AI, reducing 300lines while keeping functionality / readability : https://github.com/Makio64/three.js/blob/simplify-batchedmesh/src/objects/BatchedMesh.js

[gkjohnson]

If its structure changes, you throw it away and generate a new one. TBH, I've never liked the idea of modifying the batch via a delete operation and then optimize it again. ... Could it be that the case that the API of BatchedMesh has been bended in the last months to accommodated the requirements of 3DTilesRendererJS?

I don't think this is at all the case. I've listed reasons why in other issues but for performance and memory-bound scenarios optimizing is better. Keeping the original geometry around in memory just to rebuild is an unnecessary cost and it's generally more expensive, anyway. Games or experiences that have assets dynamically loading in and out will benefit from something like this. For myself this will also be used in things like simulation visualizations, environments, and editors where the geometry to load is not always known ahead of time or becomes no longer needed and memory is limited. Unity supports a similar feature called "Dynamic Batching" which automatically builds and adjusts buffers to render based on changing geometry. The implementation details are likely different but this is not some unknown, bespoke concept.

Generally I don't agree with calling the class over-engineered. I agree that there are a variety of use cases, though, and not all of them require the full set of features. Rather the class has been designed to support a wider breadth of them rather than just focusing on simple ones.

In terms of separating the class - I wish we could talk about some of the solutions I have proposed but it seems the only acceptable one may be to move portions of implementation to examples. If an examples class is added I could see one called "DynamicBatchedMesh" that supports optimizing and deleting geometry and instances from the class. This will really just mean moving the delete*, resize*, and optimize functions to a separate class. The DynamicBatchedMesh class would also be reliant on "private" internals of BatchedMesh since it must adjust the geometry references etc. Is this okay?

I did a very quick attempt using AI, reducing 300lines while keeping functionality / readability :

From looking over the diff, this change just removes comments, whitespace, and rearranges code. It makes readability worse, in my opinion. I don't trust AI to make meaningful changes to something like this, unfortunately.