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

Improve MeshPhysicalMaterial transmission support Part2 #21975

Merged
merged 4 commits into from
Jun 15, 2021

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Jun 11, 2021

Related issue: #21884 (comment)

Description

This PR improves MeshPhysicalMaterial transmission support.

image

@WestLangley Looking good now?

Changes

  1. In the previous PR I first applied transmission to diffuseColor

https://github.com/mrdoob/three.js/blob/r129/src/renderers/shaders/ShaderChunk/transmission_fragment.glsl.js#L30

and then applied lighting to it. This is the main reason why the color was darken even if transmission is 1. In this PR I apply the transmission after the lighting.

  1. In the previous PR, I calculated specular in the transmission function. But specular is calculated in the lighting. I speculate I can reuse it.

https://github.com/mrdoob/three.js/blob/r129/src/renderers/shaders/ShaderChunk/transmission_pars_fragment.glsl.js#L80-L82

  1. I turn off tone mapping for the first opaque objects pass to avoid to apply it again in the second transmissive objects pass.

Screenshots

I still have an issue that multi sample render target is black in the screenshots. So I made the screenshots by temporarily changing the code to non-multi-sample render target. Please feel free to update the screenshots if you can make correctly.

@mrdoob
Copy link
Owner

mrdoob commented Jun 11, 2021

I still have an issue that multi sample render target is black in the screenshots. So I made the screenshots by temporarily changing the code to non-multi-sample render target. Please feel free to update the screenshots if you can make correctly.

Have you tried running npm update inside the test folder?

@WestLangley
Copy link
Collaborator

Now that it is looking better, we need to figure out how to properly deal with premultipliedAlpha -- when to use it, and when not to.

For a later PR, of course.

@WestLangley
Copy link
Collaborator

Also, _transmissionRenderTarget should not be UnsignedByteType, because that leads to clamping and texture hue shifts.

Perhaps UnsignedByteType can be a fall-back if HalfFloatType is not supported.

UnsignedByteType
Screen Shot 2021-06-11 at 7 49 42 AM

HalfFloatType is mostly better :-)
Screen Shot 2021-06-11 at 7 49 59 AM

@takahirox
Copy link
Collaborator Author

@mrdoob

Have you tried running npm update inside the test folder?

Ah, I didn't notice that I needed to run it in the test directory. Updated the screenshots, thanks!

@WestLangley

Also, _transmissionRenderTarget should not be UnsignedByteType, because that leads to clamping and texture hue shifts.

Perhaps UnsignedByteType can be a fall-back if HalfFloatType is not supported.

Good catch. Updated, thanks!

@mrdoob mrdoob added this to the r130 milestone Jun 15, 2021
@mrdoob mrdoob merged commit 6d6a324 into mrdoob:dev Jun 15, 2021
@mrdoob
Copy link
Owner

mrdoob commented Jun 15, 2021

Excellent! Thanks!

Comment on lines +1292 to +1293
const currentToneMapping = _this.toneMapping;
_this.toneMapping = NoToneMapping;
Copy link
Owner

Choose a reason for hiding this comment

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

Oh! Actually... I guess we should also set outputEncoding to LinearEncoding?

Copy link
Collaborator Author

@takahirox takahirox Jun 15, 2021

Choose a reason for hiding this comment

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

No because we use renderTarget texture's encoding as output encoding if currentRenderTarget isn't null. (Texture's default encoding is linear.)

https://github.com/mrdoob/three.js/blob/r129/src/renderers/webgl/WebGLPrograms.js#L174

If we will start to copy the framebuffer we will need to convert from sRGB to linear and from tone mapping to none in the shader if they are set.

@takahirox takahirox deleted the ImproveTransmission branch June 15, 2021 19:38
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