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

WebGLCubeUVMaps: Add support for render targets. #23152

Merged
merged 1 commit into from
Jan 5, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 5, 2022

Fixed #22236.

Description

This PR adds support for render targets in WebGLCubeUVMaps. That means the render targets of cube cameras can now be used with PBR materials.

@elalish I've have enhanced PMREMGenerator such that it is possible to reuse existing render targets for the conversion process. I'm not sure how to implement the details of #22236 (comment) though.

TBH, it surprised me that the conversion per frame has such a decent performance (tested with webgl_materials_cubemap_dynamic and PBR materials). I had this different in mind when testing last year.

@Mugen87 Mugen87 requested a review from WestLangley January 5, 2022 13:58
@WestLangley
Copy link
Collaborator

@Mugen87 So far, this appears to be working for me.

renderer.info shows more shaders, but I expect that is just the non-disposed PMREM shaders.

I've have enhanced PMREMGenerator such that it is possible to reuse existing render targets

It appears that is appropriate now, since the PMREM render target always has the same parameters regardless of the inputs.

TBH, it surprised me that the conversion per frame has such a decent performance

Me, too. Perhaps you should also compare with a cube environment map just in case.

In fact the ms/frame PMREM performance statistics would be of interest. 😇

@elalish
Copy link
Contributor

elalish commented Jan 5, 2022

Wow, I'm impressed the performance is decent for a PMREM per frame too! I mean, I did try to make it as fast as I could, and it may well be that converting to halfFloats sped it up significantly. I too would love to see some frame rate comparisons.

My comment was about bringing back the old mipmaps-as-PMREM solution that was in three.js before my PMREMGenerator and was removed during @WestLangley's simplification. However, if this is adequately fast, then perhaps there's no need. Would be good to test a WebXR example with lighting estimation as well.

@mrdoob
Copy link
Owner

mrdoob commented Jan 5, 2022

@Mugen87 Should I merge this so we have some time to test performance?

@WestLangley
Copy link
Collaborator

bringing back the old mipmaps-as-PMREM solution that was in three.js

@elalish Yes, that is what #23144 is about -- just in case it is needed as a fall-back.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 5, 2022

@Mugen87 Should I merge this so we have some time to test performance?

Yes, that sounds good. We still need to fix the WebXR example with lighting estimation.

Comment on lines +73 to +74
this.isRenderTargetTexture = false; // indicates whether a texture belongs to a render target or not
this.needsPMREMUpdate = false; // indicates whether this texture should be processed by PMREMGenerator or not (only relevant for render target textures)
Copy link
Collaborator

@WestLangley WestLangley Jan 5, 2022

Choose a reason for hiding this comment

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

I am inclined to think these properties should be "private", since they are for renderer-use only.

this.__isRenderTargetTexture = false;
this.__needsPMREMUpdate = false;

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: I'm assuming it should be a double-underscore.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jan 5, 2022

Choose a reason for hiding this comment

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

Depending on how user or third-party code is developed both flags are actually required.

isRenderTargetTexture is just the pendant to all other is* properties and required for type checks. Besides, users have to set needsPMREMUpdate to true (similar to other needsUpdate flags) if they are using a dynamic render target as an env map applied to PBR materials and author the render target's data by themselves.

Copy link
Collaborator Author

@Mugen87 Mugen87 Jan 5, 2022

Choose a reason for hiding this comment

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

I was actually thinking about suggesting something like RenderTargetTexture (which extends from Texture) in order to work a bit easier with this type of texture. However, this idea is not yet mature^^.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, lets test performance in the meantime.

@mrdoob mrdoob merged commit f37eacb into mrdoob:dev Jan 5, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 5, 2022

Thanks!

@mrdoob mrdoob added this to the r137 milestone Jan 5, 2022
@WestLangley
Copy link
Collaborator

@mrdoob build required

@mrdoob
Copy link
Owner

mrdoob commented Jan 6, 2022

Done 👍

@WestLangley
Copy link
Collaborator

@Mugen87 Do the reflections look correct in your test cases?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 6, 2022

After a closer look, it seems the reflections along the x axis are flipped. Let me file a PR to fix this issue.

@arpu
Copy link

arpu commented Jan 6, 2022

@Mugen87 any plans for a sample to share?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 6, 2022

In #23158, I've updated webgl_materials_cubemap_dynamic.html to use MeshStandardMaterial.

@ligaofeng0901
Copy link
Contributor

I tested my page, and the performance is quite ok. I found there still is a framebuffer of _pingPongRenderTarget in PMREMGenerator, which updates in each frame. After running a few minutes, the framebuffer has been created and destoryed thousands of times. I think it's not necessary and can be optimized.
BTW, I also want to pass the rendertarget parameters to the .fromScene, in this way I can update a pmrem texture in each frame under my control. should the PMREMGenerator support better for the situation of updating textures in each frame.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 11, 2022

I found there still is a framebuffer of _pingPongRenderTarget in PMREMGenerator, which updates in each frame.

@elalish Do you think it's okay to remove the following line and add it to dispose() instead?

this._pingPongRenderTarget.dispose();

@WestLangley
Copy link
Collaborator

renderer.info shows more shaders, but I expect that is just the non-disposed PMREM shaders.

@Mugen87 The PMREM shaders used to be disposed of when they are used only once. Last I checked, they are not.

Maybe there is a way to determine when disposal is appropriate...

@elalish
Copy link
Contributor

elalish commented Jan 11, 2022

@Mugen87 Yes, that sounds good to me. @WestLangley I believe those shaders all get disposed when the dispose method is called on PMREMGenerator, which seems reasonable.

@ligaofeng0901
Copy link
Contributor

@Mugen87 Do you think if it's ok about adding a extra parameter to .fromScene? https://github.com/mrdoob/three.js/blob/dev/src/extras/PMREMGenerator.js#L103 . In this way the method can be used by a user controlled renderTarget.

@WestLangley
Copy link
Collaborator

@Mugen87 @elalish PMREMGenerator.dispose() is not being called. We used to do that at the app level.

Granted, we do not want to dispose() if PMREM is generated every frame, but that is not the typical use case.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 12, 2022

PMREMGenerator.dispose() is not being called.

It is called in WebGLRenderer.dispose().

WebGLCubeUVMaps only creates a single instance of PMREMGenerator. Buffering certain resources for the conversion process (3 materials and 1 render target) within the renderer is the only solution. It's important to ensure that we don't create these entities over and over again.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 12, 2022

Do you think if it's ok about adding a extra parameter to .fromScene? https://github.com/mrdoob/three.js/blob/dev/src/extras/PMREMGenerator.js#L103 . In this way the method can be used by a user controlled renderTarget.

I'm not sure about the dimensions but other render target parameters should not be configurable by the user, IMO.

@ligaofeng0901
Copy link
Contributor

I'm not sure about the dimensions but other render target parameters should not be configurable by the user, IMO.

@Mugen87 OK, thanks! Maybe my use case is not very common. Generating a realtime enviroment map can use CubeCamera now. I just guess using .fromScene can also implement the effect in example https://threejs.org/examples/webgl_materials_cubemap_dynamic.html , and maybe fromScene has a better performance with an outer render target. I tested the two different ways on two macbooks, and got the different results.

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.

WebGLRenderer: fails to convert CubeCamera renderTarget to PMREM CubeUV format
6 participants