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: Add support for multiple programs per material. #20135

Merged
merged 7 commits into from
Mar 5, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Aug 20, 2020

Fixed #15047.
Fixed #19056.
Fixed #17701.

A material can now reference to multiple shader programs. This avoids expensive shader recompilation effects and solves some conceptual issues like #17701. Since this PR is a deep change in how WebGLRenderer manages shader programs, I recommend to merge it at the beginning of a dev cycle.

The performance of existing apps should not be affected by this change since the general code flow in the renderer is the same. That said, it's still room for performance improvements since I've noticed during the development that certain tasks like the build of uniforms lists could be optimized.

BTW: I've renamed initMaterial() to getProgram() since the method now returns an instance of WebGLProgram to the caller.

@cedemax
Copy link

cedemax commented Sep 16, 2020

Would this fix the closed issue #19566? If so, is there any example or tips on how to modify the use of overridematerial (in SSAOPass for example) to use this new feature? Thanks!

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 16, 2020

Would this fix the closed issue #19566?

Yes, I think so.

@mrdoob
Copy link
Owner

mrdoob commented Mar 1, 2021

Lets try merging this one this cycle! Do you mind resolving the conflicts?

@mrdoob mrdoob added this to the r127 milestone Mar 1, 2021
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 1, 2021

Conflicts resolved!

@mrdoob
Copy link
Owner

mrdoob commented Mar 2, 2021

@Oletus Do you have time to take a look at this PR?

Copy link
Contributor

@Oletus Oletus left a comment

Choose a reason for hiding this comment

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

This is definitely a welcome change! Few things I'd like to see changed inline but nothing major.

Also, what I'm a bit worried about is loading multiple versions of the same shader program unintentionally, though it's obviously better than wrecking runtime performance by constantly recompiling programs. For example, it's easy to end up using multiple framebuffer formats in a situation where just using sRGB for everything would be fine. Each format multiplies the number of shaders that need to be loaded, increasing load times. Would there be any suitable mechanism for communicating to the three.js app developer that their render pipeline may be using more variants of shaders than they expect?

src/renderers/WebGLRenderer.js Show resolved Hide resolved
src/renderers/WebGLRenderer.js Show resolved Hide resolved
src/renderers/WebGLRenderer.js Show resolved Hide resolved
src/renderers/WebGLRenderer.js Show resolved Hide resolved
Copy link
Contributor

@Oletus Oletus left a comment

Choose a reason for hiding this comment

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

Looks good!

@mrdoob
Copy link
Owner

mrdoob commented Mar 5, 2021

Would there be any suitable mechanism for communicating to the three.js app developer that their render pipeline may be using more variants of shaders than they expect?

We could try to add this to https://github.com/threejs/three-devtools?

@mrdoob mrdoob merged commit d2fa6ed into mrdoob:dev Mar 5, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 5, 2021

Maaaaaany thanks!

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