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

made 'setProgram' and 'loadUniformsGeneric' public methods #6021

Closed
wants to merge 4 commits into from

Conversation

arose
Copy link
Contributor

@arose arose commented Feb 4, 2015

Changed setProgram and loadUniformsGeneric in WebGLRenderer so that they are public method and accessible from the outside. The goal is to be able to inject code during the object rendering phase, specifically just after or before the WebGL program is set. This allows, for instance, additional transformation matrices like modelViewMatrixInverse that depend on data from the to-be-rendered object without reverting to per-object materials. Please see #6010 for a discussion. Also see #5208 for another example where methods of WebGLRenderer were made public to allow for custom code. Thanks for considering this.

@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2015

Looks good to me!
Could you also provide an example to make sure that this doesn't break in the future?

@arose
Copy link
Contributor Author

arose commented Feb 5, 2015

Could you also provide an example to make sure that this doesn't break in the future?

Of course, I will think of something.

@arose
Copy link
Contributor Author

arose commented Feb 7, 2015

Added an example that patches the WebGLRenderer to update a pickingColor uniform to realize GPU-based picking of geometry instances using a single material.

http://arose.github.io/demo/public-renderer-methods/examples/webgl_interactive_instances_gpu.html

@WestLangley
Copy link
Collaborator

You are using MeshLambertMaterial without any lights in the scene. Try:

scene.add( camera );
camera.add( new THREE.PointLight( 0xffffff, 1 ) );

Your highlight box seems to be unusually large. Could it be an illusion?

//

I am not seeing the compelling benefit of this PR in this example -- I am not saying it is not there, just that I do not see it. The example has 2000 draw calls in a static scene.

Can you please explain what you would have had to do differently if the features of this PR were not available to you?

@arose
Copy link
Contributor Author

arose commented Feb 8, 2015

Changed the example [1] so that one can compare the 'single material' vs the 'one material per instance' approach. This PR makes it possible to use a single material for multiple geometry instances while allowing to change the material's uniforms on a per instance basis.

The updated examples allows setting the number of geometry instances drawn and disabling the 'single material' approach. Notice that initialization is about twice as fast or faster for the 'single material' approach. You may open the browser's developer console to see some stats.

Thanks for the suggestions @WestLangley!

Your highlight box seems to be unusually large. Could it be an illusion?

Yeah, it often looks quite big because of rather "spike" form of the model. Made it nonetheless a bit smaller and fixed the transparency so that front and back are always visible.

Can you please explain what you would have had to do differently if the features of this PR were not available to you?

Basically, just use a separate material for each instance, which I added to the example for comparison.

[1] http://arose.github.io/demo/public-renderer-methods/examples/webgl_interactive_instances_gpu.html

@arose
Copy link
Contributor Author

arose commented Feb 8, 2015

One thing I haven't yet incorporated in the example is adding a new transformation matrix uniform. For example the modelViewProjectionMatrix. Without this PR this would only be possible by calculating the modelViewMatrix yourself and not be using the one that will be calculated by WebGLRenderer.

Just to be clear, I think this PR allows you to do potentially brittle things (and the example is not free from that) and I would very much like to see an API that allows for object-specific uniforms. However I don't know if there is enough demand and also not how such an API should look like. Maybe it is just too specific and exposing some more internals of WebGLRenderer is the right way to go.

@WestLangley
Copy link
Collaborator

Demo is much improved. Thanks!

One thing I haven't yet incorporated in the example is adding a new transformation matrix uniform.

Maybe it is best not to do that here. I think what you have is complicated enough. : - )

Just to be clear, I think this PR allows you to do potentially brittle things

Agreed. Hence I am on the fence about this one.

Another approach may be to allow for user-specified callbacks -- rather than exposing internals. It is a subtle difference, I know.

@arose
Copy link
Contributor Author

arose commented Feb 9, 2015

Another approach may be to allow for user-specified callbacks -- rather than exposing internals. It is a subtle difference, I know.

I guess such a callback could better communicate why it is there compared to just exposing internals.

The following function could be used to implement such a callback. It would need to be called in renderBuffer, renderBufferDirect and renderImmediateObject, just after setProgram is called..

function callPreRenderCallback( camera, lights, fog, material, object ){

    if( object.preRenderCallback ){

        object.preRenderCallback( camera, lights, fog, material );

    }

}

Alternatively, the callback could be a method of WebGLRenderer instead of object.

function callPreRenderCallback( camera, lights, fog, material, object ){

    if( _this.preRenderCallback ){

        _this.preRenderCallback( camera, lights, fog, material, object );

    }

}

Not sure about the naming, though.

Let's wait what @mrdoob has to say.

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2015

Yeah. I'm not sure this is good. A callback could be nicer but I worry about performance regressions...

For this specific use case, would it help if Mesh had a color property that that single material could use?

@tarelli
Copy link

