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

Instanced mesh #10750

Closed
wants to merge 47 commits into from
Closed

Instanced mesh #10750

wants to merge 47 commits into from

Conversation

pailhead
Copy link
Contributor

@pailhead pailhead commented Feb 7, 2017

I was trying to implement a higher level abstraction for InstancedBufferGeometry but needed to tinker with the shader lib and renderer. Would it be possible / good idea to expose a bit of these encapsulated functions so they can be overriden? Same for the defines on the standard materials.

This should spin around 1 million triangles depending on how it generates:

http://dusanbosnjak.com/test/webGL/three-instanced-mesh/webgl_instanced_mesh.html

The performance double sided demo with 30k objects instead of 5k, was getting around 16fps with 30k, instanced its 60.

http://dusanbosnjak.com/test/webGL/three-instanced-mesh/webgl_performance_doublesided.html

@pailhead
Copy link
Contributor Author

pailhead commented Feb 7, 2017

var cluster = new THREE.InstancedMesh( 						
  geometry , 						
  new THREE.MeshPhongMaterial( {
    color: c,
    shininess: 150,
    specular: 0xffffff,
    shading: THREE.SmoothShading
  } ), 							
  function( positioningObject , instanceIndex , instanceNumber ){
     positioningObject.position.set(...);
     positioningObject.rotation.set(...);
     positioningObject.scale.set(...);
  },

  numInstances,
  false, //uniform scale
  false //delete original geometry
);

@titansoftime
Copy link
Contributor

@Benjamin-Dobell is working on one in #10093 which is freaking amazing. I have tested it out and the performance gains are beyond fantastic. Check it out!

@pailhead
Copy link
Contributor Author

pailhead commented Feb 8, 2017

oh lord, and i'm the one doing the trolling lol :) I'm afraid to say anything more in that conversation, i hope it was just a giant misunderstanding. I'm more interested in breaking up the renderer into smaller modules so they could be monkey patched than to get this particular pull request accepted.

This is a totally different approach, no auto anything, it just simplifies the creation of static instanced geometries, and hooks the instancing functionality to the shaders. It may not even do a perfect job at it, but the point is, it works somewhat, but requires a lot of hacking and overriding. (like replace an entire class just to add a #define to a template.

@titansoftime
Copy link
Contributor

Uh.. yea I didn't read that entire thread. No time for that. Hey man the more the merrier in my book. I can't wait to benchmark your build as well!

@pailhead
Copy link
Contributor Author

pailhead commented Feb 8, 2017

Well, i'm currently working on a module. The only way i can think of making it work is by locking it to a specific version of three and then maintaining the monkey patch.

Probably doesnt work yet, but stay tuned in. In the meanwhile, two examples are posted up there, the one with the shadows renders 1 million triangles in a few passes (2 lights). The build should be unminified. You also have my fork and branch.

https://github.com/pailhead/three-instanced-mesh

@Benjamin-Dobell
Copy link
Contributor

Benjamin-Dobell commented Feb 8, 2017

@pailhead You had almost 3 months to propose an alternate implementation, but instead chose to introduce artificial constraints such as "static geometry" so that you could bash my implementation, without any statistical or analytical evidence to substantiate your claims.

Now that you've got an alternate proposal in place we can have a constructive conversation about this.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 8, 2017

No intention to bash anyone, three.js comes first and foremost :) peace brothers, how many people even do stuff like this, especially for fun, we should be friends

@pailhead
Copy link
Contributor Author

pailhead commented Feb 8, 2017

Ungh, so in 78 i could monkey patch it by doing something like this:

