Three.js: Change InstancedMesh api signature to count, geom, mat

[drcmda]

Description of the problem

The current signature is unfortunate, since geom and mat have defaults (which means they should be last), but count has to be supplied (it should therefore be first). This causes us some headaches when we need to create an instancedmesh declaratively, like so:

<instancedMesh args={[null, null, count]}>
  <sphereBufferGeometry attach="geometry />
  <meshBasicMaterial attach="material" />

declarative use or not, this is also weird in the imperative way:

const m = new THREE.InstancedMesh(null, null, count)
m.geometry = ...
m.material = ...

the proposal would be:

new THREE.InstancedMesh(count)
new THREE.InstancedMesh(count, geom)
new THREE.InstancedMesh(count, geom, mat)

which i think is natural, in js and ts alike

(count: number, geom?: THREE.Geometry, mat?: THREE.Material)

examples:

https://codesandbox.io/s/three-fiber-useloader-pjcc1

https://codesandbox.io/s/react-three-fiber-gestures-xj31x?from-embed

The prototype is still very new and hasn't even been documented. Can we fix this before it's too late? @mrdoob

[mrdoob]

Another solution would be to set 1 as default for count I guess? 😁

[drcmda]

would be a great solution, if count can later be set by a getter this would even be optimal. otherwise i think when we need to set a count we run into the same problem (b/c it looks like constructor count initiates instanciation).

const m = new THREE.InstancedMesh()
m.count = 10
m.geometry = ...
m. material = ..

<instancedMesh count={10}>
  <sphereBufferGeometry attach="geometry />
  <meshBasicMaterial attach="material" />

[mrdoob]

Hmm, that's react-three-fiber style influencing three.js api... 😁

I'm not sure what's the right solution here. We can't easily resize count without creating a new TypeArray when increasing it.

[drcmda]

i think defaults coming last and required vars first is a standard? in typescript for instance:

but this is exactly what instancedmesh does, two optionals, followed by a required one. these little dings and dongs are just more visible when you put them into a declarative format.

[mrdoob]

I was joking. Lets focus on the resizing count issue.

[donmccurdy]

Alternatively, make Geometry and Material required? 😇

[mrdoob]

Haha!

Here's my understanding of the issue:

@drcmda is mixing two issues in one (the typescript one is related but unrelated).

The core issue is that he wants to use the API like this:

const mesh = new THREE.InstancedMesh();
mesh.geometry = geometry1;
mesh.material = material1;
mesh.count = 50;
// user changes parameters in react-three-fiber layer
mesh.geometry = geometry2;
mesh.material = material2;
mesh.count = 500;

Handling these property changes requires logic that should sit somewhere, either in react-three-fiber or in three.

I don't know...

[drcmda]

@donmccurdy that would be worse for us, b/c then we can't declare instance meshes any longer, they'll have to be imperative. i know our case isn't pressing for threejs, but even just fixing it for api clarity is a plus - the other objects all work required first, optional last, and every other mesh can receive geom/mat later, which is a very useful feature. 😊

since it's not documented yet, we could just flip it and create a small console.warning + fallback mod if we detect that arg 1 or 2 isn't a number.

[donmccurdy]

I see. 👍

I'd disagree that required, immutable constructor arguments make a declarative API impossible — this is supported in A-Frame VR by re-creating an underlying object when needed, for example — but I understand this might have unwanted side effects on your API.

Making count the first argument, or making it optional, seems OK to me. But I do think an operation like mesh.count++ should not hide its (negative) performance implications.

[mrdoob]

I agree.

@drcmda

Again, the issue here is count and not geometry or material.

I'm happy to default count to 1 in the constructor if it helps but, unfortunately, I think you will have to add extra code in your layer. You'll have to create a new InstancedMesh every time the user changes the count property (at least when they increase it). Don't forget to dispose the previous InstancedMesh (not that I have coded that yet 😇).

[drcmda]

declarative use is not impossible, it just causes people headaches b/c they need to inspect source-code before using it: https://spectrum.chat/react-three-fiber/general/how-to-approach-react-three-fiber~b875f48d-a022-4fed-8846-66d1c0c3b036 imo:

• it's not optimal b/c the other three prototypes do not behave like this
• from an api standpoint: it's a prototype with two optionals first, followed by a required arg, which is unusual (and breaks typescript)
• this will never go away after we set it in stone by documenting it in 110
• it could create complications down the line, and most certainly will for us

You'll have to create a new InstancedMesh every time the user changes the count property (at least when they increase it). Don't forget to dispose the previous InstancedMesh (not that I have coded that yet 😇).

reconcilers have no layers. <instancedMesh args={...} /> is the same as new THREE.InstancedMesh(...). JSX is just a different way of expressing it. changing args in the reconciler automatically re-constructs the object (and calls .dispose() on unmount, if it exists).

count = 1 wouldn't change much, it's the args={[null, null, count]) that's a little unfortunate.

[RemusMar]

But I do think an operation like mesh.count++ should not hide its (negative) performance implications.

+1

[mrdoob]

count = 1 wouldn't change much, it's the args={[null, null, count]) that's a little unfortunate.

I guess I'm going to have to learn React...

Is not possible to do something like this?

<instancedMesh args={[
    <sphereBufferGeometry>,
    <meshBasicMaterial>,
    count
]}>

[WestLangley]

Would

mesh = new InstancedMesh( geometry, material, options );

be acceptable as an API? options is an object.

Or

mesh = new InstancedMesh( geometry, material ).setCount( n ); // oops... Is this even valid?

I much prefer the first patern, but I believe @mrdoob prefers the second...

If not, then my preference is to retain the current API for three.js-consistency.

mesh = new InstancedMesh( geometry, material, count );

[donmccurdy]

@drcmda for context, many threejs objects (Mesh, Line, LineLoop, LineSegments, Points, SkinnedMesh) share geometry, material as the first two arguments. While they aren't technically required arguments, there are exceedingly few cases where the user would not provide them in the constructor. I think we're hoping for a solution that maintains consistency with those method signatures.

[drcmda]

understood, it's no big deal. i think the count prop was kinda special, b/c these other objects can mostly be constructed without passing any args, but instanced mesh can't. i guess im mixing them up with the geometry/material objects, they're from what i remember required first, optional later.

Is not possible to do something like this?

yes, but you could just as well pass the classes. but it's not that bad: null, null, count is a small price to pay. :-)

@WestLangley i would generally prefer constructor args over imperative apis, they're shutting out the declarative stuff for real, it can't be properly mapped.

[mrdoob]

Is not possible to do something like this?

yes, but you could just as well pass the classes. but it's not that bad: null, null, count is a small price to pay. :-)

Well, then that's react-three-fiber "opinionated" design. Probably influenced by the React/JSX ecosystem? But the Three.js API is meant to use by passing stuff in the constructors. The fact that you can change geometry and material after the fact is just a nice-to-have feature.

Regarding null, null, count... Mesh currently allows undefined, Geometry or BufferGeometry.

https://github.com/mrdoob/three.js/blob/dev/src/objects/Mesh.js#L49-L50

I don't know what happens when WebGLRenderer tries to render a null. In react-three-fiber API that could happen if the user forgot the geometry node:

<instancedMesh count={[null, null, 10]}>
  <meshBasicMaterial attach="material" />

[Mugen87]

Closing. It seems the consensus is to not change the API in the suggested way.