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

Examples: added instancing_modified example #18256

Merged
merged 1 commit into from
Dec 30, 2019

Conversation

WestLangley
Copy link
Collaborator

Credit for the inspiration for this PR goes to @takahirox.

https://twitter.com/superhoge/status/1176740112345911297

This example is simpler, and just shows how to use code injection to add a custom instanced attribute -- in this case color -- to InstancedMesh.

Suggestions welcome.

examples/webgl_instancing_modified.html Show resolved Hide resolved
var colorChunk = [
'vec4 diffuseColor = vec4( diffuse * vInstanceColor, opacity );'
].join( '\n' );

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much trouble would it be to make this modification offset the UVs rather than changing the color — aside from the effort of finding a model and texture that look good with some offsets? 🤔

There's an easier way to set per-instance color, which I included in instancing / scatter:

// Assign random colors to the blossoms.
var _color = new THREE.Color();
var color = new Float32Array( count * 3 );
var blossomPalette = [ 0xF20587, 0xF2D479, 0xF2C879, 0xF2B077, 0xF24405 ];
for ( var i = 0; i < count; i ++ ) {
_color.setHex( blossomPalette[ Math.floor( Math.random() * blossomPalette.length ) ] );
_color.toArray( color, i * 3 );
}
blossomGeometry.setAttribute( 'color', new THREE.InstancedBufferAttribute( color, 3 ) );
blossomMaterial.vertexColors = THREE.VertexColors;
stemMesh = new THREE.InstancedMesh( stemGeometry, stemMaterial, count );
blossomMesh = new THREE.InstancedMesh( blossomGeometry, blossomMaterial, count );

^But if for some reason that's not a recommended way to do this, I'm happy to address it there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm imagining a translation-only UV offset... so e.g. four sub-textures could be packed adjacent in one image...

. 1 – 2 – 3 – 4
◾️◾️◾️◾️

... and the offset would select a different sub-texture for different instances. That would seem like a good example for something like minecraft-style voxels, where there are a finite number of voxel types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blossomGeometry.setAttribute( 'color', new THREE.InstancedBufferAttribute( color, 3 ) );

blossomMesh = new THREE.InstancedMesh( blossomGeometry, blossomMaterial, count );

Yikes. I can't imagine adding an InstancedBufferAttribute to BufferGeometry ever working. I would not recommend we encourage that coding pattern -- even if it does work in this case.

^But if for some reason that's not a recommended way to do this, I'm happy to address it there.

Yes, I'd recommend you think about changing that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's InstancedMesh + InstancedBufferGeometry + InstancedBufferAttribute there, which doesn't seem obviously wrong, but ¯\(ツ)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that InstancedBufferAttribute should only be added to InstancedBufferGeometry. Adding it to regular BufferGeometry is inappropriate -- IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no BufferGeometry in any of these examples — I'm passing InstantedBufferGeometry to the InstancedMesh constructor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm passing InstantedBufferGeometry to the InstancedMesh constructor.

blossomGeometry.setAttribute( 'color', new THREE.InstancedBufferAttribute( color, 3 ) );

Is not blossomGeometry is an instance of BufferGeometry?

I'm passing InstancedBufferGeometry to the InstancedMesh constructor.

The InstancedMesh constructor expects a BufferGeometry, no?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh you're right. In my head that was definitely InstancedBufferGeometry. 🤔

I would expect the InstancedMesh constructor to accept InstancedBufferGeometry, to allow users to keep its API even when using other instanced attributes, but apparently I haven't actually tested that yet!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^But I was not expecting BufferGeometry+InstancedBufferAttribute to work. I'll update that example...

@mrdoob mrdoob added this to the r113 milestone Dec 30, 2019
@mrdoob mrdoob merged commit 99fd5fe into mrdoob:dev Dec 30, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 30, 2019

Thanks!

@WestLangley
Copy link
Collaborator Author

@mrdoob @donmccurdy Should I change the name to webgl_instancing_custom_attribute?

@donmccurdy
Copy link
Collaborator

Should I change the name to webgl_instancing_custom_attribute?

I don't have a preference between the current name and this suggestion, but I do like that neither is specifically focusing on color being the customized attribute.

Also... had you realized that this example is also adding InstancedBufferAttribute to BufferGeometry? I didn't earlier, but:

Screen Shot 2019-12-30 at 3 16 53 PM

@WestLangley
Copy link
Collaborator Author

@donmccurdy Oh, gosh. I'm doing the same thing that you are doing. :/

OK, I think I will have to give it up and live with it.

Sorry for the unnecessary noise... And thank you for your feedback. :-)

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 7, 2020

I do agree that adding InstancedBufferAttribute to BufferGeometry feels bad when InstancedGeometry exists, and it's obviously an easy mistake to make... that suggests either:

(a) if there are real reasons this is fragile or might fail in the future, we should perhaps log a warning now.

(b) if InstancedBufferGeometry has no remaining distinction from BufferGeometry, except the .maxInstancedCount property which is now redundant with InstancedMesh.prototype.count, then maybe we should remove InstancedBufferGeometry and just accept that InstancedBufferAttributes can be added to BufferGeometry, valid only if the geometry is used with InstancedMesh (maybe?).

@WestLangley
Copy link
Collaborator Author

I agree with @donmccurdy that since InstancedMesh now supports instance colors, webgl_instancing_modified.html needs to be changed to modify some other per-instance attribute.

Or just remove the example. As it is, it is just confusing.

@mrdoob
Copy link
Owner

mrdoob commented May 12, 2021

How about a visibility attribute?

@WestLangley
Copy link
Collaborator Author

I think for now, I'd recommend removing the example until the time at which someone suggests a use case for which customized instancing is required.

@mrdoob
Copy link
Owner

mrdoob commented May 12, 2021

until the time at which someone suggests a use case for which customized instancing is required.

I just did 😅

@WestLangley
Copy link
Collaborator Author

since InstancedMesh now supports instance colors, webgl_instancing_modified.html needs to be changed to modify some other per-instance attribute.

How about a visibility attribute?

Is there a practical use case for which customized instancing is required? If so, we could show an example of that.

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.

4 participants