tarelli commented Feb 23, 2015

I think we are in need of something very similar (extend setProgram from a custom renderer) and in our case it's not just a matter of adding something else that would have to happen after setProgram is called (the callback approach) but more a matter of replacing some of the internal setProgram logic itself so I'd +1 the original proposal from @arose.

@arose
Copy link
Contributor Author

arose commented Feb 23, 2015

For this specific use case, would it help if Mesh had a color property that that single material could use?

So if there is Mesh.color (Line, Sprite, ... should be handled similarly) we could have

function refreshUniformsCommon ( uniforms, material, object ) {
    // [...]
    if( object.color ){
        uniforms.diffuse.value = object.color;
    }else{
        uniforms.diffuse.value = material.color;
    }
    // [...]
}

instead of

function refreshUniformsCommon ( uniforms, material ) {
    // [...]
   uniforms.diffuse.value = material.color;
    // [...]
}

This would work for my GPU picking use case. A more general approach to object-specific uniform values could be a .uniformValues property in Mesh (and Line, ...) containing a plain object with keys and values. If a key matches a material uniform name the material's uniform value is replaced with the objects value.

For my other use case I need the modelViewMatrixInverse, the modelViewProjectionMatrixInverse, the modelViewProjectionMatrix and the modelViewMatrixInverseTranspose. These could be added by looking if there are uniforms in the material with such names and if so updating the respective matrices.

I could prepare a PR if you think this could be the way to go.

@arose
Copy link
Contributor Author

arose commented Feb 24, 2015

[...] a matter of replacing some of the internal setProgram logic itself [...]

While my use cases might be solved with more specific changes to the WebGLRenderer (see my above comment), I still find my original PR appealing as it allows some tinkering without the need to change the original three.js source. However, by making the methods public, developers might expect them to be a stable API. In light of such expectation the example is probably a bad idea. Maybe prefix the methods by an underscore to indicate that they are actually private and scrape the example.

@tarelli If you have the time, why don't you explain your use case in another issue. Maybe there is something valuable that can indeed be added to the three.js library. Anyway, sometimes people don't have the time and just need a way to tinker with the internals (without the need to change the original source).

@WestLangley
Copy link
Collaborator

For this specific use case, would it help if Mesh had a color property that that single material could use?

I think that allowing a renderable object to override any of its material's uniforms would be an excellent enhancement.

@tarelli
Copy link

tarelli commented Jul 20, 2015

Sorry for the delay, been very busy, here is an example for what we are doing: https://github.com/openworm/org.wormsim.frontend/blob/development/src/main/webapp/js/components/dev/customrenderer/AnimationCustomRenderer.js#L4521.
We have a custom version of the renderer which has an extra method setCurrentMatrix which allows injecting at every step what transformations have to be performed for a given animation. All the changes are in setProgram. The approach we are currently using is replacing the whole renderer which I don't like it as it introduce a lot of redundancy and will require updating a lot of code with each three.js release. Ideally it would be possible to do the above just customising setProgram.

As another point I have found something that I don't understand.If I modify the file three.js itself replacing WebGLRenderer with our version I get 50fps for a given scene. If I replace the renderer later on (still before there is any renderer initialised) just reassigning THREE.WebGLRenderer={...} performance for the same scene go to 35fps. The code is exactly the same, is there some catch due to how Javascript work that would make a replaced version of the renderer slower than one declared in the original three.js file itself?

Update
This is a video of the custom animation. The transformations are streamed (as they could be calculated in real time) rather then being hardcoded in the Collada file which only defines mesh and skeleton.

@arose
Copy link
Contributor Author

arose commented Aug 17, 2015

I intend to look at this after the refactoring of WebGLRenderer slows down.

@mrdoob
Copy link
Owner

mrdoob commented Aug 18, 2015

Do you mind giving another go? I think I'm done refactoring... 😅

@arose arose mentioned this pull request Aug 25, 2015
@arose
Copy link
Contributor Author

arose commented Aug 26, 2015

@mrdoob I tried another approach in #7048 -- the code is much less intrusive than in #6150.

@tarelli Is it correct that you basically replace object.skeleton.boneMatrices with another array of matrices?

In your code you replaced

_gl.uniformMatrix4fv( p_uniforms.boneGlobalMatrices, false, object.skeleton.boneMatrices );

with

_gl.uniformMatrix4fv( p_uniforms.boneGlobalMatrices, false, newMatrix );

Is it necessary to have this code inside the render loop? Don't you have access to object.skeleton.boneMatrices outside of WebGLRenderer so that you can change the matrices befor calling renderer.render()?

HTH Alexander

@mrdoob
Copy link
Owner

mrdoob commented Aug 29, 2015

@mrdoob I tried another approach in #7048 -- the code is much less intrusive than in #6150.

Much nicer!

@mrdoob mrdoob closed this Aug 29, 2015
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

4 participants