I have changed the new PMREMGenerator and the associated PMREMNode #28334

Spiri0 · 2024-05-11T01:01:51Z

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:

… 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

Spiri0 · 2024-05-11T01:50:42Z

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

Mugen87 · 2024-05-11T08:26:29Z

Why do you need a larger size for PMREMs in fromScene()?

Spiri0 · 2024-05-11T09:22:24Z

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.
I noticed that the textures get better in proportion to the size value and concluded that this was the right place to adjust the renderTarget texture size.
Where else do I adjust the size of the renderTarget texture?

Mugen87 · 2024-05-11T09:36:23Z

Wait, I have edited my previous comment. I recall #23322 now. It's probably best if @elalish reviews this PR.

Mugen87 · 2024-05-11T09:42:56Z

We really should keep features in sync whenever possible so please also update the original PMREMGenerator as well.

Mugen87 · 2024-05-11T09:45:40Z

examples/jsm/nodes/pmrem/PMREMNode.js

@@ -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 ) {


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.

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?

Um, I don't understand. It seems the issue in the fiddle can be fixed without enhancing the API of PMREMGenerator.

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.

Spiri0 · 2024-05-11T10:03:58Z

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.

Mugen87 · 2024-05-11T10:44:39Z

Do you mind demonstrating the issue with a fiddle?

Spiri0 · 2024-05-11T11:02:59Z

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.

Spiri0 · 2024-05-11T12:47:22Z

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.
In addition, it can certainly be made more compact because I do everything with shaders and wgslFn, the basic elements, because I don't know the other node materials well.
I can expand the example. My impression is that you would like to avoid wgsl in the examples, which is why I didn't touch it.

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.

Mugen87 · 2024-05-11T14:03:16Z

Fiddle: https://jsfiddle.net/7fgjueh5/

elalish · 2024-05-14T17:23:42Z

examples/jsm/renderers/common/extras/PMREMGenerator.js

 	 */
-	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) ) {


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.

Spiri0 · 2024-05-20T03:45:47Z

@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?
But that would be a separate topic

Mugen87 · 2024-05-20T09:02:01Z

The API of PMREMGenerator in src should also be updated, see #28334 (comment).

Mugen87 · 2024-05-20T09:03:27Z

examples/jsm/nodes/pmrem/PMREMNode.js

@@ -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 ) {


That comment can be removed.

Spiri0 and others added 3 commits May 11, 2024 02:22

Update PMREMNode.js

d2ef275

commented out WebGLCoordinateSystem

Update PMREMNode.js

20081de

Mugen87 reviewed May 11, 2024

View reviewed changes

elalish reviewed May 14, 2024

View reviewed changes

Mugen87 reviewed May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I have changed the new PMREMGenerator and the associated PMREMNode #28334

I have changed the new PMREMGenerator and the associated PMREMNode #28334

Spiri0 commented May 11, 2024

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024 •

edited

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024 •

edited

Mugen87 commented May 11, 2024 •

edited

Mugen87 May 11, 2024 •

edited

Spiri0 May 11, 2024

Mugen87 May 11, 2024

Spiri0 May 11, 2024

Spiri0 commented May 11, 2024 •

edited

Mugen87 commented May 11, 2024

Spiri0 commented May 11, 2024

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024

elalish May 14, 2024

Spiri0 commented May 20, 2024

Mugen87 commented May 20, 2024

Mugen87 May 20, 2024

I have changed the new PMREMGenerator and the associated PMREMNode #28334

Are you sure you want to change the base?

I have changed the new PMREMGenerator and the associated PMREMNode #28334

Conversation

Spiri0 commented May 11, 2024

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024 • edited

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024 • edited

Mugen87 commented May 11, 2024 • edited

Mugen87 May 11, 2024 • edited

Choose a reason for hiding this comment

Spiri0 May 11, 2024

Choose a reason for hiding this comment

Mugen87 May 11, 2024

Choose a reason for hiding this comment

Spiri0 May 11, 2024

Choose a reason for hiding this comment

Spiri0 commented May 11, 2024 • edited

Mugen87 commented May 11, 2024

Spiri0 commented May 11, 2024

Spiri0 commented May 11, 2024

Mugen87 commented May 11, 2024

elalish May 14, 2024

Choose a reason for hiding this comment

Spiri0 commented May 20, 2024

Mugen87 commented May 20, 2024

Mugen87 May 20, 2024

Choose a reason for hiding this comment

Mugen87 commented May 11, 2024 •

edited

Mugen87 commented May 11, 2024 •

edited

Mugen87 commented May 11, 2024 •

edited

Mugen87 May 11, 2024 •

edited

Spiri0 commented May 11, 2024 •

edited