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

Material: Replace .format with .alphaWrite #23166

Merged
merged 3 commits into from
Jan 7, 2022

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Jan 7, 2022

Related issues:

Implements the solution proposed in #22598 (comment). This fixes the problem of .transparent=true having no effect for materials loaded with THREE.GLTFLoader, because .alphaWrite is automatically flipped to true ignored when .transparent=true is set. I hope it's also a clearer name than .format=RGB[A]Format, and more comparable to .colorWrite.

Unfortunately I'm no longer sure how to test all of the issues discussed above. The test model provided with #15483 renders correctly regardless of whether alphaWrite is on or off:

Screen Shot 2022-01-06 at 5 14 04 PM

@takahirox
Copy link
Collaborator

takahirox commented Jan 7, 2022

Thanks for making a PR.

This fixes the problem of .transparent=true having no effect for materials loaded with THREE.GLTFLoader, because .alphaWrite is automatically flipped to true when .transparent=true is set.

A concern of this is if .transparent is set back to false .alphaWrite won't be reset so the behavior can be different from the first one.

So I voted for the option B in #22598 (comment)

What do you think of it?

@Mugen87 Mugen87 added this to the r137 milestone Jan 7, 2022
@donmccurdy
Copy link
Collaborator Author

Sounds good to me, switched to (B) here. 👍

@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 7, 2022

Sidenote: The docs should also reflect this change (a separate PR for this is fine):

https://threejs.org/docs/#api/en/materials/Material.format

@mrdoob mrdoob merged commit be2400d into mrdoob:dev Jan 7, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 7, 2022

Thanks!

@donmccurdy donmccurdy deleted the feat-alphawrite branch January 7, 2022 20:12
@WestLangley
Copy link
Collaborator

WestLangley commented Jan 7, 2022

You should simply follow the glTF spec and have material.alphaMode (OPAQUE, BLEND, MASK) and material.alphaCutoff.

Remove material.alphaTest.

material.alphaWrite is misnamed. You can't "write alpha" into the destination if the destination has no alpha channel.

@mrdoob
Copy link
Owner

mrdoob commented Jan 7, 2022

That may be the best solution indeed...

Not a fan of how GLTF is "designing" the Three.js API itself, but I guess it's inevitable.

@donmccurdy
Copy link
Collaborator Author

Hm, I can only really see .alphaMode working if it replaces .transparent. There are some benefits – .alphaMode is very similar to Blender's "Blend Mode" (although I don't like that name as much...) and provides a good place to offer alpha hash or other modes someday. But it certainly wasn't my intention to push a big change on the material API for what is currently a pretty subtle issue.

Blender's Material → Settings → Blend Mode UI:

Screen Shot 2022-01-07 at 2 49 42 PM

@mrdoob
Copy link
Owner

mrdoob commented Jan 10, 2022

@WestLangley

You should simply follow the glTF spec and have material.alphaMode (OPAQUE, BLEND, MASK) and material.alphaCutoff.

That sounds good to me.

We would then have to turn transparent into a getter/setter that basically enables/checks alphaMode = THREE.BlendAlphaMode, right?

And then WebGLRenderer would use alphaMode to build the render lists.

@WestLangley
Copy link
Collaborator

WestLangley commented Jan 10, 2022

@mrdoob It is worth a try... but is it a bit complex... Here are a few random thoughts to get started:

  1. transparent in three.js has two purposes. (1) to set the render list, and (2) to set the blending mode.

  2. The current render lists are: background, opaque, transmissive, and transparent. Note, with "custom blending", the object could be in the opaque or transparent list depending on the value of transparent.

  3. From WebGLState.js, transparent = false is only honored if blending is Normal Blending.

  4. Setting material.transparent = false when material.blending = NormalBlending is the same as setting material.blending = THREE.NoBlending -- it just disables blending.

  5. With opaque mode, we need to overwrite source alpha with 1 so that destination alpha is set to 1.

  6. With mask mode, we need to discard fragments with alpha < alphaCufoff, and set source alpha to 1. (currently, we do not set source alpha to 1)

  7. I think all the logic can go in a new alphaMode shader chunk, to replace the alphaTest chunk.

  8. alphaMode should probably only apply in the case of Normal Blending.

@donmccurdy
Copy link
Collaborator Author

With mask mode, we need to discard fragments with alpha < alphaCufoff, and set source alpha to 1. (currently, we do not set source alpha to 1)

Would we also want a blend-and-mask mode? glTF doesn't offer that, but it's a reasonable combination I think.

alphaMode should probably only apply in the case of Normal Blending.

