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

PMREMGenerator extension: Now you can set texture size and position #30334

Open
wants to merge 33 commits into
base: dev
Choose a base branch
from

Conversation

Spiri0
Copy link
Contributor

@Spiri0 Spiri0 commented Jan 16, 2025

Related issue: #30071

I have modified the PMREMGenerator so that you can now change the texture size and position. To do this, I changed the values ​​to a parameter list. In my opinion, that makes more sense than having to constantly hand over all the values. This way you only need to specify the ones you want to change. Otherwise you would have to fill the empty spaces with undefined.
This change made it necessary to also adapt the examples webgpu_ocean, webgpu_postprocessing_ao, webgpu_postprocessig_ssr

Spiri0 and others added 23 commits December 2, 2024 07:48
Arrays are not currently taken into account by the wgslTypeLib. However, with the struct extension mrdoob#29908, arrays will also become important as a type.
Since textureSampleLevel is usable in compute shaders, this small PR allows sampler to be used in compute shaders for this purpose
enables the use of samplers in compute shaders
add background to environment reflection
Copy link

github-actions bot commented Jan 16, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 336.25
78.28
336.39
78.31
+131 B
+33 B
WebGPU 505.56
140.43
505.69
140.45
+131 B
+25 B
WebGPU Nodes 505.03
140.32
505.16
140.35
+131 B
+25 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.25
112.13
465.38
112.16
+132 B
+36 B
WebGPU 579.6
157.12
579.73
157.15
+132 B
+31 B
WebGPU Nodes 534.98
146.67
535.11
146.7
+132 B
+29 B

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 16, 2025

Please add the size and position to the PMREMGenerator of WebGLRenderer as well.

fromScene( scene, sigma = 0, near = 0.1, far = 100 ) {

@Spiri0
Copy link
Contributor Author

Spiri0 commented Jan 17, 2025

I'll take care of it over the weekend, luckily it's a small thing

@Spiri0
Copy link
Contributor Author

Spiri0 commented Jan 20, 2025

I remember how Sunag adjusted these lines to fix the reflection error. These were still at the old status in the webgl PMREMGenerator. I also adjusted that straight away. I see that no environment is rendered in the webgl_pmrem_test as was previously the case with the webgpu pmrem example. Therefore, this will never have been noticed.

const upSign = [ 1, 1, 1, 1, - 1, 1 ];
const forwardSign = [ 1, - 1, 1, - 1, 1, - 1 ];

Attila Schroeder added 2 commits January 20, 2025 08:18
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 20, 2025

const upSign = [ 1, 1, 1, 1, - 1, 1 ];
const forwardSign = [ 1, - 1, 1, - 1, 1, - 1 ];

Do you mind revert this change in src/extras/PMREMGenerator.js? I'm not sure it is correct since WebGPURenderer uses a different coordinate system and that might cause different signs in the WebGPU version of PMREMGenerator. Let's investigate this separately.

@Spiri0
Copy link
Contributor Author

Spiri0 commented Jan 21, 2025

No problem, I'm happy if I can contribute something to the project, even if it's not much.

* @return {WebGLRenderTarget}
*/
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 ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't we have these properties in class scope? e.g:

const sceneRT = new THREE.PMREMGenerator( renderer );
sceneRT.position = new THREE.Vector3( ... );
sceneRT.faceSize = 256;
sceneRT.fromScene( ... )

Maybe we will have more options in the future, that should help.

Copy link
Collaborator

@Mugen87 Mugen87 Jan 24, 2025

Choose a reason for hiding this comment

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

position and size are only relevant for fromScene() and have no purpose for the rest of the API. Hence, I would favor to keep both in the method signature. As a compromise, how about doing the following?

generator.fromScene( scene, sigma, near, far, options );

In this way, we can add position, size and potentially future configuration properties to options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me.

@Mugen87 Mugen87 added this to the r173 milestone Jan 24, 2025
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.

3 participants