Skip to content

Conversation

@TobiasNoell
Copy link

Related issue: #27141

Description

ThreeJS already supports scaling the effect of the environment map by the envMapIntensity property of a material.
For the node materials, it would be very handy to also have a envMapIntensityNode property available to setup more complex intensity values rather than just a single float value.

While a similar effect can already be achieved by material.envNode = cubeTexture( myCubeTexture ).mul( intensity ); for the WebGPURenderer, the WebGLRenderer currently only supports setting the environment via scene.environment = texture;, i.e. all materials share one common environment map.

For some use-cases it is desirable to scale the intensity of the environment map for each material individually.
It is already possible to do this via material.envMapIntensity = 0.5;. However, this allows to only set static values. For the node materials, it would be really good to have a envMapIntensityNode parameter which allows more sophisticated node based settings. For example material.envMapIntensityNode = THREENodes.pow(material.roughnessNode, THREENodes.float(4.0)); to let the environment map only contribute to the really mirroring parts of a surface.

@TobiasNoell
Copy link
Author

@sunag: I tried to also add the functionality in a meaningful way to WebGPU using a multiplication of the envMap. Happy to get some feedback.

P.S: I understand that WebGPU is the "new" thing and ultimately everything will shift towards it. But it would be really appreciated if we could still have the overall WebGL functionality kept more or less on par until this happens wherever this is possible. 👍

@github-actions
Copy link

github-actions bot commented Nov 9, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
668.7 kB (166 kB) 668.8 kB (166 kB) +161 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
449.8 kB (108.9 kB) 449.9 kB (108.9 kB) +161 B

@mrdoob mrdoob requested a review from sunag November 28, 2023 10:01
@TobiasNoell TobiasNoell force-pushed the add-env-map-intensity-node branch 2 times, most recently from 483ddce to 130da14 Compare November 29, 2023 09:28
@TobiasNoell TobiasNoell force-pushed the add-env-map-intensity-node branch from 130da14 to cdcd8da Compare November 29, 2023 09:29
@TobiasNoell TobiasNoell changed the title Add envMapIntensityNode for node materials Add envMapIntensityNode for node materials in WebGLRenderer Nov 29, 2023
vec4 envMapColor = textureCubeUV( envMap, worldNormal, 1.0 );
return PI * envMapColor.rgb * envMapIntensity;
return PI * envMapColor.rgb;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@WestLangley Are these changes appropriate?

I'm pinging you since one can easily overlook that this PR modifies shaders in the core.

Copy link
Author

@TobiasNoell TobiasNoell Nov 29, 2023

Choose a reason for hiding this comment

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

Maybe just for additional clarification:

Before: getIBLIrradiance, getIBLRadiance and getIBLAnisotropyRadiance were returning the environment intensity from the map with the scaling already applied inside the function before the value is returned. This had the drawback that only global scaling per material were possible.

Now: getIBLIrradiance, getIBLRadiance and getIBLAnisotropyRadiance are returning the unmodified environment intensity from the map. The scaling is then applied to their returned values after they are called in lights_fragment_maps.glsl.js (this is the only place that they are ever called inside ThreeJS). This has the advantage that we can define a pixel wise scaling per material (e.g. we use it in order to let the environment illumination affect only for specular parts of a material, which was not possible before).

Copy link
Collaborator

Choose a reason for hiding this comment

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

we use it in order to let the environment illumination affect only for specular parts of a material, which was not possible before

This is a physically-based shader, and the radiance and irradiance are jointly-consistent and scale together.

If I understand correctly, the OP wants to modify the shader so he can use Node to violate the physics of light in his app and have the environment intensity apply only to radiance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the additional explanation! Against this backdrop, I see this change critical, tbh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@TobiasNoell Would it be an acceptable workaround to apply your shader modifications via onBeforeCompile()?

Copy link
Author

Choose a reason for hiding this comment

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

The idea was to add a more generic envMapIntensityNode slot to the node materials (since we already have a envMapIntensity property for the classic materials).

It is true that the user can decide to setup a specific combination of nodes as input for envMapIntensityNode that would then yield a result that is not strictly correct in a physical sense, but still achieves the intended result.

In any case, it would be the user that is setting up the node. If the user decides to apply a (physically incorrect) spatially varying scaling of the environment for a material, the user should be aware of what its intentions are, right?

If the envMapIntensityNode is ignored or a node combination that yields any uniform floating value, everything would still remain strictly physically plausible.

Copy link
Author

@TobiasNoell TobiasNoell Nov 30, 2023

Choose a reason for hiding this comment

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

This is an example what we are trying to achieve, to make it more practical:

We have a scene that is illuminated by point lights. This gives a nice appearance including shadows on the diffuse parts of the surface, but the chrome like specular parts look too dark:
image

To make the chrome look nicer, we setup an environment map for the scene. This makes the chrome parts look nice, but destroys the appearance of the diffuse parts:
image

