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

Performance problem with reflector and new outputEncoding option #18699

Closed
1 of 6 tasks
nimloth05 opened this issue Feb 21, 2020 · 4 comments · Fixed by #18709
Closed
1 of 6 tasks

Performance problem with reflector and new outputEncoding option #18699

nimloth05 opened this issue Feb 21, 2020 · 4 comments · Fixed by #18709
Labels

Comments

@nimloth05
Copy link

nimloth05 commented Feb 21, 2020

Description of the problem

If I add the line renderer.outputEncoding = THREE.sRGBEncoding; in the webgl_mirror.html example I get a massive performance problem. I posted this problem in the threejs discussion board here: https://discourse.threejs.org/t/performance-problem-with-outputencoding-and-reflection-in-linux/12901 The gist of it is that it seems that the shaders are recompiled every frame since the internal RenderTarget from Reflector does not use the correct output encoding. Mugens suggestion to add encoding: sRGBEncoding to the target parameters worked.

I tested this only under Linux Ubuntu 19.10.

I guess the question is now, how to fix? Should the user be able to pass in the encoding? Should reflector detect it automatically? I guess automatically would be nice, but it would also be nice to specify the other parameters of renderTarget so I guess extending the constructor options object would also not be the worst idea...

If the extending parameter solution is chosen by the dev team I could provide a PullRequest for that - if desired.

Three.js version
  • [X ] Dev
  • [ X] r113
Browser
  • [] All of them
  • [X ] Chrome
  • Firefox
  • Internet Explorer
OS
  • [] All of them
  • Windows
  • macOS
  • [X ] Linux
  • Android
  • iOS
@nimloth05
Copy link
Author

For anyone encountering this problem: Currently (as of r113 release) the only solution is to copy Reflector.js and patch it manually. I found an article that automates this process as part of the npm installation process: https://opensource.christmas/2019/4 This worked for me nicely.

@mrdoob mrdoob added this to the rXXX milestone Feb 22, 2020
@Mugen87 Mugen87 added the Addons label Feb 22, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 22, 2020

How about extending the options parameter?

options = options || {};
var color = ( options.color !== undefined ) ? new THREE.Color( options.color ) : new THREE.Color( 0x7F7F7F );
var textureWidth = options.textureWidth || 512;
var textureHeight = options.textureHeight || 512;
var clipBias = options.clipBias || 0;
var shader = options.shader || THREE.Reflector.ReflectorShader;
var recursion = options.recursion !== undefined ? options.recursion : 0;

We could have something like...

var encoding = options.encoding !== undefined ? options.encoding : THREE.LinearEncoding; 

...and use it in the ctor to setup the internal render target. In this way, it would not be necessary to change Reflector's signature (which would be necessary if we would pass renderer).

@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

@Mugen87 sounds good to me 👍

@WestLangley
Copy link
Collaborator

Just so you are aware, a reflector would typically be rendered in linear space, not in some other color space. So what is proposed here as a "solution" is really more of a work-around.

//

If the renderer is recompiling shaders every frame, that is a design problem which should be addressed separately.

//

@nimloth05 Another workflow is to render everything in linear space and then follow-up at the end with EffectComposer and a GammaCorrectionShader.

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

Successfully merging a pull request may close this issue.

4 participants