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

Clean up Translucency.js #19156

Merged
merged 2 commits into from
Apr 17, 2020
Merged

Clean up Translucency.js #19156

merged 2 commits into from
Apr 17, 2020

Conversation

marcofugaro
Copy link
Contributor

I was adapting the Translucency shader to work with MeshStandardMaterial, and I noticed that 90% of the code from the shader is from the Phong shader.

I cleaned it up and now it basically works like this:

ShaderChunk[ "meshphong_frag" ].replace( 
  "RE_Direct( directLight, geometry, material, reflectedLight );",
  "..."
)

The Pros of this are:

  • it's easier to understand and modify
  • it's up to date with the Phong material

The Cons of this are:

  • this also needs to be updated if ever the line RE_Direct( directLight, geometry, material, reflectedLight ); will be changed.

But I guess that's what CIs are for.

The updated example is the same of the current example since this is only a cleanup.
https://raw.githack.com/marcofugaro/three.js/translucency-update/examples/webgl_materials_translucency.html


In this PR also:

  • accidentally make the Translucency work also with Spotlights
  • fix typo in webgl_materials_translucency.html

However there is a thing I wasn't able to do: make the shader work without a texture passed in thicknessMap.
In this line, if no texture, the sampled color is 0, making the shader output color black. How do I remove it from the equation? With a uniform useTexture? Know of a better way?

" vec3 thickness = thicknessColor * texture2D(thicknessMap, uv).r;",


Also, I have some suggestions:

  • should we start using template literals in these shaders like we're doing in the ShaderChunks? Their browser support is the same as esmodules
  • We should rename this to SubsurfaceScatteringShader.js, that's more clear and easier to find in my opinion (I was looking for subsurface scattering the Three.js and was puzzled to find nothing at first).

@mrdoob mrdoob added this to the r116 milestone Apr 17, 2020
@mrdoob mrdoob merged commit 19beb8e into mrdoob:dev Apr 17, 2020
@mrdoob
Copy link
Owner

mrdoob commented Apr 17, 2020

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Apr 17, 2020

  • should we start using template literals in these shaders like we're doing in the ShaderChunks? Their browser support is the same as esmodules

Not until we figure out how to convert modules to non modules.

  • We should rename this to SubsurfaceScatteringShader.js, that's more clear and easier to find in my opinion (I was looking for subsurface scattering the Three.js and was puzzled to find nothing at first).

Sounds good to me!

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2020

It seems the PR changed only the jsm version but not the js verison of the code. The next time node utils/modularize.js is executed, the changes of the PR will be reverted.

@marcofugaro
Copy link
Contributor Author

Sorry @Mugen87 I tought examples/js was gonna be removed soon. I'll make the changes in the js folder in another PR.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2020

In the meanwhile, I'm not a fan of modifying, copying or enhancing shader chunks in experimental shader example code since these examples are hard to maintain and can easily break. A similar demo that made problems in the past is webgl_materials_envmaps_parallax.

I think I would prefer to avoid demos in the future that do stuff like that since they make it harder to move forward with changes to the core shaders.

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Apr 17, 2020

Well since the CI is now up, the user can be notified when some example breaks in the PR. So the user gets to know where the code he is modifying is also referred, and updates it accordingly.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2020

Yes, that's right. I'm criticizing the idea to add such examples in general. Updating core shaders is more cumbersome the more example code has to be changed because of this approach.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 17, 2020

I would advise to add such features to the core OR move it as a showcase to the forum.

Anyway, that should not block this particular change. I just wanted to put this somewhere since I've noticed these troubles during support. TBH, I had a different view on this topic in the past. But in the meanwhile, I think it's the more appropriate approach considering the size of the project.

@munrocket
Copy link
Contributor

It's unclear how to make new PR in examples, why we not updated the contribution guide?

@WestLangley
Copy link
Collaborator

WestLangley commented Apr 17, 2020

I agree with @Mugen87. I find this shader difficult to read.

I also think this example should have been a showcase originally... It relies on bunny_thickness.jpg.

@munrocket
Copy link
Contributor

We have 6 examples with water and only one with subsufrace scattering :D

webgl_refraction
webgl_shaders_ocean
webgl_shaders_ocean2
webgl_water
webgl_water_flowmap
webgl_gpgpu_water

Some of them literally the same.

@marcofugaro
Copy link
Contributor Author

@WestLangley in my opinion, this is much easier to read for a developer who doesn't know all of the Three.js internals (talking from personal experience here).

I also think this example should have been a showcase originally

Yeah, but also in the examples is much more easier to find for users. And also subsurface scattering is such a fundamental thing, that would feel weird not having an example of it. I personally came looking for it after having used it in Unity.

It relies on bunny_thickness.jpg

In my project I removed the sampler of that texture, but do you know of a way to make it optional? I don't know if there's a better way than a boolean uniform. I can make a PR if we decide on a way.

@WestLangley
Copy link
Collaborator

It relies on bunny_thickness.jpg

In my project I removed the sampler of that texture

You are saying this technique does not require a thickness map for the model?

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Apr 17, 2020

I just replaced

vec3 thickness = thicknessColor * texture2D(thicknessMap, uv).r;

with

vec3 thickness = thicknessColor;

since my model shape was kinda uniform (like a sphere).

This is like setting setting all of the thickness map to 1 and just playing with the color.

The results were acceptable for me, however I don't know if this is technically correct since they did not mention any of this in the presentation.

@WestLangley
Copy link
Collaborator

See the link at the top of the shader. In theory, a model-specific so-called thickness map is required, or must be computed prior to rendering.

But you are correct, it can be used without it. The rendering will just be missing detail.

@mrdoob
Copy link
Owner

mrdoob commented Apr 18, 2020

@Mugen87

A similar demo that made problems in the past is webgl_materials_envmaps_parallax.

The good news is that we now have automated pixel testing running in the examples so we'll be able to spot these things faster.

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.

None yet

5 participants