To fix this, we want to scale down the effect of the environment map for the rough parts of the surface, but this is only possible with the changes this PR proposes:
image
It is done by setting material.envMapIntensityNode = THREENodes.pow(material.roughnessNode, THREENodes.float(4.0));

It is true, that the last image is not strictly physically correct, but it looks the nicest out of the three 😄

All materials in the scene are node materials. If there is any better way to achieve the same I'm happy to hear it.

One additional note regarding the violation of the physics of light:
It should be mentioned that also with the classical materials it is already possible to do this by setting
material1.envMapIntensity = 0.5 and material2.envMapIntensity = 2.0 (i.e. different scaling factors per material). The WebGPURenderer even allows entirely different environment maps per material.

vec4 envMapColor = textureCubeUV( envMap, reflectVec, roughness );
return envMapColor.rgb * envMapIntensity;
return envMapColor.rgb;
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this PR, the methods getIBLRadiance(), getIBLIrradiance(), and getIBLAnisotropyRadiance() are no longer returning the units their names imply.

Copy link
Author

@TobiasNoell TobiasNoell Nov 29, 2023

Choose a reason for hiding this comment

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

Ok, got it (was not aware that the multiplication is also a unit conversion). I could revert everything there and keep the scaling / unit conversion inside these functions and thus revert all changes in envmap_physical_pars_fragment.glsl.js. Instead initialize envMapIntensityFactor in lights_fragment_maps.glsl.js as 1.0? This way it would be much less „invasive“.

Copy link
Author

@TobiasNoell TobiasNoell Nov 30, 2023

Choose a reason for hiding this comment

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

I pushed a less invasive version that requires no changes in envmap_physical_pars_fragment.glsl.js

@TobiasNoell
Copy link
Author

@sunag @Mugen87 @WestLangley After the extended discussions above and adaptions to the code, is there any chance this gets merged?

@WestLangley
Copy link
Collaborator

It should be mentioned that also with the classical materials it is already possible to do this by setting
material1.envMapIntensity = 0.5 and material2.envMapIntensity = 2.0 (i.e. different scaling factors per material).

That seems to me to be the natural solution for your use case. I am curious as to why that is not acceptable to you.

@TobiasNoell
Copy link
Author

That seems to me to be the natural solution for your use case. I am curious as to why that is not acceptable to you.

I want to scale the material.envMapIntensity according to the roughness of the material. However, the roughness is not a single scalar value but a map itself (i.e. an arbitrary material.roughnessNode).

Thus I'd need something like:
material.envMapIntensityNode = THREENodes.pow(material.roughnessNode, THREENodes.float(4.0));

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2024

I think your use case is too specific that it justifies a modification of WebGLRenderer.

Fortunately, you can achieve your requirements with WebGPURenderer. It will be normal that certain features will only be available with the new renderer and its node material. Also keep in mind that renderers/webgl-legacy/nodes/ is already deprecated as the name implies.

@sunag I feel the time is right to remove renderers/webgl-legacy now. Are you okay with that step?

@Mugen87 Mugen87 closed this Apr 17, 2024
@Mugen87 Mugen87 added this to the r164 milestone Apr 17, 2024
@TobiasNoell
Copy link
Author

That's fine, I don't have any particular interest of this being merged anymore because I already forked Three.

Is there some documentation how node materials can be used with the WebGPURenderer?
To me as a user it is a bit unclear what is the future proof way how Three should be used, which methodology is currently replaced by which and why 😄

A migration guide from WebGL to WebGPU would be a prerequisite IMO before stuff is removed. Also there are devices that still do not support WebGPU and require WebGL, so Three will stop working on these?

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2024

WebGPURenderer has a WebGL backend so if WebGPU isn't available on a device, it will fallback to WebGL 2.

Regarding the documentation, expect more to see in this regard during this year. WebGPURenderer gradually matures and at a certain point we will add documentation and migrating more examples to WebGPURenderer.

@sunag
Copy link
Collaborator

sunag commented Apr 18, 2024

@sunag I feel the time is right to remove renderers/webgl-legacy now. Are you okay with that step?

@Mugen87 For me ok, I will take care of this removal.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 4, 2024

@Mugen87 to clarify – are we removing support for node-based materials in WebGL? or is the idea that we can use node-based materials with WebGPURenderer, and WebGPURenderer will fall back to WebGL2 while still supporting those node-based materials (with some limitations).

@Mugen87
Copy link
Collaborator

Mugen87 commented May 4, 2024

or is the idea that we can use node-based materials with WebGPURenderer, and WebGPURenderer will fall back to WebGL2 while still supporting those node-based materials (with some limitations).

Yes, that is the plan.

Part of the discussion above mentioned the legacy node support for WebGLRenderer which has been removed with r164 in the meanwhile.

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.

5 participants