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

WebGLRenderer: Allow overrideMaterial to be changed on a per-object b… #21774

Closed
wants to merge 1 commit into from
Closed

WebGLRenderer: Allow overrideMaterial to be changed on a per-object b… #21774

wants to merge 1 commit into from

Conversation

fleroviux
Copy link
Contributor

@fleroviux fleroviux commented May 3, 2021

Description

Right now when rendering a scene its override material will only be read once at the start of renderObjects(), which means that it cannot be changed e.g. in onBeforeRender() on a per-render-object basis. But for our G-buffer implementation we wanted to be able to pick an override material based on mesh/material variables like morphTargets/morphNormals or skinning which would be a bit expensive to enable for all render objects.

This patch moves the override material logic into the renderObject() method, after the call to onBeforeRender(), which faciliated us to do this. As a side effect onBeforeRender() now receives the original mesh material as parameter instead of the override material, which is desirable in my opinion.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2021

The skinning property will be gone with r129, see #21788.

Assuming there would be no morphTargets and morphNormals property, would you still need this PR?

@fleroviux
Copy link
Contributor Author

WebGLPrograms would then automatically add flags for skinning and morph targets to the program and its key right?
But that probably wouldn't work for RawShaderMaterial, which we use to render into multiple render targets and is still necessary with the merged MRT feature ( see #16390 (comment) ).

So replacing morphTargets with something like MorphMesh would work if there is a solution for RawShaderMaterial to be automatically compiled into different programs based on the mesh or for MRT to work with a normal ShaderMaterial.

Please correct me if I have any misconceptions.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 6, 2021

The idea is not to introduce new class like MorphMesh but to detect whether morph targets are defined in the geometry or not. The renderer is then able to produce different programs even for a single material that is used for a normal and animated mesh.

@fleroviux
Copy link
Contributor Author

I looked at the PR #21788 and noticed that WebGLShadowMap already achieves something similar to what we want to do with renderBufferDirect(), I didn't know about that method before. Might actually make for an overall cleaner solution than using overrideMaterial and onBeforeRender, but I have to evaluate that. If that works for us then this PR wouldn't be necessary anymore.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 5, 2021

With r131, morphTargets/morphNormals have been removed. That means you can use the same override material for correctly processing animating meshes.

I looked at the PR #21788 and noticed that WebGLShadowMap already achieves something similar to what we want to do with renderBufferDirect(), I didn't know about that method before.

Also noticed that with r131 the WebGLShadowMap class does not need the former materials variants logic anymore. So a single set of depth materials is sufficient for processing the scene. Previously, it was necessary to have separate depth materials for skinned and morphed meshes. Custom implementations also benefit from this simplification.

Ideally, it's now not necessary anymore to modify renderObjects() with this PR.

@fleroviux
Copy link
Contributor Author

Thank you! I'll make sure to try this once we update.

@fleroviux fleroviux closed this Aug 5, 2021
@fleroviux fleroviux deleted the feature/per-object-override-material branch August 10, 2021 09:18
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.

2 participants