I think I would have expected alphaMode = BlendAlphaMode to be allowed with other blend modes. In Blender for example, "Blend Mode = Alpha Blend" appears to be required when using additive or multiply blending.

0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 27, 2022
See: mrdoob/three.js#23219

It will be replaced with .alphaWrite later

See: mrdoob/three.js#23166
joshuaellis pushed a commit to three-types/three-ts-types that referenced this pull request Jan 27, 2022
* change: Remove RGBFormat

See: mrdoob/three.js#23223
See: mrdoob/three.js#23228

* change: Remove .format from Material

See: mrdoob/three.js#23219

It will be replaced with .alphaWrite later

See: mrdoob/three.js#23166

* test: remove use of RGBFormat from a test case
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 27, 2022
See: mrdoob/three.js#23219

It will be replaced with .alphaWrite later

See: mrdoob/three.js#23166
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 27, 2022
Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 27, 2022
Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Jan 27, 2022
Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166
joshuaellis pushed a commit to three-types/three-ts-types that referenced this pull request Jan 27, 2022
Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166
joshuaellis added a commit to three-types/three-ts-types that referenced this pull request Jan 27, 2022
* feat: autodetect sRGB compression

* feat: remove inline sRGB decode

* chore: remove roughness mipmapper

* feat: added constant SRGB8

* feat: WebGLCubeUVMaps: Add support for render targets

* chore: Remove RGBFormat (#159)

* change: Remove RGBFormat

See: mrdoob/three.js#23223
See: mrdoob/three.js#23228

* change: Remove .format from Material

See: mrdoob/three.js#23219

It will be replaced with .alphaWrite later

See: mrdoob/three.js#23166

* test: remove use of RGBFormat from a test case

* feat: add LDrawUtils

* chore: make ConvexGeometry points optional

* chore: remove RGBIntegerFormat

* feat: Box3 now supports computing minimal bounds for setFromObject

* feat(Material): Add a new property .alphaWrite (#161)

Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166

* chore: remove UnsignedShort565Type

* chore: remove RoomEnvironment from OTHER_FILES

* chore: remove LDrawLoader from OTHER_FILES

* chore: fix linting

* chore: fix linting

Co-authored-by: 0b5vr <[email protected]>
@Mugen87 Mugen87 mentioned this pull request Jan 27, 2022
joshuaellis added a commit to three-types/three-ts-types that referenced this pull request Mar 1, 2022
* fix(WebXRManager): setAnimationLoop should accept null like WebGLRenderer (#158)

* fix: Missing Property in Raycaster (#160)

* Fix Missing Property in Raycaster

Adds uv2 to Intersection interface to more accurately reflect the original documentation.

See: (https://threejs.org/docs/#api/en/core/Raycaster.intersectObject)[https://threejs.org/docs/#api/en/core/Raycaster.intersectObject]
This has been submitted in response to Josh's request in (this PR)[DefinitelyTyped/DefinitelyTyped#58462]

* Add name to contributors

* r137 (#162)

* feat: autodetect sRGB compression

* feat: remove inline sRGB decode

* chore: remove roughness mipmapper

* feat: added constant SRGB8

* feat: WebGLCubeUVMaps: Add support for render targets

* chore: Remove RGBFormat (#159)

* change: Remove RGBFormat

See: mrdoob/three.js#23223
See: mrdoob/three.js#23228

* change: Remove .format from Material

See: mrdoob/three.js#23219

It will be replaced with .alphaWrite later

See: mrdoob/three.js#23166

* test: remove use of RGBFormat from a test case

* feat: add LDrawUtils

* chore: make ConvexGeometry points optional

* chore: remove RGBIntegerFormat

* feat: Box3 now supports computing minimal bounds for setFromObject

* feat(Material): Add a new property .alphaWrite (#161)

Since it's undocumented I could not fill the doc comment properly,,,

See: mrdoob/three.js#23166

* chore: remove UnsignedShort565Type

* chore: remove RoomEnvironment from OTHER_FILES

* chore: remove LDrawLoader from OTHER_FILES

* chore: fix linting

* chore: fix linting

Co-authored-by: 0b5vr <[email protected]>

* feat: add PackedPhongMaterial

* fix: Scene Utils are not in namespace

resolves #153

* r137

* change (Material): remove `alphaWrite`

See: mrdoob/three.js#23361

Co-authored-by: Cody Bennett <[email protected]>
Co-authored-by: Joe Tipping <[email protected]>
Co-authored-by: Josh <[email protected]>
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.

Material: transparent=true has no effect on glTF materials (since r132)
5 participants