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

SSRNode: Honor metalness. #29605

Merged
merged 1 commit into from
Oct 9, 2024
Merged

SSRNode: Honor metalness. #29605

merged 1 commit into from
Oct 9, 2024

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 9, 2024

Related issue: #29597, #21487

Description

This makes sure metalness is not just an on/off switch for SSR but actually influences the intensity. The lighting of the example is a bit updated so it better fits to PBR materials.

We might want to honor even more parameters (see #28752) but for now it's a good start.

@Mugen87 Mugen87 added this to the r170 milestone Oct 9, 2024
@Mugen87 Mugen87 merged commit 5e3e26a into mrdoob:dev Oct 9, 2024
11 checks passed
@Methuselah96 Methuselah96 mentioned this pull request Nov 4, 2024
68 tasks
@ligaofeng0901
Copy link
Contributor

Why is only the metalness factor taken into account? Does it mean there is no SSR effect on the fragment that metalness is zero? In ssr( colorNode, depthNode, normalNode, metalnessNode, camera ) , metalnessNode must be a texture node, what about make it be any type of node? so we can pass the param like max(metalness, roughness.oneMinus()).

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 8, 2024

As mentioned in #28752, honoring more material properties is wanted but I guess there are some uncertainties in doing it in a PBR conform way. I suggest we continue the discussion in #28752 and try to find a solution to include roughness in the SSR code.

Ideally, we have some sort of reference/documented resource that we implement (I have not checked so far if the formula proposed in #28752 (comment) is correct).

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