Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InstancingMesh: Added per instance color support #19947

Merged
merged 1 commit into from Jul 31, 2020
Merged

InstancingMesh: Added per instance color support #19947

merged 1 commit into from Jul 31, 2020

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jul 28, 2020

This allows doing this:

const color = new THREE.Color( 0xff0000 );
const mesh = new THREE.InstancedMesh( geometry, material, 10 );
mesh.setColorAt( 0, color );

@mrdoob mrdoob added this to the r120 milestone Jul 28, 2020
@donmccurdy
Copy link
Collaborator

@mrdoob note that this already works, without GLSL or renderer changes, as shown in this example:

blossomGeometry.setAttribute( 'color', new THREE.InstancedBufferAttribute( color, 3 ) );
blossomMaterial.vertexColors = true;
stemMesh = new THREE.InstancedMesh( stemGeometry, stemMaterial, count );
blossomMesh = new THREE.InstancedMesh( blossomGeometry, blossomMaterial, count );

Maybe .setColorAt(...) could rely on the same method?

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 28, 2020

I looked at that example but I'm not able to follow the code.

And then we have this other one:
https://threejs.org/examples/?q=inst#webgl_instancing_modified

I think we need a simpler example.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 28, 2020

Does the approach in webgl_instancing_scatter support instanceColor + vertexColors?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 28, 2020

Does the approach in webgl_instancing_scatter support instanceColor + vertexColors?

No, it is relying on the fact that existing shader code for the color attribute can be given an instance attribute instead of a vertex attribute. So you can only use one or the other.

I think we need a simpler example.

I think most of the complexity has to do with turning a Mesh into InstancedMesh+InstancedBufferGeometry. calling .setAttribute( 'color', new THREE.InstancedBufferAttribute( color, 3 ) ); is not complicated. In my opinion https://threejs.org/examples/?q=inst#webgl_instancing_modified should be changed to do something that can only be done by editing the shader, like different UV offsets for each instance to give the effect of different textures on each.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 29, 2020

No, it is relying on the fact that existing shader code for the color attribute can be given an instance attribute instead of a vertex attribute. So you can only use one or the other.

What does GLTF think about this?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jul 29, 2020

To support EXT_mesh_gpu_instancing we need the ability to put custom InstancedBufferAttributes on InstancedMesh, as both of these examples do. But glTF doesn't specify any particular meaning of those attributes, except the instance transform. So, support for per-instance colors is not required for glTF: having color+instanceColor, or just color, would both be fine.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 31, 2020

As far as I can see, this PR doesn't conflict with that usage, correct?

The intent of this PR is to make a nicer API for per-instance colors.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 31, 2020

Just for my understanding: This PR allows to use vertex colors and a color value per instance right?

In any event, I like the idea of defining the instance color via setColorAt() without working with buffer attributes.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 31, 2020

This PR allows to use vertex colors and a color value per instance right?

This PR allows setting a color per-instance that multiplies the geometry vertex colors (if enabled) which multiplies the material color, etc

@donmccurdy
Copy link
Collaborator

As far as I can see, this PR doesn't conflict with that usage, correct?

I don't see any confilct, no. :) Worst case, we could always change the internal implementation of setColorAt later.

@mrdoob mrdoob merged commit 1213e0f into dev Jul 31, 2020
@mrdoob
Copy link
Owner Author

mrdoob commented Jul 31, 2020

Thanks doob!

@donmccurdy donmccurdy deleted the instancedmesh branch July 31, 2020 16:10
@donmccurdy donmccurdy restored the instancedmesh branch July 31, 2020 16:10
@mrdoob mrdoob deleted the instancedmesh branch July 31, 2020 16:11
@LasseD
Copy link

LasseD commented Sep 30, 2020

I am happy to see color being added to instancing.

Is there a reason for not including alpha, or at least the possibility of including the alpha channel?

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 30, 2020

@LasseD Instancing and transparency are tricky, see #20431.

@LasseD
Copy link

LasseD commented Sep 30, 2020

Thanks @Mugen87 . This is what I feared. Thanks for the link, it provides good insight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants