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

Examples: Depth peeling example #24227

Closed
wants to merge 7 commits into from
Closed

Examples: Depth peeling example #24227

wants to merge 7 commits into from

Conversation

ingun37
Copy link
Contributor

@ingun37 ingun37 commented Jun 12, 2022

Description

A very basic depth peeling implementation based on Order-Independent Transparency - NVIDIA

Control depth between 1~5
depth-change

Enable/Disable
enable

Inspired by #15312.

If possible, I would love to add DepthPeeling class into examples/jsm/.

@ingun37 ingun37 changed the title Depth peeling example Examples: Depth peeling example Jun 12, 2022
@ingun37 ingun37 requested a review from Mugen87 June 14, 2022 10:37

const clonedMaterial = obj.material.clone();
clonedMaterial.blending = THREE.NoBlending;
clonedMaterial.onBeforeCompile = ( shader ) => {
Copy link
Collaborator

@Mugen87 Mugen87 Jun 14, 2022

Choose a reason for hiding this comment

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

In the past, we've come to the conclusion to not add this kind of shader patching to the examples anymore since it easily breaks when built-in materials are changed.

#15312 also did that and it always felt like a hack. But I assume there is no other way to support depth peeling right? Meaning the technique requires a change in the shader code, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can match void main() { and prepend the peeling codes at the beginning of the main instead of appending to the end. It should be safer than just matching }$.

Copy link
Contributor Author

@ingun37 ingun37 Jun 14, 2022

Choose a reason for hiding this comment

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

Or I can make snapshot of current(r141) MeshStandardMaterial fragment shader and embed it using ShaderMaterial? This way it won't break by any built-in material update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Um, I'm not yet sure what's best in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think void main() { match should be practically safe since those four tokens will always exist consecutively according to GLSL specification.
But I understand it's a "hack" nevertheless and your reluctance to this approach.

Copy link
Contributor Author

@ingun37 ingun37 Jun 18, 2022

Choose a reason for hiding this comment

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

Depth Peeling is simple technique yet its not easily accessible due to lack of examples. So I think it will have benefits at least to some extent for users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what @Mugen87 is suggesting is to add the depth peeling to the renderer itself rather than to create an example or class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeviPesin Yes, I understood that part. But he asked @mrdoob second opinion and I pryingly added mine 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All in all, please close the PR if you guys think it's against the THREE.JS's policy. I'm happy either way 👍

This pull request was closed.
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