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 Geometry Suggestion #13995

Closed
titansoftime opened this issue May 4, 2018 · 36 comments
Closed

Instanced Geometry Suggestion #13995

titansoftime opened this issue May 4, 2018 · 36 comments

Comments

@titansoftime
Copy link
Contributor

titansoftime commented May 4, 2018

I successfully created a ground foliage system using instanced buffer geometry. Setting up the instancing wasn’t nearly as bad as I though it would be, however one aspect was just downright tedious.

What I’m referring to is recreating the Lambert shader manually and then adding in the custom orientation / position adjustments. This was necessary for the foliage material to respect scene lighting / shadows etc. Rebuilding the Lambert Material into a ShaderMaterial was not fun.

Would it not be easier to have a few for example: ShaderChunks[‘Instancing’]? If I’m not overlooking anything, it would just require a few chunks, an instancing_pars chunk which could contain the instancing attributes and applyQuaternionToVector function (from three.js dev instancing examples), and one right after the ‘begin_vertex’ chunk to alter the vec3 transformed variable using the function.

Would this not be much more user friendly? If a user wants to take advantage of instancing (who wouldn't), they simply have to set instancing: true on the material and of course set up the needed buffer attributes. They would no longer have to manually reconstruct an entire materials shader.

I've seen more complex examples of trying to ease usage of instancing, perhaps we should start with something a little more simple like this?

I’d be more than happy to make a PR, if this is desirable.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2018

Rebuilding the Lambert Material into a ShaderMaterial was not fun.

Can you please explain your approach? Besides, are you aware of Material.onBeforeCompile() or the reusable character of shader chunks?

@titansoftime
Copy link
Contributor Author

I did a console log of THREE.ShaderLib['lambert'], then replaced all the #include with the corresponding THREE.ShaderChunk in both the vertex and fragment shader.

I'm familiar with the shader chunks but not Material.onBeforeCompile().

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2018

Replacing the includes is actually not necessary, see this example.

Apart from that, I'm in general not sure if we need to add new library functionality if Material.onBeforeCompile() is properly used.

@donmccurdy
Copy link
Collaborator

I haven't tried using builtin materials with instancing, so neither the "not fun" @titansoftime mentions nor the onBeforeCompile trick you mention are familiar to me... Perhaps we could write an example of this, and consider the material.instancing=true change if that isn't straightforward enough?

@titansoftime
Copy link
Contributor Author

titansoftime commented May 6, 2018

Oh, well I feel like an idiot for swapping out those includes lol, oh well.

I'll check out the onBeforeCompile and see if that works as well as patching in my new shader chunks.

I must add though, as an end user, I think it would be far more simple to utilize my suggested method, and it's a pretty small change.

When I first looked into instancing a few years ago, I saw that I would have to mess with shaders and was like "umm, nope".

Adding a instancing flag/define to materials and adding like 2 additional chunks would open up instancing to novice users in my humble opinion.

I've got my solution working fine for me, just want to extend the benefits of the knowledge your assistance has brought me to others (especially newbies) =]

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2018

Perhaps we could write an example of this.

I've written this codepen to show how to enhanced MeshStandardMaterial with Material.onBeforeCompile() for basic instanced rendering. If you guys like it, I make a PR with a new example.

https://codepen.io/anon/pen/bMoOKB?editors=1010

@WestLangley
Copy link
Collaborator

WestLangley commented May 6, 2018

There are many more details that need to be dealt with, in general. I'm filing another example today.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2018

The codepen was just intended to get a first impression of the approach. It's good to see a more detailed example 👍

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

@Mugen87
It sounds like you really want to write an example for onBeforeCompile rather than address whatever the instancing issue is outlined here.

Yes, onBeforeCompile may help when makingInstancedBufferGeometry more integrated, but just making it do + vec3(offset) is quite quite limited, wouldn't you agree?. Although the current example is pretty limited too. If you were to write a summary of what that one liner demo does what would that be?

@titansoftime
I really believe that you are asking for this:
#13198

All the drawbacks that i've been trying to point out with the onBeforeCompile are outlined in this thread. To reproduce:

  1. extend the one liner, one chunker + offset with real world needs for this case - rotation, scaling, working with shadows, normal maps, etc, etc. Is it intuitive, is it verbose, is it prone to bugs?
  2. make it further extensible, so that some other area can also be replaced with a real world example (add a different normal mapping decoder on top of instancing, make them all use a different plane clipping algorithm (world space vs view space))

A twisting head is a really poor example of what people encounter when writing applications. This is a great one. If the shader from a community module is all that's needed to allow for this feature, you should be able to patch it in, and then build on top of it.

I would love to see an example of onBeforeCompile used there.

Oh yeah:

@Mugen87
3. Are you sure that your code wouldn't break a material that's already using onBeforeCompile?

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

Just to reiterate:

@titansoftime (From the OP)

Would it not be easier to have a few for example: ShaderChunks[‘Instancing’]?

From @mrdoob
#10791 (review)

Paraphrasing:

Suggestion:

With feature FOO we can easily add dummy named chunks

//inside some material template
#include <logdepthbuf_pars_fragment>
#include <clipping_planes_pars_fragment>
...
#include #include <MY_DUMMY_CHUNK_PROPOSAL>
#include #include <AFTER_NORMAL>
#include #include <THREES_GLOBAL_UNIFORM_INJECTION_PLACE>
#include #include <EXTERNAL_GLSL_FUNCTIONS>
...

^ this means that a PR could be:

Hi, i'm refactoring the shaders, and i found a point where it would be useful to put hook FOO. I've added the #include <FOO> into all the templates that need it and added a test/example. Please review.

Response:

No, it should be different syntax.

% HOOK_NAME %

The PR from above:

Hi, i'm refactoring the shaders, and i found a point where it would be useful to put hook FOO. However i don't see anything that can parse the proposed % FOO % hook so i'm not sure what to do. It seems like it's possible just to add another #include <FOO>, but i don't want to be "...abusing the includes system".

@titansoftime

Would it not be easier to have a few for example: ShaderChunks[‘Instancing’]? If I’m not overlooking anything, it would just require a few chunks, an instancing_pars chunk which could contain the instancing attributes and applyQuaternionToVector function (from three.js dev instancing examples), and one right after the ‘begin_vertex’ chunk to alter the vec3 transformed variable using the function.

Would this not be much more user friendly?

In the context of instancing:
Yes, three.js would have to settle on what that API looks like. One proposal was InstancedMesh but with access to the core it could be solved in many ways. The easiest is probably what you are describing. Material controls the bulk of it, the rest is in InstancedBufferGeometry. It's the cleanest most minimal 1:1 relationship.

myMaterial_to_be_instanced.instancing = true 

^ is actually probably too much, the renderer knows that it needs the instancing so it could just treat InstancedBufferGeometry with some assumed knowledge.

Maybe to not confuse it one could have MaterialFriendlyInstancedBufferGeometry, no flags are to ever be set, just the contract that you need to fill the attributes aFoo, aBar, aBaz. But this wouldn't even have to be on an attribute level. More like setQuaternionAt. However, this gets awfully close to what InstancedMesh already does.

Actually i think i may have just described InstancedMesh, it aspires to have a minimal interface, which IMO is:

const myRootGeometry = getBufferGeometry()
const myAgnosticMaterial = new WhateverMaterial()

//InstancedMesh
// neither the geometry or the material actually know about instancing
// this information is on the Mesh class, the scene graph, 
// which is the same level you manage your .position .quaternion .scale anyways 
const myInstancedMesh = new InstancedMesh( myRootGeometry, myAgnosticMaterial ) 

//Your proposal
const myInstancedMaterial = new WhateverMaterial()
myInstancedMaterial.instanced = true //<-- this is too much code im really lazy

const my_____Mesh = new Mesh( myRootGeometry, myInstancedMaterial )
//^ how do we call this, if its myInstancedMesh you now have two things that warrant having "instanced" in their names
// if it's just myMesh it becomes a bit ambigous, it may or may not be something that you care about
// if its something static then its just "east cliff section" if it's something dynamic, it's "my cluster of stuff"
// in one case you will care about the instance related methods in the other you will set and forget

//the rest of what you described?
// somewhere you would have to call this, 
// my opinion is that it should be on the same level you otherwise set THESE types of positions and quaternions
// which is Object3D
[SOME_OBJECT_MATERIAL_MESH_GEOMETRY_CONTROLLER].setQuaternionAt(instanceIndex, quaternion)
[SOME_OBJECT_MATERIAL_MESH_GEOMETRY_CONTROLLER].setPositionAt(instanceIndex, quaternion)

attributes and applyQuaternionToVector function (from three.js dev instancing examples

^ this i'd need a link to, i'm not sure what applyQuaternionToVector does, but i think it would be helpful if you listed the methods or how the api would look. What's the minimum set of things you want to interface here?

In the context of extending materials, either at run time, or in PRs:

Try doing it with onBeforeCompile.

You will find that it's literally the antitheses of what you are asking for. I've literally tried to make a point,

Hey everyone still reaches out to THREE.ShaderChunk when they want to extend shaders it indicates a flaw with onBeforeCompile.

The answer IMHO is not to modify a dictionary of strings in the global THREE namespace, but instead consider that whatever reaches into that dictionary, whenever it does it, could have an alternative dictionary there provided by the user (or package author).

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

@Mugen87

Replacing the includes is actually not necessary, see this example.

Apart from that, I'm in general not sure if we need to add new library functionality if Material.onBeforeCompile() is properly used.

What is the proposal here, or what do you have in mind when you say "properly used"?

I see the example as the inverse of the problem. It's how to include some shader chunk that's exclusively used by the templates, in a custom ShaderMaterial.
The problem here is how to use something that would be exclusive to a ShaderMaterial or RawShaderMaterial (custom GLSL) and use it in a template.

Please correct me if i'm wrong. I also didn't quite understand the following, so i'm not sure if u nderstood the context of the example.

I did a console log of THREE.ShaderLib['lambert'], then replaced all the #include with the corresponding THREE.ShaderChunk in both the vertex and fragment shader.

If this contract is up on that template level, meaning:

Let's all agree that every Material is an instance of ShaderMaterial

then overriding a built-in material such as MeshLambertMaterial would require providing a new template. Meaning, you would copy the code for Lambert, put your additional #includes, stick it all into a ShaderMaterial and call it done. You wouldn't need onBeforeCompile but it's not the least verbose solution either.

I've seen this approach i think, but i thought that onBeforeCompile is supposed to bring convenience, or flexibility here? Am i wrong in thinking that? If not, what is the convenience?

@titansoftime

and one right after the ‘begin_vertex’ chunk to alter the vec3 transformed variable using the function.

You're describing stages here, current syntax:

myMaterial.onBeforeCompile = shader => { 
  shader.vertexShader = shader.vertexShader.replace( //<-- uhh, giant string... i'd prefer a different data structure, i'd like to not use `replace()`
    '#include <begin_vertex>', //<-- soo much text, i'm a human i don't care about the #include part, the machine does
    `
     ${ myOnBefore_begin_vertex}
     ${ THREE.ShaderChunk.begin_vertex }
     ${ myOnAfter_begin_vertex}`
     `
  )
}

^ a gotcha is that this function must not include any branching logic, meaning there is no if else in there, otherwise it behaves rather erratically. Not a bug though:
#13192


And yeah sorry for the wall of text, but I don't think it's out of place because it sparked a conversation including all these features. I'm not sure what the topic here is, and if it would help if @titansoftime clarified it. As someone who worked on both the described instancing interface, and someone who's been the most vocal about onBeforeCompile this topic touches both.

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

// super generic utility extension, very broad
class Material_onBeforeAfter_begin_vertex extends THREE.SomeMaterial{
	constructor(params){
		super(params)
		
                //no need to schedule this to happen with replace() at some async point in the future 
                // when WebGLRenderer gets a hold of this object
		this.chunks.begin_vertex = `
			${params.beforeChunk}
			${THREE.ShaderChunk.begin_vertex}
			${params.afterChunk}
		`

	}

}

//a very specific material, with hard coded chunks
//extends because maybe before begin_vertex is very common and shared
class InstanceMaterial extends Material_onBeforeAfter_begin_vertex{

	static chunk_before_begin_vertex = '...'
	static chunk_after_begin_vertex = '...'

	constructor(params){
		params.beforeChunk = InstanceMaterial.chunk_after_begin_vertex
		params.afterChunk = InstanceMaterial.chunk_after_begin_vertex

		super(params)
                //already abstracted, no need for anything other than providing two strings in shape of valid GLSL code
	}

}

//a generic wrapper that should be able to wrap any instance of SomeMaterial 
//it could be also a function taking any instance of any material
class NormalMappingMaterialWrapper extends THREE.SomeMaterial {
	
	static chunk_replace_normal_begin = '...'

	static chunk_append_to_normal_begin = '...'

	constructor(someMaterial){
		super()
		this.copy(someMaterial)
                 
                //logic, which is impossible with onBeforeCompile()
		if( this.chunks.normal_begin ) {
			this.chunks.normal_begin = NormalMappingMaterialWrapper.chunk_replace_normal_begin
		} else {
                        //conflict can be solved on case by case basis, within what ever system is working as a whole, meaning, up to you!
			console.warn('Source material already has a chunk override for this chunk, would you like to foo, bar or baz')
		}

		if( this.chunks.normal_begin ) {
			this.chunks.normal_begin += NormalMappingMaterialWrapper.chunk_append_to_normal_begin
		} else {
			this.chunks.normal_begin = THREE.ShaderChunk.normal_begin + NormalMappingMaterialWrapper.chunk_append_to_normal_begin
		}

	}
}

//now this should hopefully work
const myInstance_NormalMapped_Material = new NormalMappingMaterialWrapper( new InstanceMaterial() ) 

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

Even with onBeforeCompile in place, i've seen experienced users reach out for THREE.ShaderChunk
#12977 (comment)

Unfortunately, I think that this is currently the superior way to onBeforeCompile.

const myChunkManager = new ChunkManager()  //<-- knows of all the chunks, and all the defines
//^ this is where you solve how all your extended shaders work as a whole, in regards to three.js and themselves

const { foo, bar, baz, qux } = myChunkManager.getChunkKeys() //some known names of various shader effects

myChunkManager.overrideChunks(THREE.ShaderChunks)

//can conditionally apply effect to any material regardless of it's other effects, if there is a solution on how they work together
function someMaterialWrapper( material ){
  const mat = material.clone()
  if( mat.defines[foo] ){
    mat.defines[bar] = ''
  } else {
    mat.defines[foo] = ''
  }
  if(mat.defines[baz]){
    delete mat.defines[baz]
  } 
  mat.defines.qux = ''

  if(mat.defines.SPECIAL){
     console.warn(`Apologies, but this wrapper cannot be applied to type ${mat.type}`)
     return material
  }
  return mat
}

Material.defines === {} is a decent structure to work with, so something like this could work, provided any material, check whether it's some extended material by checking the defines, and then apply some logic that makes your effect work with the input effect.

None of this is possible when working with onBeforeCompile as there is no way of knowing that is defined in that function, unless:

const  shaderInjectInfo = extractDataFromFunctionBody( material.onBeforeCompile.toString() ) //<-- gnarly

@pailhead
Copy link
Contributor

pailhead commented May 6, 2018

@titansoftime

tl:dr;
Npm module/github:
https://github.com/pailhead/three-instanced-mesh/blob/master/monkey-patch/index.js
PR here (glsl files)
https://github.com/mrdoob/three.js/pull/10750/files

//three-instanced-mesh does this for you:
//the PR if accepted would hardcode it into the library
const chunks = require('theseShaderChunks') //{ chunkName: chunkGLSL }
Object.keys(chunks).forEach( chunkName => THREE.ShaderChunk[chunkName] = chunks[chunkName]

//no
yourMaterial.instanced = true

//yes
yourMaterial.defines.INSTANCE_TRANSFORM = '' //your geometry now expects attribute vec3 aPosition, and others...

//too lazy, dont want attributes
const myInstanced = new require('three-instanced-mesh')(geometry,redPlasticMaterial,size)
myInstanced.setPositionAt(5, position)

Short answer:

Would it not be easier to have a few for example: ShaderChunks[‘Instancing’]?

No.

Proposing new entry points / hooks is slow, and work is not being done in this area in favor of the NodeMaterial where you will be able to build out all of this with additional syntax.
"Instancing" in particular is too specific and shouldn't be a chunk, instead it could be a #define NAME (which would mean it should be present in multiple chunks). Additional chunks should preferably be added is an abstract way as possible. Think of words like before, after, transform, computed, will,has, lighting, normal etc. "Instance" is not an operation as such, but a step in several operations, and mostly comes down to access data here in addition to there use data here instead of there vs after vertex transformation do instancing.

Yes.
With more generic names, happening at various stages, yes, three.js would become much more flexible.

@pailhead
Copy link
Contributor

pailhead commented May 9, 2018

if Material.onBeforeCompile() is properly used.

@Mugen87
I still may have doubts that i've properly used it, but one can't say that i haven't done my due dilligence :) i've tried, i've asked around for patterns, raised issues etc, asked on SO, asked on stack overflow. If you have insight into how to properly use it, i would genuinely honestly love to hear it. I think the fact that there hasn't been an example of it's proper use made for a year speaks volumes. Lot's of people have asked for this, i imagine there's a reason they're not using it.

I've put the proposal in the PR so it's easier to compare:
#14031

The example trims some 130 lines from the example @WestLangley mentioned here, and some 160 lines from GLTFLoader (not sure if anyone uses that one though). It costs roughly 10 additional lines in the core.

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

I also took a stab at this. I wanted to have instancing for Foliage, and I wanted to be able to edit the instances in:

  • count
  • instanced attribute values

I also wanted to be able to reference each instance individually, so I can remove it later or edit its attributes (position,rotation,scale).

I ended up writing a lot of code, mostly due to abstractions which are required to achieve this, the end result was being able to render millions of instances while being able to manipulate them at runtime.
Some problems I ended up solving:

  • material rewriting (based on @pailhead's approach, that was very helpful)
    • have to also create a .customDepthMaterial to get shadows to work
  • instance index <-> reference mapping
  • dynamic resizing of number of instances, including standard things like over-allocation to reduce chance of rebuilding the attributes entirely.
  • make random removes cheap on average ( O(1) ) (e.g. index 1234 out of 5000 instances)
  • spatial index abstraction on top of that, to only include instances that are visible, this removes a limit of number of instances to some degree
  • binary serialization/deserialization. Reducing time and size footprint of the instance set.

@titansoftime
Copy link
Contributor Author

have to also create a .customDepthMaterial to get shadows to work

Is this what you did to enable casting of shadows? I have been struggling for days trying to figure this out. Receiving shadows was no issue.

Also, very cool regarding the ability to reposition/rotate instances. Mine does not allow that.

Question regarding removing instances? Does this not require you to basically recreate the BufferGeometry? This would have noticeable runtime performance issues yea?

For my spatial indexing, I created a simple cell/chunk system based on scene x/z dimensions and number of chunks per axis. The one issue I had to resolve was frustum culling. For each cells geometry I had to basically create a box of vertices for each cell and set the geo boundingSphere based off those points, otherwise things would prematurely get culled. Curious as to your approach.

@titansoftime
Copy link
Contributor Author

@pailhead

I've read through your posts and it seems that the main issue with my proposal is that it does not conform with the direction of the API.

That is understandable, but I figured adding a couple new chunks to the existing couple dozen would be fine.

a gotcha is that this function must not include any branching logic, meaning there is no if else in there, otherwise it behaves rather erratically. Not a bug though:

I'm curious as to what additional logic you are referring to. At least for my use case it was simply enough to alter the transformed position/rotation in that one spot (after begin_vertex). What additional logic would be needed for instance placement?

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

@titansoftime

Question regarding removing instances? Does this not require you to basically recreate the bufferGeometry? This would have noticeable runtime performance issues yea?

There are some tricks you can do. Simplest one is to not rebuild BufferGeometry and let it have more attributes than what you're actually using, at some later point you may decide that it's too big and it's not worth it to keep using all that memory and rebuild it, but that's an arbitrary point in time of your choosing.

For my spatial indexing, I created a simple cell/chunk system based on scene x/z dimensions and number of chunks per axis. The one issue I had to resolve was frustum culling. For each cells geometry I had to basically create a box of vertices for each cell and set the geo boundingSphere based off those points, otherwise things would prematurely get culled. Curious as to your approach.

I use an already existing BVH implementation and populate it with references to individual instance (references are just int32 values). BVH leaves have an actual bounding box (AABB) for the instance. Here's what a leaf looks like:

{
id: int32,
x0: double,
y0: double,
z0: double,
x1: double,
y1: double,
z1: double
}

My culling code looks like this:

camera.updateProjectionMatrix();

const frustums = [];
const frustum = new THREE.Frustum();
frustum.setFromMatrix(new THREE.Matrix4().multiplyMatrices(camera.projectionMatrix, camera.matrixWorldInverse));
frustums.push(frustum);

group.update(frustums); // <- Magic happens here

Code for the Group.update:

/**
 * 
 * @param {THREE.Frustum[]} frustums
 */
InstancedFoliage.prototype.update = function (frustums) {
    const instances = this.instances;

    const elementsToAdd = new Set();
    const elementsToRemove = [];

    function visitLeaf(node) {
        const index = node.object;
        //TODO check screen space size to decide if element should be seen or not
        elementsToAdd.add(index);
    }

    //build visible set
    this.bvh.threeTraverseFrustumsIntersections(frustums, visitLeaf);

    // console.log(`Visible: `, elementsToAdd.size);


    //remove those that are no longer visible
    instances.traverseReferences(function (ref) {
        if (!elementsToAdd.has(ref)) {
            //no longer visible
            elementsToRemove.push(ref);
        } else {
            //visible, and is already in the group, update the set
            elementsToAdd.delete(ref);
        }
    });

    //cull instances that are no longer visible
    elementsToRemove.forEach(function (ref) {
        instances.remove(ref);
    });

    const elementData = [];

    //process entities that have become newly visible
    for (let index of elementsToAdd) {
        //read transform data for instance
        this.data.getRow(index, elementData);

        const positionX = elementData[0];
        const positionY = elementData[1];
        const positionZ = elementData[2];

        const rotationX = elementData[3];
        const rotationY = elementData[4];
        const rotationZ = elementData[5];
        const rotationW = elementData[6];

        const scaleX = elementData[7];
        const scaleY = elementData[8];
        const scaleZ = elementData[9];

        const i = instances.add(index);

        //apply instance transforms
        instances.setPositionAt(i, positionX, positionY, positionZ);
        instances.setRotationAt(i, rotationX, rotationY, rotationZ, rotationW);
        instances.setScaleAt(i, scaleX, scaleY, scaleZ);
    }

};

I have a compact binary table for keeping instance transforms beside BVH, that way BVH remains fairly small and is traversed quite quickly.

You probably noticed, but this means that I have to essentially compute a bounding box for every instance. This is actually not that big a deal, I compute one for each instance specifically, but you could use an overestimated one by taking the bounding sphere and scaling that, for my usecase I'm okay paying extra for creation of an instance for the benefit of having a really tight box at runtime. That's also a reason why I don't rebuild the entire thing every time but use efficient serialization format to just load thousands of already processed instances.

Also, regarding culling - i just turn it off on the THREE.Mesh -> mesh.frustumCulled = false. Since i do my own culling anyway, no point in having three trying to do anything here.

with respect to performance. I made a small toy example with 1,000,000 boxes.
Scene volume: 2000x2000x2000
camera near plane: 0.01
camera far plane: 100

moving camera around takes under 1ms on my hardware to run that InstancedFoliage.update. Here's a sample "bad" frame:
image
entire frame took 1.4ms here, including rendering. You can also make out removal operations there, they took about 0.2ms in this case.

link: http://server1.lazy-kitty.com/tests/instanced-foliage-1mil/

@pailhead
Copy link
Contributor

pailhead commented May 16, 2018

@titansoftime

Also, very cool regarding the ability to reposition/rotate instances. Mine does not allow that.
I'm curious as to what additional logic you are referring to.

We're talking about different things.

  1. I don't understand your implementation or proposal. reposition and rotate are the two most basic operations i want to do with instances. You mentioned that you achieved this with a few lines but can you rotate the instances in your example or not? Can they cast shadows? Can they be non-uniformly scaled? These are all the expectations one might have from an api such as the one you described.

  2. I'm talking about the logic in onBeforeCompile.

    Would this not be much more user friendly? If a user wants to take advantage of instancing (who wouldn't),

    Three.js has an api that allows you to do this extension, without asking the whole world to conform to your use case. I was just mentioning that it doesn't make things much easier, even though it exists. And was mostly responding to @Mugen87:

    if Material.onBeforeCompile() is properly used.

    Simple example, i wrote InstancedMesh, I like that one, you like this one. It's going to be hard for either one of us to just agree, but the difference is, i've actually had a PR here for a year :) I LIKE INSTANCES TO CAST SHADOWS you don't need that - different use cases.

  3. Is this what you did to enable casting of shadows? I have been struggling for days trying to figure this out. Receiving shadows was no issue.

    Not at all, this is what I did, the man said so himself:

    material rewriting (based on @pailhead's approach, that was very helpful)
    have to also create a .customDepthMaterial to get shadows to work

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

@titansoftime this is the second time you're participating in an instancing issue here. The first time you were in support of a really bad solution, and an inspiration to writing InstancedMesh. You posted on SO and the only reply was mine, and i tried to give you more insight into how things work. I thought you tried it, but here you are again asking for the same thing. You say you're a busy individual, but i really think you're trying to reinvent the wheel.

I tell you that you are asking for the same thing, you don't believe it, but then you keep asking for more of the InstancedMesh! Think it through, maybe there's one more thing missing for you to be convinced. We established that having shadows is nice, so thats already less like your proposal and more like InstancedMesh.

If I’m not overlooking anything, it would just require a few chunks, an instancing_pars chunk

So yeah, in short you're overlooking a lot of things :)


@WestLangley and @Mugen87

I think you guys are wasting time on onBeforeCompile (mugen and "proper" usage that never happened) and rewriting proposed PRs (west). This is fine, it's your time, it's your life, it's your right. Piece, love ❤️ and happy feelings!

What i don't understand is why couldn't you go to the InstancedMesh PR for example and say:

Hey we're ditching this.

Why couldn't you go to one of the onBeforeCompile issues and go:

Hey, this actually works, i know how to properly use it just need the time to write examples, please be patient.

It would be respectful towards my time. This is what's going on here?

Ultimately i just want a single file to have @pailhead in it, to show my hard work, dedication and love towards graphics.

(based on @pailhead's approach, that was very helpful)

This kind of stuff feels really nice. But what i like doing more is learning. @Usnul could have maybe improved the npm module. Or pinged me if he had figured out a better way to do something if this was part of the core.

@titansoftime
Copy link
Contributor Author

@unsul Super cool stuff man.

@pailhead

Man, I don't even know where to begin. In respects to me saying "Also, very cool regarding the ability to reposition/rotate instances. Mine does not allow that." I said repositioning, not positioning. As in altering position at runtime. Obviously you want to position your instances...

I don't understand your issue with me asking Unsul a question about shadows...

In fact, I am not really enjoying reading walls of exasperation.

Closing this.

@pailhead
Copy link
Contributor

pailhead commented May 16, 2018

Youre using different terminology. I never really thought of a concept of "repositioning" something in 3d graphics. It's redundant, it feels like i would always be "repositioning" instead of "positioning". Even when i create a buffer, it has positions either as garbage or zeroes.

The term for updating attributes and such in three.js is dynamic.

I don't understand your issue with me asking Unsul a question about shadows...

It's redundant. You in particular have followed at least two discussions on this topic here. I've wrote you walls of text for a specific question on SO that also explains all these things. You shouldn't have been asking @Usnul, or at least not here, you should have been asking in a completely different thread.

Thank you for closing it though, this is the main issue, it should have never been open. This is a duplicate of various other PRs and issues.

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

@pailhead

@Usnul could have maybe improved the npm module. Or pinged me if he had figured out a better way to do something if this was part of the core.

I didn't want to waste your time. I prefer to figure things out on my own. After reading pretty much everything you wrote on Instanced Meshes, including your code - I came to a conclusion that our philosophies are quite different in how we think about this problem. In brief - compared to your implementation mine is less tightly coupled with THREE.js and is a lot more "over-engineered". If you want to collaborate towards a common solution - i'm quite open. If you decide you want that - feel free to ping me so we could have chat.

@pailhead
Copy link
Contributor

pailhead commented May 16, 2018

@Usnul

No, sorry if it came the wrong way. I've put it up on npm and licensed it and write walls of texts so it can be built upon and improved. I would definitely like to discuss the philosophies.

I'm curious what you mean by coupling since both seem to be coupled. My impression is that you've done a lot on the culling front and structures to efficiently do all of that on the CPU?

I didn't want to waste your time

Please waste my time with this :)

I'd really like to see your code, and am curious if it can be abstracted from just the drawing logic?
This is why i want to have a breadcrumb with my name somewhere. I'm interested in this topic, and want to work on it. Maybe before the code, we could come up with a proposal together, in writing? What it should do, what it shouldnt do etc. Drop me a line at bodu3d@gmail.com.

There's probably a whole bunch of things to be done interface wise, and optimization wise, but i think the rendering bit is more or less solved, and should carry across.

Maybe describe all the risks, pitfalls etc a user can encounter (this is why i asked everyone to submit use cases in the module docs, but received none 😢 ). What happens when you need static, what happens when you need dynamic, non-uniform scale, texture atlases, different material properties, culling etc. etc.

@titansoftime

Thanks for reopening, I think that this is not your own personal issue even though you opened it, it might be of some value for others to continue discussing this. You don't have to participate if it annoys you :)
And this actually:

I’d be more than happy to make a PR, if this is desirable.

@titansoftime titansoftime reopened this May 16, 2018
@titansoftime
Copy link
Contributor Author

I will create a PR this weekend. Not for the intent of merging, but just so that we can actually be on the same page. Perhaps my code will require less context clues than my terminology.

I am also super interested in Unsul's BVH approach to instance culling. I have to artificially inflate my bounding spheres for each "cell" in order to get good looking results, which of course results in less than ideal culling and the drawing of excessive polygons.

@pailhead
Copy link
Contributor

pailhead commented May 16, 2018

Perhaps my code will require less context clues than my terminology.

Thank you, I think it will. I was really annoyed when i tried to propose things without code, but now I see why it's not the best way :) Without some universally agreed upon glossary of terms, code is less ambiguous.

which of course results in less than ideal culling and the drawing of excessive polygons.

This may not be the safest of assumptions. More specifically, you may not really care about drawing a few polygons outside the frustum, if you're saving on the draw call overhead. What would be far more undesirable is to have a whole draw call wasted, only to find out you should cull in the vertex shader.

My understanding of this is that, since both the frustum/sphere information is in the same space where you issue that call, you can cull the call, by checking if it needs to even happen on an entity. The entity here is a mesh with geometry, you use a simplification (box or sphere) and test against the camera frustum which you also have available.

I can think of countless scenarios where you would have different culling approaches, depending on your need, when using instancing. In one of them, a looser sphere, might still be a good idea, if it culls thousands of objects outside the frustum.


What is culling in the context of instanced geometries anyway?

  1. Cull cell 0103 in the level quad tree because it's not seen in the camera. Cell 0103 contains "instanced_lamp_posts_0103" instance cluster. Out of many instanced_lamp_posts_${cell_id}, you cull only the ones in the cells that are not visible. The rest you draw with M draw calls, as many cells are seen in the camera. Each cell contains > 1 lamp posts, grouped under one draw call.

  2. Something like this? Does this even make sense or would it just run slower?

if( aCulled ) {
  gl_Position = vec4(2.,2.,2.,1.);
  return;
}
#include <my_vertex_shader>

@titansoftime

If you could maybe describe this better with words (not sure how to do this one with code) i think it would be extremely helpful, what are you culling with your spheres, how are you culling it?

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

@pailhead

  • your approach is for fixed number of instances, mine is dynamic
  • your approach is for visualization only, mine includes spatial reasoning
  • your approach is for all instances to be in GPU memory, mine is intended for visualization of a fraction of a very large set
  • yours tracks instances by index, mine by a reference, which only makes a difference when you have the ability to remove instances though

@pailhead
Copy link
Contributor

pailhead commented May 16, 2018

I'm curious to see what all that means, do you have some code to share? I'm curious what happens with webgl once you figure out what's visible what not etc. How are you managing the attribute with references, you have to at some point store this into a typed array?

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

@pailhead

What is culling in the context of instanced geometries anyway?

I cull on CPU in the context of instanced geometry, essentially only one geometry is ever submitted in a draw call, the attributes of that geometry and number of requested instanced to draw changes, sometimes attributes are rebuilt when arrays' capacity becomes much larger than actual number of drawn instances. I think we're on the same page - I implemented this system in order to reduce number of draw calls to 1.

How are you managing the attribute with references, you have to at some point store this into a typed array?

Not the most elegant solution, i'm sure, it's a two-way map essentially, index <-> reference

PS: I included an example
http://server1.lazy-kitty.com/tests/instanced-foliage-10mil/
http://server1.lazy-kitty.com/tests/instanced-foliage-1mil/
I plugged population process into a separate "Thread", so it's fairly slow, but you can interact with the scene. Also, insertion like this, one element at a time, is pretty much worst case scenario for BVH, you end up with deeply unbalanced tree and poor quality hierarchies (not very good grouping), so population process slows down as it goes, as tree becomes larger - it takes longer to find a good place for a new node and refitting is linear with depth.

@pailhead
Copy link
Contributor

You're taking a slice out of this huge dataset and rendering it with one call with one InstancedBufferGeometry?

@Usnul
Copy link
Contributor

Usnul commented May 16, 2018

that's exactly it

@pailhead
Copy link
Contributor

pailhead commented May 17, 2018

It could work with non instanced meshes though too? I'm curious if it could have been built on top of InstancedMesh. As it only provides a naming convention to three attributes and methods to set data in them. The rest is just wiring to render correctly with the shadow maps and such.

Once you use your BVH you could render 1 or more either Mesh or InstancedMesh. I wonder if the abstractions could be split that way. I was kinda hoping someone would use the npm module to try something like this on top of it, but no one did so far :(

@Usnul
Copy link
Contributor

Usnul commented May 17, 2018

@pailhead
I appreciate your desire. We could talk about that module if you want. My implementation is dynamic in size, meaning you can add/remove instances, which your implementation doesn't permit - i guess that's the largest different really.

@pailhead
Copy link
Contributor

Ty, just point me to the code, I’m curious to see how it’s being managed.

@Usnul
Copy link
Contributor

Usnul commented May 18, 2018

@pailhead
I work on non-public repository. I would be happy to share the code, if you ping me privately.

@mrdoob
Copy link
Owner

mrdoob commented Sep 16, 2019

Closing in favor of #17505.

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

No branches or pull requests

7 participants