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.

Sign up for GitHub

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

Jump to bottom

ShaderChunk: Turn alphaTest into a uniform #22110

Closed
wants to merge 9 commits into from
Closed

ShaderChunk: Turn alphaTest into a uniform #22110

wants to merge 9 commits into from

Conversation

mrdoob
Copy link
Owner

@mrdoob mrdoob commented Jul 10, 2021 •
edited

Related issues: #19803 (comment) #15654 #5623

Description

I think this has been proposed a few times. Should make programs more reusable.

@mrdoob mrdoob added this to the r131 milestone Jul 10, 2021
@WestLangley
Copy link
Collaborator

WestLangley commented Jul 10, 2021 •
edited

I think I would prefer what @WestLangley said: #15654 (comment).

Perhaps we can change the default value of Material.alphaTest from 0 to null, and then define USE_ALPHATEST if Material.alphaTest !== null.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 10, 2021

Oh! I missed that comment.

Mugen87
Mugen87 reviewed Jul 11, 2021
View reviewed changes
src/materials/Material.js Outdated
@@ -62,7 +62,7 @@ class Material extends EventDispatcher {

this.dithering = false;

this.alphaTest = 0;
this.alphaTest = null;
Copy link
Collaborator

@Mugen87 Mugen87 Jul 11, 2021 •
edited

Choose a reason for hiding this comment

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

When I remember correctly having a nullable material property seemed problematic when sheen was implemented. There is also a PR that tries to remove this behavior, see https://github.com/mrdoob/three.js/pull/17700/files#diff-98984bfa7fa23284a6a8bce191397059df3700f4377553fa7312dd69a78d780fL38.

The policy for sheen right now is that null disables its influence in the shader. With this change, this would also be true for alphaTest. I'm just mentioning this since #17700 should be closed if this PR is going to be merged.

Copy link
Collaborator

@Mugen87 Mugen87 Jul 11, 2021 •
edited

Choose a reason for hiding this comment

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

Instead of using null, we could keep the status quo and set USE_ALPHATEST only when alphaTest is greater zero. I guess I would prefer this approach. Introducing a uniform still makes sense since the engine produces less shader compilations when alphaTest is animated (although I'm not sure how common this use case is).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, I've tried to make null work but having this double state is a nightmare, speciall in the editor...
Reverting to 0 but adding a > 0 check.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Instead of using null, we could keep the status quo and set USE_ALPHATEST only when alphaTest is greater zero. I guess I would prefer this approach. Introducing a uniform still makes sense since the engine produces less shader compilations when alphaTest is animated (although I'm not sure how common this use case is).

Ha! I missed that comment. Glad that we came to the same conclusion 😀

Copy link
Owner Author

Choose a reason for hiding this comment

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

The policy for sheen right now is that null disables its influence in the shader. With this change, this would also be true for alphaTest. I'm just mentioning this since #17700 should be closed if this PR is going to be merged.

After working on this PR I think #17700 is the way to go indeed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'll finish that PR after we're done with this one 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 11, 2021

Um, it seems this change breaks MeshDistanceMaterial.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 11, 2021

Setting alphaTest to null by default had quite a few side-effect and it over complicated the code.

We can achieve the same optimization by adding USE_ALPHATEST when alphaTest > 0.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 11, 2021

Um, it seems this change breaks MeshDistanceMaterial.

Do you mind double checking? There was a typo when you tested it 🤞

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 11, 2021 •
edited

It seems examples like webgl_morphtargets_sphere and webgl_points_billboards are now broken. Both use PointsMaterial.

image

There is also another things that bothers me: If you have a material with alphaTest set to 0, you have to set needsUpdate to true once if you are going to change the value to > 0. For all further changes of alphaTest, it's not necessary to set needsUpdate anymore. This could be quickly considered as a bug since the behavior is indeed incosistent.

@mrdoob
Copy link
Owner Author

mrdoob commented Jul 11, 2021 •
edited

It seems examples like webgl_morphtargets_sphere and webgl_points_billboards are now broken. Both use PointsMaterial.

Fixed!

There is also another things that bothers me: If you have a material with alphaTest set to 0, you have to set needsUpdate to true once if you are going to change the value to > 0. For all further changes of alphaTest, it's not necessary to set needsUpdate anymore. This could be quickly considered as a bug since the behavior is indeed incosistent.

Hmm, yeah... I wonder if avoiding that if is worth having to be aware of this... 🤔

Mugen87
Mugen87 reviewed Jul 11, 2021
View reviewed changes
src/renderers/shaders/UniformsLib.js
@@ -18,7 +18,7 @@ const UniformsLib = {
uv2Transform: { value: new Matrix3() },

alphaMap: { value: null },
alphaTest: { value: 0 }
alphaTest: { value: null }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Um, shouldn't this be 0 (similar to opacity)?

@donmccurdy
Copy link
Collaborator

For cases like NodeMaterial, where alphaTest is animated, reducing alphaTest from >0 to =0 is not going to trigger a recompile on its own, correct? If so this sounds OK and useful.

My suggestion in #15654 (comment) had been something like a material.alphaMode = OPAQUE | BLEND | CLIP | BLEND & CLIP enum, possibly to replace.transparent=true's effect on blending, but that's a bigger change and not incompatible with this change anyway.

@WestLangley
Copy link
Collaborator

@mrdoob Why not #22110 (comment)?

@mrdoob mrdoob modified the milestones: r131, r132 Jul 29, 2021
@Mugen87 Mugen87 mentioned this pull request Aug 20, 2021
Merged
@mrdoob mrdoob removed this from the r132 milestone Aug 24, 2021
@mrdoob
Copy link
Owner Author

mrdoob commented Aug 24, 2021 •
edited

Second try: #22409

@mrdoob mrdoob closed this Aug 24, 2021
@mrdoob mrdoob deleted the alphatest branch August 24, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mugen87 Mugen87 Mugen87 left review comments

@donmccurdy donmccurdy Awaiting requested review from donmccurdy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mrdoob @WestLangley @Mugen87 @donmccurdy