module.exports = function( THREE ){

	//patches these methods and shader chunks with the required logic 

	require('./Material.js')(THREE);  //override the constructor


        //all the constancts, libs and stuff are under THREE namespace
	THREE.WebGLProgram = 	require('./WebGLProgram.js')(THREE);  
	THREE.WebGLPrograms =	require('./WebGLPrograms.js')(THREE);

	require('./WebGLShadowMap.js')(THREE);
        
        //just override the chunks
	THREE.ShaderChunk[ 'begin_vertex' ] = require('./begin_vertex.glsl.js'); 
	THREE.ShaderChunk[ 'defaultnormal_vertex' ] = require('./defaultnormal_vertex.glsl.js');

	THREE.ShaderChunk[ 'common' ] += 	require('./common.glsl.js'); //just append the inverse function
	
}
module.exports = function ( THREE ){

	require('./monkey-patch/index.js')( THREE );

	THREE.REVISION = "78_InstancedMesh";

	console.log( THREE );

	return THREE;

}

I'd have to override all three webgl functions, but they would still not be the entire webglrenderer.

In 84, most of the classes and constants are imported and exist only in that scope. For instance, replacing THREE.WebGLPrograms wont get it called in THREE.WebGLRenderer since it's private , and doesnt exist in THREE' scope.

I tried to build the renderer but it built all the imports recursively, so i ended up with a 500kb file containing definitions for matrices and vectors.

I was going to ask about breaking the WebGL* modules even further, and exposing all of the functions so that they could be overridden. But in the newer versions this is closed even further, or at least a bit confusing.

I want to achieve this:

https://www.npmjs.com/package/three-instanced-mesh

require('three-instanced-mesh/monkey-patch.js')(THREE); //make three read an additional Material property and add some #defines 

require('three-instanced-mesh')(THREE);

var im = new THREE.InstancedMesh(...);

So with this, i can theoretically have a class that helps set up THREE.InstancedBufferGeometry, fill an attribute for me, and have some shader snippets to do a transformation. It may be somewhat useful and i can share it with others.

