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
I have changed the new PMREMGenerator and the associated PMREMNode #28334
base: dev
Are you sure you want to change the base?
Conversation
… also made the position of the cubeCamera adjustable. The rendered textures being projected mirrored, so it was necessary to make sure that uvNode.x.negate() was executed in the PMREMNode. That was not the case.
commented out WebGLCoordinateSystem
To be on the safe side, I only commented out the old parts in the PMREMNode and did not delete them. Please check whether these can then be deleted so that the code remains clean |
Why do you need a larger size for PMREMs in |
I don't get any error messages but if you say it's the wrong place to adjust the renderTarget texture size then that's it. |
We really should keep features in sync whenever possible so please also update the original |
@@ -162,7 +162,8 @@ class PMREMNode extends TempNode { | |||
|
|||
const texture = this.value; | |||
|
|||
if ( builder.renderer.coordinateSystem === WebGLCoordinateSystem && texture.isPMREMTexture !== true && texture.isRenderTargetTexture === true ) { | |||
//if ( builder.renderer.coordinateSystem === WebGLCoordinateSystem && texture.isPMREMTexture !== true && texture.isRenderTargetTexture === true ) { | |||
if ( texture.isRenderTargetTexture === true ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: Please avoid addressing multiple issues with a single PR.
We can leave it for now but in the future try to fix different issues in different PRs please. That makes them easier to review and merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that it had to be one PR because the changes in both files (PMREMGenerator and PMREMNode) belong together for it to work properly. Was the idea wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Um, I don't understand. It seems the issue in the fiddle can be fixed without enhancing the API of PMREMGenerator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fiddle seems to offers more options than CodePen, so I'll look into that too. But I'm happy that it worked and that it can now be experienced live with the fiddle.
Thanks for doing that, Mugen
The one with the two issues: Of course thats already possible. If you just change the pmremNode without the sign correction in the x axis in the PMREMGenerator, then the x axis reflection is not correct and that is independent of the camera translation.
The camera translation has no influence at all and is therefore actually a different topic. The bottom line for me is that it's just important to get it done properly and if those are two issues I can live with that very well.
I'm happy that the PMREMGenerator has come and the pmremNode, as the WebGLCubeRenderTarget previously seemed simply out of place in WebGPU, even though it worked well.
The reason why I changed the check was this one. What you see here is independent of my expansion of the camera position. The reflections on the surface were wrong. They were reversed. So I traced back in the code where the UV coordinates were influenced and that was the location. The check was only carried out for WebGL coordinate system but it is also necessary in WebGPU. And if necessary for both worlds, then the distinction between cases is unnecessary. That's my logic. That's why I didn't delete it for the time being, but I'd like to ask to check it. Here is a screenshot without the changes. You can see that the spheres are reflected correctly but the textures are not. Therefore, a sign change in the PMREMGenerator was also necessary for the -x/+x textures in conjunction with the uv coordinate mirroring in the PMREMNode. I assume that the incorrect texture representation was not noticed because the example was only done with the colored spheres and they are symmetrical so that the reflection error could not be noticed with them. |
Do you mind demonstrating the issue with a fiddle? |
I even wanted to do that because I usually always use a CodePen for issues, but I had no idea where I could get the cube texture (px.png, nx.png, py.png, ny.png, pz.png, nz.png) in CodePen. Otherwise I would have done it quickly, because I don't like submitting issues without a CodePen example. So I decided to take the more painful route and get to work and analyze it myself. |
What do you think about to expand the example with the cubeBox with the envMap? For me that's just 39 lines. With the envMap the example simply looks more impressive. I made a CodePen here with the extended example code. https://codepen.io/Spiri0/pen/gOJOXdg?editors=0010 Of course this doesn't run on codePen but locally. |
Fiddle: https://jsfiddle.net/7fgjueh5/ |
*/ | ||
fromScene( scene, sigma = 0, near = 0.1, far = 100 ) { | ||
fromScene( scene, sigma = 0, near = 0.1, far = 100, size = 256, position = new Vector3( 0, 0, 0) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good change to me - I should have added size control to fromScene
when I made the PMREM size variable in the first place. It can also increase render speed by decreasing cache usage to set the size small when using rougher materials.
@Mugen87 is there anything left you would like from me regarding this PR? What do you think of my idea to add the environment map to the example? |
The API of |
@@ -162,7 +162,8 @@ class PMREMNode extends TempNode { | |||
|
|||
const texture = this.value; | |||
|
|||
if ( builder.renderer.coordinateSystem === WebGLCoordinateSystem && texture.isPMREMTexture !== true && texture.isRenderTargetTexture === true ) { | |||
//if ( builder.renderer.coordinateSystem === WebGLCoordinateSystem && texture.isPMREMTexture !== true && texture.isRenderTargetTexture === true ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment can be removed.
The renderTarget size was hardcoded to 256. I made this value adjustable. The setting is optional. The default value is 256
The position of the cubeCamera was also hard-coded and not changeable. I also made the position adjustable. This is also optional. The default position is the origin. When testing with textures, I noticed that they were mirrored around the y axis. That's why I have to ensure the uvNode.x.negate() call in the PMREMNode. This was only carried out for the WebGL coordinate system, but is also important for WebGPU so that it is correct
I tested a few different positions. Here is a screenshot with position (100, 100, 100) and renderTarget size 512: