Docs: Update BatchedMesh with addInstance docs

[gkjohnson]

Related issue: #28404, #28462

Description

Updates the BatchedMesh docs with the new functionality from #28404. Separate PR so it's easier to take a look at. It would be good to get some eyes on it that are less familiar with the concepts here, as well. I've also removed the instancedMultiDraw "getInstancedCountAt" and "setInstanceCountAt" functions to avoid naming confusion.

cc @donmccurdy @RenaudRohlinger @CodyJasonBennett

[donmccurdy]

I think 'items' will need to be defined, maybe something like:

Suggested change

	[page:Integer maxItemCount] - the max number of individual items planned to be added and rendered.<br />
	[page:Integer maxItemCount] - the max number of individual items (total instances of any geometry) planned to be added and rendered.<br />

I lean slightly toward the name, maxInstanceCount, with clarification that the max instance count is shared, not per-geometry.

[donmccurdy]

Especially given the requirement of instantiating each geometry, I think the term "instances" over "items" makes most sense.

[gkjohnson]

Oops - thanks I thought I got rid of "items" everywhere!

[donmccurdy]

I realize it's implied by the word "unique", but just to spell it out:

Suggested change

	[page:Integer maxVertexCount] - the max number of vertices to be used by all unique geometries.<br />
	[page:Integer maxIndexCount] - the max number of indices to be used by all unique geometries.<br />
	[page:Integer maxVertexCount] - the max number of vertices to be used by all unique geometries. Additional instances of a geometry do not contribute toward the vertex limit.<br />
	[page:Integer maxIndexCount] - the max number of indices to be used by all unique geometries. Additional instances of a geometry do not contribute toward the index limit.<br />

[donmccurdy]

Wouldn't "geometry" be the right term in this case? Questions:

1. We're using the terms "index" and "id" interchangeably in this section, which do we prefer?
2. When calling setGeometryAt for an ID previously allocated with addGeometry, are instances of that geometry affected?
3. When calling setGeometryAt for an ID previously allocated with addInstance, is any other instance affected?
4. Is an "instance of an instance" different in any way from an instance created from the same base geometry?

We could consider not supporting creation of "instances of instances" quite yet, if the preferred answers to these questions are non-obvious.

[donmccurdy]

This line answers some of my questions above, but now I'm wondering. Is this by design, by technical requirement, or undecided?

Suppose a BatchedMesh represents a forest with instances of various tree species, each having a high-res and low-res geometry available. Trees within 100m use a high-poly geometry, and those further away use low-res geometries. When an instance crosses the 100m radius I might want to swap out the geometry of that instance alone, which may be difficult within this API. I expect it would be less common to want to the shared geometry of all instances at once, but I'm not sure.

[gkjohnson]

Yeah this was one of the reasons for the other notes in #28404:

Another API option might be to enabling managing of geometry vs instances separately ...

There are technically two types of ids at being used here. A "geometry id", which identifies a range in the BatchedMesh geometry to draw, and an "instance id", which internally refers to a geometry to draw via geometry id. Instance ids are, of course, used to set matrices colors, etc of the given id, as well. The current API obscures geometry ids by referencing the geometries via instance id in the interest of making the API easier to understand but it may just make it more confusing in some cases.

In the LoD case you're referring to users could just hide and show associated with different LoDs but I agree that's not ideal if only because of memory usage (unnecessary matrix data storage). And adding / deleting instances will have a performance cost to "fix up" all ids & matrix textures once the "optimize" function is eventually added. We could add a "swap geometry" function to change the underlying geometry pointer but this will lead to other issues like losing the underlying geometry id once all the associated instances lose it.

I'm starting to feel that we should expose the underlying geometry ids (via addGeometry) in addition to instances (via addInstance) so the user can be aware of and control both, which I think may be more explainable, too. And then other class abstractions can simplify things further if needed. The downsides are that this will be a bit more verbose and be a breaking change. Using the class would now look like this (copied from the other PR):

const mesh = new BatchedMesh( ... );

// initialize geometries
const geometryId0 = mesh.addGeometry( sphereGeometry );
const geometryId1 = mesh.addGeometry( boxGeometry );