But if i want to add the functionality to this module, so that it can work with shadows, AO, regular materials, i need to patch a lot of stuff under the hood on three :(

Any thoughts @mrdoob?

@pailhead pailhead mentioned this pull request Feb 8, 2017
12 tasks
@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

It's possible to use 3 v4 atts instead of 4 here.

Without shadows working, there's no need do change anything in the renderer.

npm install three-instanced-mesh

if you want to take it for a spin.

I think if WebGLShadowMap would tag it's depth shaders with #define INSTANCED an effect like this could be further extended.

@mrdoob
Copy link
Owner

mrdoob commented Feb 9, 2017

I'm liking this! 👍

Would it be possible to change the API to something more like this? #10093 (comment)

var mesh = new THREE.Mesh( geometry, material );
var instances = new THREE.Instances( mesh, 1000 ); // This produces an internal array of 1000 identity matrices

var vector = new THREE.Vector3();

for ( var i = 0; i < 1000; i ++  ) {

    vector.x = i;
    instances.setPositionOf( i, vector ); // This sets the position in the internal matrices array

}

scene.add( instances );

@mrdoob
Copy link
Owner

mrdoob commented Feb 9, 2017

Actually, something similar to what you have already:

var instances = new THREE.InstancedMesh( geometry, material, 1000 );

var vector = new THREE.Vector3();

for ( var i = 0; i < 1000; i ++  ) {

    vector.x = i;
    instances.setPositionOf( i, vector ); // This sets the position in the internal matrices array
    // instances.setRotationOf( i, euler );
    // instances.setQuaternionOf( i, quaternion );
    // instances.setScaleOf( i, vector );
    // instances.setMatrixOf( i, matrix );

}

scene.add( instances );

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

w00t, i was able to build this and hook it up without needing to hack anything!

https://github.com/pailhead/three-instanced-mesh/blob/master/README.md

I'd be more than happy to commit this though. Need to figure out what exactly happens when you update the attribute buffer.

What you're proposing should be a method right? Still could take the "placement" function?

Should the distributed geometry also be made into a module or kept private within the mesh?

@mrdoob
Copy link
Owner

mrdoob commented Feb 9, 2017

Still could take the "placement" function?

Not a fan of that API... I would prefer to keep things separated (constructor from transformations)

Should the distributed geometry also be made into a module or kept private within the mesh?

Not sure I understand what you're asking...

@@ -70,10 +71,12 @@ function WebGLShadowMap( _renderer, _lights, _objects, capabilities ) {

var useMorphing = ( i & _MorphingFlag ) !== 0;
var useSkinning = ( i & _SkinningFlag ) !== 0;
var useInstancing = ( i * _InstancingFlag ) !==0;
Copy link
Owner

@mrdoob mrdoob Feb 9, 2017

Choose a reason for hiding this comment

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

Shouldn't this be i & _InstancingFlag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should, but, like i said, i dont think there is a need to do any of this any more, a material could be decorated with a define, and there is no need to change anything in order for this to work

Should also be useInstance same length as the other two :D

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, that sounds good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok ill look a bit through your comments, and play around with updates, without an instance of Object3d, there should be some dirt flags in there too? Or should this just be left static? If so then this is ready to go, there are no changes to any of the files except for the three shader snippets. What i'd do maybe is extend the define mechanism a little bit. Right now the piggybacking on uv_pars_vertex seems to work, but it feels like there should be a dedicated global_pars_vertex since common is used in fragment shaders as well...

Could that be a part of this commit or a separate thing?

Copy link
Contributor Author

@pailhead pailhead Feb 9, 2017

Choose a reason for hiding this comment

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

Can you see that anyone would want to use skinning and morphing with this?

This is how i've worked around programs and shadows:

https://github.com/pailhead/three-instanced-mesh/blob/master/index.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, any thoughts on

var cluster = new THREE.InstancedMesh( geometry , material , 1000 , optionalPlacementFunction );

Should also convert regular geometry to buffer allow for that, as is it would work only with buffer.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you see that anyone would want to use skinning and morphing with this?

Lets wouldn't support skinning and morphing for now... 😅

Also, any thoughts on

var cluster = new THREE.InstancedMesh( geometry , material , 1000 , optionalPlacementFunction );

I would prefer keeping it simple:

var cluster = new THREE.InstancedMesh( geometry, material, 1000 );

@lojjic
Copy link
Contributor

lojjic commented Feb 9, 2017

I'm just seeing this branch now. I have a few questions based on my particular use case and how I might use this new API for it:

  1. It looks like the InstancedMesh has its numCopies set at construction time; does this mean that the number cannot be changed after the fact without destroying and replacing that InstancedMesh object and its underyling geometry+buffers?
  2. Assuming that answer is yes, is there any way to frustum-cull instances without changing the numCopies?
  3. How would one handle raycasting to select (and, presumably, modify in some way that may make it non-instanceable) a single instance within the InstancedMesh?
  4. The instanceUniform flag is an interesting optimization; for the non-uniform case I'm curious about the impact of computing the normal matrix per-vertex on the GPU rather than per-mesh on the CPU, have you benchmarked that at all with varying vertex counts?
  5. Is there any fallback for the (admittedly rare) case where the instanced arrays extension is not available?

EDIT: Whoa, I just noticed that not only is the numCopies static, the distributeFunction is only called once at construction time. Unless I'm mistaken this means that the transforms of the instances are totally static?! If so, then this is a total non-starter for me. I don't need a baking solution, I need to be able to improve performance of dynamic objects.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

EDIT: Whoa, I just noticed that not only is the numCopies static, the distributeFunction is only called once at construction time. Unless I'm mistaken this means that the transforms of the instances are totally static?! If so, then this is a total non-starter for me. I don't need a baking solution, I need to be able to improve performance of dynamic objects.

Yes you are right, this is just for "baking" or drawing lots of static stuff. Perhaps you should make your own fork of three.js and try to experiment a bit with the thing you need?

I worked on 78 for i don't even know how long, because thats the one where i maintained the instancing and some other stuff. It took this whole pull request to figure out that none of this is needed, other than perhaps global vert and frag includes. Try running npm install three-instanced-mesh and you should be able to use this with just shadows.

Look here for source

  1. It looks like the InstancedMesh has its numCopies set at construction time; does this mean that the number cannot be changed after the fact without destroying and replacing that InstancedMesh object and its underyling geometry+buffers?

Generally you shouldnt resize buffers.

With regards to updating BufferGeometries, the most important thing to understand is that you you cannot resize buffers (this is very costly, basically the equivalent to creating new a geometry). You can however update the content of buffers.

So i think the approach here is generally in the spirit of three. It's not impossible to set the scale to 0, thus not rasterizing an instance, but a transformation needs to happen. I don't entirely understand what happens here, or for example when you branch the shader and say if( instanceAtt == something ) gl_Position = vec4(0.);, but i think it doesnt actually help.

  1. Assuming that answer is yes, is there any way to frustum-cull instances without changing the numCopies?

Well, dont know if it's yes or no. I think its more no, but these two questions are not related. In the example of that cluster of boxes/cylinders/spheres, you can assign the bounding box manually, knowing that your logic ran Math.random() in three directions, it would be a box from (0,0,0) to (1,1,1). You can also write a function to test the meshe's bounding box with each iteration, and extend the previous one if it needs extending, youll end up with the tightest fitting AABB.

Up to you if you want to instance every lamp post in the world, or every lamp post belonging to a tile in a grid... But yeah, this is a mesh, it can be frustum culled.

  1. How would one handle raycasting to select (and, presumably, modify in some way that may make it non-instanceable) a single instance within the InstancedMesh?

You probably couldnt do this without the shaders. Although i was thinking that maybe material color should also be considered and have an attribute, especially now that this can use 3 atts instead of 4.
In either case, you'd have to write your own logic:

var positions_rotations_scale_colors_whatever = computeStuff();

var culling_raycating_collision = computeBoundingStuff( myMesh );

var drawing_stuff = new InstancedMesh( myMesh.geometry ); 

Just imagine that you have two scenes, a precomputed set of positions, you build the graph out of a 1000 mesh nodes in one, you build a graph out of a 1 mesh node rendering 1000 instances. Youd use the other one for collision, picking, culling etc....

  1. The instanceUniform flag is an interesting optimization; for the non-uniform case I'm curious about the impact of computing the normal matrix per-vertex on the GPU rather than per-mesh on the CPU, have you benchmarked that at all with varying vertex counts?

Have you seen the examples? I've got it running quite buttery with millions of triangles, no culling, and two shadow lights. I've got to look a bit into the math myself, this was just taken from literature. I agree that there is a tradeoff here, and i think things could be slightly simpler if the lighting was done in world space, but i'm not sure. I don't really see a way to avoid the inverse if it's an on uniform scale.

In any case, you should make that choice based upon the knowledge of how webgl works:

Few high-res meshes - precompute stuff.
Many low-res meshes - parallelize stuff

  1. Is there any fallback for the (admittedly rare) case where the instanced arrays extension is not available?

Not sure what would happen here. For my use case it would actually be useful to transform and merge the instances together, but it should probably be left for the user to handle.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

I don't need a baking solution, I need to be able to improve performance of dynamic objects.

The question is should that be a part of this, perhaps by saying .dynamic = true; or if it shuold be a part of a particle system...

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

@mrdoob this is actually a bit more complicated now that i think of it. With the constructor, the mechanism for distribution is super simple.

What happens when you set only one transformation

setPositionOf( 20 , ... );
setRotationOf( 20 , ... );
//move to next one, what should happen here
setPositionOf( 21 , ...)

I'm not keeping any position rotation scales as objects, it just goes straight to the buffer.

It would probably have to be something like

instanced.setPositionOf( index , vector , writeIndexToAttribute );

So:

instanced.setPositionOf( 20 , vec , false );
instanced.setRotationOf( 20 , vec , true );
instanced.setPositionOf( 21 ...);

or (of course not with this names but illustrates the problem):

instanced.setPositionToBeSet( vec );
instanced.setRotationToBeSet( vec );
instanced.updateTRSBufferAt( index );

@lojjic
Copy link
Contributor

lojjic commented Feb 9, 2017

@pailhead Thanks for the clarifications.

So, even though this was aggressively promoted as an alternative/better approach to handle the use cases that were improved by @Benjamin-Dobell's #10093 (including mine), it in fact does not do that at all; it's for an entirely different use case. That's frustrating, but I'll move forward...

I do see that even though your InstancedMesh class specifically does not meet my need, the changes you made to the program/shader code that lets them accept the transformation matrix as instanced attributes are generically useful. So another layer could handle setting up the InstancedBufferGeometries in using whatever API shape is most ergonomic for the use case, and feed to the shaders using those same hooks. I'm 100% in favor of that.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

So, even though this was aggressively promoted as an alternative/better approach to handle the use cases that were improved by @Benjamin-Dobell's #10093 (including mine)

No it was not. As a three.js user, i just voiced my concern about having cruft in the core.

This is available as a module, it doesn't even have to be present in the library, it can be completely optional if you want to use it or not.

@pailhead
Copy link
Contributor Author

pailhead commented Feb 9, 2017

the changes you made to the program/shader code that lets them accept the transformation matrix as instanced attributes are generically useful.

Well, it's up to you how you fill that attribute, the pull request once i update it helps you format that matrix4 with three vectors, guess you can call that generic enough. But sounds to me like you would settle for just a #IS_INSTANCED define that would be set on all encountered InstancedBufferGeometry.

Please consult this to see how this is implemented and what hooks three.js already provides.

https://github.com/pailhead/three-instanced-mesh/blob/master/index.js

Using this pattern you can totally modify this however you feel. Make other methods for setting up attributes, different shaders, whatever.

Consult the other conversation on how to render those crates from the warehouse, or whatever dynamic thing that needs to happen. Up to you to fine that sweet spot where something is performance tuned, but this should give you an additional tool.

I've been super patient, but i'm done giving free graphics lessons, the cohort from the other thread owes me a beer ;)

@Benjamin-Dobell
Copy link
Contributor

how many people even do stuff like this, especially for fun

I've been super patient, but i'm done giving free graphics lessons, the cohort from the other thread owes me a beer ;)

