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

[WIP] Add CubeTexture blur #30165

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

[WIP] Add CubeTexture blur #30165

wants to merge 3 commits into from

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Dec 19, 2024

The idea here is to make the PMREM cubemap blur function available as a generic public blur method for a CubeTexture. So far I only have a sketch, but this being my first attempt at using TSL, I've run into some early problems that I'd like to ask @sunag about.

Copy link

github-actions bot commented Dec 19, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.4
79.05
339.4
79.05
+0 B
+0 B
WebGPU 486.48
134.97
486.85
135.11
+372 B
+139 B
WebGPU Nodes 485.95
134.87
486.32
135.01
+372 B
+137 B

🌳 Bundle size after tree-shaking

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

Before After Diff
WebGL 465.3
112.11
465.3
112.11
+0 B
+0 B
WebGPU 555.58
150.43
555.52
150.41
-54 B
-27 B
WebGPU Nodes 511.65
140.19
511.6
140.19
-54 B
-5 B

const cubeUVsampler=Fn((envMap, sampleDirection)=>{
return bilinearCubeUV( envMap, sampleDirection, mipInt, CUBEUV_TEXEL_WIDTH, CUBEUV_TEXEL_HEIGHT, CUBEUV_MAX_MIP );
});
material.fragmentNode = blur( { ...materialUniforms, latitudinal: latitudinal.equal( 1 ), sampler: cubeUVsampler } );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the heart of the refactor I'm attempting - make the sampling function something that can be passed in so I can use different ones for a CubeUV texture vs an actual CubeTexture.

@@ -214,7 +214,7 @@ export const textureCubeUV = /*@__PURE__*/ Fn( ( [ envMap, sampleDir_immutable,

} );

const bilinearCubeUV = /*@__PURE__*/ Fn( ( [ envMap, direction_immutable, mipInt_immutable, CUBEUV_TEXEL_WIDTH, CUBEUV_TEXEL_HEIGHT, CUBEUV_MAX_MIP ] ) => {
export const bilinearCubeUV = /*@__PURE__*/ Fn( ( [ envMap, direction_immutable, mipInt_immutable, CUBEUV_TEXEL_WIDTH, CUBEUV_TEXEL_HEIGHT, CUBEUV_MAX_MIP ] ) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error I'm getting is here: only the first two arguments are defined, so it crashes when it tries to read mipInt_immutable. My other comment shows where this is being called. I thought I had verified this kind of functional programming works in your TSL editor, but maybe something is subtly different? Am I hitting a real limitation of TSL here, or am I just doing something stupid? Any guidance @sunag?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems to be related to the first argument of TSL function, I think that if you change of ( envMap, sampleDirection ) => to ( [ envMap, sampleDirection ] ) => it will work.

The second parameter in JS function is reserved for builder, where you can access the related material, object and geometry. For example: ( [ envMap, sampleDirection ], builder ) =>.

It is also possible to pass parameters as objects for example: ( { envMap, sampleDirection } ) => and call using bilinearCubeUV( { envMap, sampleDirection } ), can be useful for functions with many parameters.

@sunag
Copy link
Collaborator

sunag commented Dec 19, 2024

I'd like to ask @sunag about.

I can help with whatever you need :)

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.

2 participants