// initialize instances
const instancedId0 = mesh.addInstance( geometryId0 );
mesh.setMatrixAt( instanceId0, ... );

const instancedId1 = mesh.addInstance( geometryId1 );
mesh.setMatrixAt( instanceId1, ... );

const instancedId2 = mesh.addInstance( geometryId0 );
mesh.setMatrixAt( instanceId2, ... );

const instancedId3 = mesh.addInstance( geometryId1 );
mesh.setMatrixAt( instanceId3, ... );

And then we can swap the underlying geometry like so:

mesh.setGeometryForInstance( instanceId0, geometryId1 );

Thanks for thinking about some other use cases!

[donmccurdy]

In the LoD case you're referring to users could just hide and show associated with different LoDs but I agree that's not ideal if only because of memory usage ...

Yes, it's currently a similar API InstancedMesh. To support a similar use case there we need multiple InstancedMesh objects, and move the instance matrix between them to 'change' that instance. I think it can be confusing that users must move the instance matrix around to change the geometry of the instance, and this feels similar.

I like the API direction above. Maybe in the interest of getting a first version out sooner, what do you think of saying...

1. addGeometry returns a geometry ID
2. addInstance takes a geometry ID and returns an instance ID
3. instance IDs and geometry IDs are not interchangeable (at least for now)

.... and perhaps we do a separate PR for setGeometryForInstance to keep things simpler. I think the API setGeometryAt( id, geometry ) could also work, we'd just need to keep a WeakMap<BufferGeometry, number> for the lookups, maybe a little overhead involved.

The downsides are that this will be a bit more verbose and be a breaking change.

What is the breaking change compared to today? That calling addGeometry alone doesn't cause anything to render?

[donmccurdy]

We could also keep the API as you first proposed, say that all IDs are instance IDs, and change the behavior of setGeometryAt to affect only one instance. Either:

// (A) geometry must already be in BatchedMesh, with or without instances
batch.setGeometryAt( id, geometry )

// (B) geometry must already be in BatchedMesh, with >=1 instance 
batch.setGeometryAt( targetId, sourceId );

If the last instance of a geometry is removed, all references are lost, and it's eligible for some hypothetical future cleanup process.

[gkjohnson]

• addGeometry returns a geometry ID
• addInstance takes a geometry ID and returns an instance ID
• instance IDs and geometry IDs are not interchangeable (at least for now)

Yes this is what I would propose for now if we're going to change the API. We can add more functions later.

I think the API setGeometryAt( id, geometry ) could also work

Perhaps as an option? But it shouldn't be required since once the geometry is in the batched mesh you should want to be able to discard / GC the original geometry to save memory. This can also be done by a user abstraction.

What is the breaking change compared to today? That calling addGeometry alone doesn't cause anything to render?

Yeah - just this. The user must now call "addInstance" after "addGeometry".

If the last instance of a geometry is removed, all references are lost, and it's eligible for some hypothetical future cleanup process.

This is basically what's happening now. But I don't think I love the idea of requiring that users keep geometry around once it's been added to the BatchedMesh. As I mention above I think one expected benefit is that you can GC it after.

I'm inclined to change the API to require calling addInstance if that sounds good to you?

[gkjohnson]

I've updated the docs to reflect the separation of instanceId and geometryId.

[donmccurdy]

Agreed on all points! 👍

[donmccurdy]

All remaining comments are suggestions only, thanks!

[donmccurdy]

Just an idea, certainly not necessary in this release. We could consider having addInstance take a Matrix4 as a second argument, so that users creating static geometry do not need to explicitly save the instance ID and call setMatrixAt later.

[donmccurdy]

Especially given the requirement of instantiating each geometry, I think the term "instances" over "items" makes most sense.

[donmccurdy]

No changes here, but perhaps the future API is:

• setGeometryAt( geometryId, geometry ) overwrites a geometry (up to the allocated vertex count), and affects all instances of that geometry
• setInstanceAt( instanceId, geometryId ) changes the geometry instantiated by a specific instance ID.