If only you hadn't decided to give out "free graphics lessons" in the first place, based on a blog post you read this one time...


//@author pailhead
//for now the most convenient place to attach vert transformation logic in global scope ( before main() )
#if defined( INSTANCE_TRANSFORM )
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrdoob, this stuff should go in the global_vertex hook

vec3 s = instanceScale;
vec3 v = instancePosition;

vec3 q2 = q.xyz + q.xyz;
Copy link
Contributor Author

Choose a reason for hiding this comment

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


mat4 _instanceMatrix = getInstanceMatrix();

#define INSTANCE_MATRIX
Copy link
Contributor Author

Choose a reason for hiding this comment

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

could not find a more elegant solution, it's scattered around in different shaders, and there is a race condition

* this is in lieu of changing the renderer
* WebGLRenderer injects stuff like this
*/
this.material = material.clone();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is all super questionable, in order to not have logic in the renderer that sets up the shaders properly, and flags on materials, this hack is happening
otherwise something like

myMaterialForInstancing = new THREE.SomeMaterial()
myMaterialForInstancing.instanced = true

@mrdoob mrdoob added this to the r91 milestone Jan 17, 2018
@mrdoob mrdoob modified the milestones: r91, r92 Mar 2, 2018
@mrdoob mrdoob modified the milestones: r92, r93 Apr 18, 2018
@pailhead
Copy link
Contributor Author

@mrdoob, is this even needed to be in the core? With a more flexible shader/material system it may not need to?

@pailhead
Copy link
Contributor Author

pailhead commented May 6, 2018

I would like to close this in favor of

#13198

The PR above is less than 10 lines of code that would allow for these kind of modules to be much better, easier to write, be stackable etc.

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.

8 participants