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

Examples: Add possibility to set encoding for Reflector/Refractor. #18709

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Feb 23, 2020

Fixed #18699.

@mrdoob mrdoob added this to the r114 milestone Feb 25, 2020
@mrdoob mrdoob merged commit cf6f798 into mrdoob:dev Feb 25, 2020
@mrdoob
Copy link
Owner

mrdoob commented Feb 25, 2020

Thanks!

@WestLangley
Copy link
Collaborator

For the record, #18699 (comment).

@mrdoob
Copy link
Owner

mrdoob commented Feb 25, 2020

For the record, #18699 (comment).

Hmm...

Fixed #18699.

Do you mind elaborating on how this fixes #18699?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 26, 2020

If the user sets WebGLRenderer.outputEncoding to THREE.sRGBEncoding, it's now possible to do the same for Reflector/Refractor. Before this PR, it was necessary to copy the sources and apply this modification on app level.

It's like when rendering to a render target. In this case, it's also necessary to define the correct encoding.

@WestLangley
Copy link
Collaborator

As I said, that is a work-around. Rendering should be in linear space for a proper workflow.

How does that "fix" the performance problem -- other than as a work-around in this particular use case?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Feb 26, 2020

Rendering should be in linear space for a proper workflow.

I don't think this assumption is correct for 100% of all use cases.

How does that "fix" the performance problem

It's because of this block:

} else if ( materialProperties.outputEncoding !== _this.outputEncoding ) {
initMaterial( material, scene, object );
}

The encoding is embedded in the shader programs such that a recompilation is necessary if the encoding changes. This can lead to a ping-pong effect which totally ruins the performance.

@WestLangley
Copy link
Collaborator

I understand all that, but there is a design problem.

It seems to me that telling the user to change the color space so as to prevent the renderer from recompiling every frame is not fixing the performance problem. It is sweeping the problem under the rug.

@mrdoob Rendering the scene to a render target in linear space (for a mirror, for example), and then rendering the scene to the default render target in sRGB space, will cause the materials to recompile every frame.

It is because of the code injection... A material has one shader -- not one shader for each color space.

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.

Performance problem with reflector and new outputEncoding option
3 participants