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

GLTFLoader: Use depthWrite=false for transparent materials #18235

Merged
merged 2 commits into from
Feb 22, 2020

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Dec 23, 2019

NOTE: Let's wait until after the r112 release to merge this.

Based in part on #17706 and donmccurdy/three-gltf-viewer#169, and more importantly by what other engines appear to be doing, I've come to the conclusion that we should set depthWrite=false for transparent materials in GLTFLoader.

References

Examples

For an illustration of where this makes something worse, here's the Glennfiddich Bottle from below. You see the stem of the bottle through the base of the bottle:

depthWrite=true depthWrite=false
scotch_depthWrite=true scotch_depthWrite=false

Exceptions

The "Glennfiddich Bottle" and "Curtains" examples above are (IMHO) not modeled appropriately for realtime rendering. The latter could be fixed without too much trouble, but I'm not sure how I'd fix the former. Enabled or disabled, depthWrite doesn't fix either.

The resources below describe a few situations where developers really may want to use depthWrite=true on transparent materials. Compared to the precedent set by other engines above, these exceptions seem minor enough to justify changing the default:

@donmccurdy donmccurdy changed the title GLTFLoader: Use depthWrite=false for transparent materials [WIP] GLTFLoader: Use depthWrite=false for transparent materials Dec 23, 2019
@donmccurdy
Copy link
Collaborator Author

FYI @cdata @elalish

@RemusMar
Copy link
Contributor

depthWrite (true or false) should be used per models (mesh or material).
Not per loader!

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Dec 23, 2019

@RemusMar if you're OK with my claim that depthWrite=false should be preferred for transparent materials, what change would you suggest?

a. Within the current API — with .transparent and .depthWrite as independent booleans — we cannot change the default value of .depthWrite for a particular value of .transparent on new materials. We could change that API, I don't know what the new API should be.

b. We could document the workarounds and limitations of transparent rendering somewhere, and include the recommendation that "transparent materials should generally set depthWrite=false" as best practice.

c. We could apply that best practice to new transparent materials created by three.js on behalf of users, e.g. by loaders.

I'd suggest a combination of (b) and (c) right now. I'm not opposed to making this same change on all loaders, and perhaps elsewhere. But I'd rather discuss the PR first, before editing dozens of other loaders.

@RemusMar
Copy link
Contributor

Don,

I have always disliked forced settings via loaders.
(I've removed even those sRGBEncoding forced settings from my GLTF loaders).
Talking about the (semi)transparent objects and 3D engines, as you already pointed out, there is no perfect solution.
Probably the best one is "per polygon (triangle) sorting", but it requires a lot of processing power.
I wouldn't change the default depthWrite=true setting (for both opaque and transparent materials).
Simply because depthWrite=false is NOT the best solution in all the cases with semi-transparent materials.
The best solution here is to let the user to change that setting for a specific object (material) based on his particular object-world-camera case.

cheers

@RemusMar
Copy link
Contributor

To give you a better picture, check out this example:
http://necromanthus.com/Test/html5/SMC_gltf.html
Use mouse wheel to get closer to one of those palm trees.
If you use depthWrite=false here, the result is completely wrong.
The best solution in this case is a "trick":

	palm.material.side = THREE.DoubleSide;
	palm.material.alphaTest = 0.5;
	palm.material.transparent = false;

@donmccurdy
Copy link
Collaborator Author

In glTF, at least, there is a separate alpha mode ("MASK") for the alphaTest trick you mention above, so 3D artists can explicitly choose that mode (Alpha Clip, in Blender) and my depthTest change in this PR is disabled in that mode. For the partial transparency alpha mode ("BLEND"), depthWrite=false is the best solution in most cases. Not all, but most.

Defaults are a way of giving users settings that work well in most cases. Users will continue to be able to change this setting, but fewer users will need to do so with good defaults. I'm trying to reduce surprises and inconsistency when bringing models in from outside software. That is the responsibility of a loader — to bring a model into three.js, matching the expectations of the author as closely as possible.

I'm not opposed to changing the API rather than the loaders. But I've thought about it a bit, and I'm unable to come up with a better API. Mostly unrelated to this PR, but I am interested in introducing an alpha hash or alpha dither transparency mode to three.js, which would not have this depthWrite limitation (but has some other quirks).

I understand your dislike for having different defaults in loaders than those of the underlying three.js library. However, I do believe that if we're going to advise users to set depthWrite=false when they create transparent materials (and we should), then the loaders should do that as well.

@Kagetsuki
Copy link

Kagetsuki commented Dec 24, 2019

I don't mean to impose with my opinion, but in the specific case of glTF there seems to be a general interpretation of the standard regarding transparent materials. Software like Blender since ~2.81 is exporting very close to that interpretation (thanks to the direct contribution from Khronos Group developers), and most software me and my team have tested seems to handle transparency on imported models the same way Blender is exporting them. When we, as developers, use a library or viewer, we generally want the default settings to be such that they import our models as closely as possible to the way we exported them without needing extra adjustments on a per-model basis. Currently however, three.js requires specific adjustments to correctly load models that we see as closely adhering to the generally accepted interpretation of the standard. While this patch may not be the ideal implementation it would seem to provide default with a closer representation of what we are exporting without requiring additional code.

Just to reiterate, I'm specifically talking about importing glTF files, NOT about three.js defaults.

@RemusMar
Copy link
Contributor

I'm specifically talking about importing glTF files, NOT about three.js defaults.

The best approach is to modify your GLTF importer to fit your needs.
( same as I did with the forced sRGBEncoding).

@RemusMar
Copy link
Contributor

@donmccurdy

Don't get me wrong!
If you have a group of Blender users who asked for depthWrite=false for materials with transparency in the GLTF importer
and you think it's a good idea, feel free to modify the loader (presuming Ricardo agrees with that as well).
It's Christmas after all ...
🎅

@donmccurdy donmccurdy changed the title [WIP] GLTFLoader: Use depthWrite=false for transparent materials GLTFLoader: Use depthWrite=false for transparent materials Dec 26, 2019
@mrdoob
Copy link
Owner

mrdoob commented Dec 30, 2019

@donmccurdy what do you think about making this change in the engine directly?

@donmccurdy
Copy link
Collaborator Author

@mrdoob I think it would be a good default behavior for the engine, but I'm not sure how the API should be changed to allow that (while keeping the ability to change that default). Comparing the engines mentioned above:

  • Unity's API is most similar to ours, with an independent ZWrite setting. The ZWrite default is always true, but the documentation does recommend changing it from the default for transparent materials.
  • Unreal's transparent materials never write to the depth buffer, but the user can create a custom depth buffer and opt-in their transparent materials to use that.
  • BabylonJS has a lot of options here: needDepthPrePass, forceDepthWrite, and disableDepthWrite, with an additional separateCullingPass option to separate front- and back-face passes. I'm not sure how all of those interact, but they default to false, and the (extensive) transparency documentation mentions that needDepthPrepass can help with self-transparency issues, similar to this discussion.

The most straight-forward change would be to introduce a separate property, such as .transparentDepthWrite (default false), and ignore the .depthWrite property on materials with .transparent = true. I am undecided about that API...

If it matters, I'm also mulling over the idea of proposing an .alphaMode-like property, to switch into alpha dither- or hash-based transparency as alternatives to alpha blending. Maybe that property would also be an option for this change:

enum AlphaMode {
  ALPHA_BLEND,
  ALPHA_BLEND_DEPTH_WRITE,
  ALPHA_HASH,
  ALPHA_DITHER
}

... but that's not great either, because really depth writes could be enabled or disabled regardless of blend mode. :/

@donmccurdy
Copy link
Collaborator Author

Maybe 90% of the answer here is more documentation about transparency 😅

@RemusMar
Copy link
Contributor

The ZWrite default is always true, but the documentation does recommend changing it from the default for transparent materials

Again, the default value is TRUE and they don't force FALSE for transparent materials (because it's not always a winner).
And that's the way it should be.
cheers

@Kagetsuki
Copy link

Again, please excuse my opinion (as someone who isn't an active contributor to the project) here.

The way it should be is whatever is most adherent to the standard. With glTF specifically this hasn't been completely clearly defined until recently - and with that increasing clarity in the definition it seems most projects are moving toward a single interpretation (and I'm thinking @donmccurdy would agree with this assessment?).

As for changing engine defaults... I don't know if that's a good idea if it ends up breaking a lot of existing projects which use three.js. three.js isn't built to be just a model viewer after all - as I see it importing models seems to be more secondary to it primarily being a web-based visualization engine. So if the changes can be isolated to the glTF importer that would likely be ideal. Or, if improvements to the engine are spurred by this then that would be fantastic. But I think the position @RemusMar is trying to convey is that if the engine is changed in some significant way just to import glTF then that's a bad thing - and I would generally agree with that. On the other hand, I completely expect wider glTF adoption in the near future and if three.js is the only engine that can't cleanly import the majority of models with defaults then that would be detrimental to adoption and would likely come with more "this works on X but not three.js" complaint spam [like mine 😅] on the issue tracker.

@1147079942
Copy link

After use the depthWrite=false I can see the glass cup through the window. But still has some problem.The glass bottle should before the cup, but some of them look not like it should be.
image

@donmccurdy
Copy link
Collaborator Author

After use the depthWrite=false I can see the glass cup through the window. But still has some problem.

@1147079942 this PR doesn't fix all possible issues with transparency, and no perfect solution in WebGL exists. If you'd like to open a thread on the forums (discourse.threejs.org/) and share enough to reproduce the issue (code, model, demo) I may be able to suggest workarounds for your particular scene.

Again, the default value is TRUE and they don't force FALSE for transparent materials (because it's not always a winner). And that's the way it should be.

@RemusMar nowhere in this thread has anyone suggested forcing transparent materials to use depthWrite=false. It is, and will remain, configurable. I am suggesting that we change a default so that it's right most of the time, not wrong most of the time. In either case, some users will need to change it.


If transparent materials could default depthWrite=false library-wide, it would be a net-positive for most users. Unfortunately I don't see a good way of changing the three.js API to support that, and presumably neither did Unity, and that's fine.

I do feel that if GLTFLoader (or other loaders, for that matter) are going to create transparent materials on behalf of users, they should use best practices for those materials, and depthWrite=false is usually the winner.

@donmccurdy
Copy link
Collaborator Author

Another example, here's a generic "transparent liquid in a transparent bottle" situation:

depthWrite = true depthWrite = false
bottle with artifacts and no visible contents bottle with fewer artifacts and a golden liquid inside

@RemusMar
Copy link
Contributor

they should use best practices for those materials, and depthWrite=false is usually the winner.

Don,

This topic is not about best practices.
In fact, depthWrite=false is GAMBLING and that leads to 50%/50%
It might give better results for the Blender users in some cases, but that means their models/worlds are NOT properly designed.
Z-sorting is a MUST for a 3D engine if you want the best results.
cheers

@Kagetsuki
Copy link

@RemusMar It is not just Blender, and in the specific case of glTF there has been a lot of discussion about how to handle transparency which has led to a fairly clearly defined implementation of the standard which most (and when I say most I mean every engine other than three.js that my team has tested) are implementing it something like what Don is presenting.

This topic is not about best practices.

I'm fairly certain it's entirely about best practices specific to handling glTF.

To quote Don, with emphasis:

I do feel that if GLTFLoader (or other loaders, for that matter) are going to create transparent materials on behalf of users, they should use best practices for those materials, and depthWrite=false is usually the winner.

In the case of glTF, as it stands now/as I understand it/as the standard is being generally interpreted and implemented, depthWrite=false is going to have a much higher chance of being correct than otherwise.

@RemusMar
Copy link
Contributor

For the last time: depthWrite=false is GAMBLING and not a solution.
Transparency is very sensitive to models/camera position and rotation and for this reason all the posted screenshots are not very relevant.
If you need depthWrite=false to get better results iy means that your model is not properly designed.
To close this subject:
http://necromanthus.com/Test/html5/head.html
Click on the stage to change the depthWrite value.
Move the mouse to rotate the model.
You'll see why GAMBLING is not a solution.
cheers

@Kagetsuki
Copy link

Kagetsuki commented Jan 11, 2020

I hate to tell you this, but in your example depthWrite=true is in fact more broken than false. Note how the alpha on the tuft of hair at the base of the head, in front of the ponytail, isn't correct.
image

With it set to false:
image

Either way this is gambling. As it is now, it's going to take a lot more work (and probably a more defined glTF standard and more robust reference implementations) to get it completely correct - but as it is now this patch and defaulting to using depthWrite=false for glTF interpretation is going to get it right more often than not.

@RemusMar
Copy link
Contributor

Obviously, you see the only rendering error for TRUE, but you don't (want to) see the complete mess for FALSE.
depthWrite_false

I have to go now.
cheers

@Kagetsuki
Copy link

@RemusMar Sorry, false absolutely looks better to me. Both are broken - one is just less broken - and the one that looks less broken to me is depthWrite=false.

Then again, perhaps "your model is not properly designed"...

@RemusMar
Copy link
Contributor

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Jan 11, 2020

This topic is not about best practices. In fact, depthWrite=false is GAMBLING and that leads to 50%/50%

@RemusMar I've collected defaults and documentation from three other engines, and examples from five user-reported issues, in this thread. I find these to be compelling evidence that the proposed change is both best practice, and improves considerably more than 50% of cases.

Transparency on what should be an opaque object (the hairtie) is not helping here:

72207274-a0257b00-348f-11ea-8feb-45287e37c7ab

Not to say you or the artist are incorrect for doing this — choose a workflow that works for your application — but if you wanted this model to be portable and to look right in multiple engines, you would need alphaTest=0.5, transparent=false on this model (which could both be represented in glTF, the user would not need to tweak this). I am trying to make our default semitransparency more portable and aligned with other engines and authoring tools, too.

If you import to BabylonJS or Blender, you'll again see depthWrite=false chosen.

babylon blender
Screen Shot 2020-01-11 at 10 18 51 AM Screen Shot 2020-01-11 at 10 21 13 AM

To put it another way, if you put a transparent object inside another transparent object you would expect to see both objects. This is more physically intuitive. There do exist cases where you wouldn't want that, and might choose .side = FrontFace or depthWrite = true to avoid it, but they're subjective choices for situations that are (IMO) more stylized or customized for an application than physically-based.

@donmccurdy
Copy link
Collaborator Author

@RemusMar If you really think it's a 50/50 gamble, despite multiple counterexamples, then you can at least rest easy that 50/50 means a net-neutral effect here. This conversation is not productive, and I'm not interested in spending time persuading you, personally, to agree with me or to use settings you do not want to use. Choose the settings you want for your own use.

You've expressed your disapproval of the PR. I acknowledge that but disagree, and I don't think I have any more to say about the topic.

@WestLangley
Copy link
Collaborator

Yes, the changes would be extensive, but maybe the basic API could be something like this:

Define material.depthWrite as null by default.

Then, in the renderer,

depthBuffer.setMask( ( material.depthWrite === null ) ? ! material.isTransparent : material.depthWrite );

@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2020

Yes, the changes would be extensive, but maybe the basic API could be something like this:

Define material.depthWrite as null by default.

Then, in the renderer,

depthBuffer.setMask( ( material.depthWrite === null ) ? ! material.isTransparent : material.depthWrite );

I was considering this too. We may want to try that at some point...

But I think for now it's better to make sure we render GLTF as they're supposed to be rendered.

@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2020

@donmccurdy Sorry for the delay dealing with this one. Do you mind resolving the conflicts?

@mrdoob mrdoob added this to the r114 milestone Feb 13, 2020
@donmccurdy donmccurdy force-pushed the feat-gltfloader-transparent-depthwrite branch from 0c56a58 to 21498d1 Compare February 22, 2020 06:13
@donmccurdy
Copy link
Collaborator Author

@mrdoob Done. ✅

@mrdoob mrdoob merged commit 421e610 into mrdoob:dev Feb 22, 2020
@mrdoob
Copy link
Owner

mrdoob commented Feb 22, 2020

Thanks!

@donmccurdy donmccurdy deleted the feat-gltfloader-transparent-depthwrite branch February 22, 2020 06:34
@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 23, 2020

Um, it seems this change has broken webgl_lightshafts. It was necessary to fix it manually, see #18708 (comment).

@donmccurdy How would you highlight this change in the migration guide? Is there something we can recommend users if they encounter similar issues like with the tree model from webgl_lightshafts?

@donmccurdy
Copy link
Collaborator Author

As you found since the model didn't render right initially, and already needed these overrides...

child.material.transparent = false;
child.material.alphaTest = 0.5;

...this model needed to be using an Alpha Clip (.alphaTest>0) alpha mode, rather than Alpha Blending (.transparent=true). Importing to Blender, it looked broken in the same way. Seems to be an occasional issue in Sketchfab exports.

The best fix is the same now as it was before; to use a masked alpha mode:

tree_fixed.glb.zip

We should highlight that this setting has changed in the migration guide for sure, but as far as specific recommendations I think we could probably benefit from having a whole page in the documentation about transparency...

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 23, 2020

There's a further issue on the base of the model (not really related to this change) —

engine screenshot
sketchfab Screen Shot 2020-02-23 at 9 53 15 AM
threejs Screen Shot 2020-02-23 at 9 53 21 AM

No available settings will fix that fully, it doesn't look right with blended or masked alpha. I think that's probably a situation for alpha dither/hash?

@samyH
Copy link

samyH commented Feb 24, 2020

@donmccurdy thanks for you continuing support on all things WebXR - probably never going to be a one size fits all for these things - as long as there is a doc somewhere via a google search on what to try in each case then that's cool - it's called development for a reason right :P

@makc
Copy link
Contributor

makc commented Apr 15, 2020

Late to the party, but I have something to say about this as well - you now need to remove this part from the documentation:
Screen Shot 2020-04-15 at 17 14 23

or maybe replace it with

There default value is random

because clearly we cant expect any specific value now.

@Mugen87
Copy link
Collaborator

Mugen87 commented Apr 15, 2020

The default values are for the case if you just create a material by yourself without parameters.

Loaders configure many material properties, not just depthWrite. Against this backdrop, I don't think your suggestion makes sense.

@donmccurdy
Copy link
Collaborator Author

Agreed with @Mugen87. However, a note suggesting to users that depthWrite should often be disabled when transparency is enabled, and vice versa, would probably be good.

@vadermemo
Copy link

This is worst when you need put an animator with opacity on materials as glasses.
The combination opacity/transparency doesn´t work with depthWrite in all cases.

Any suggestions?

@donmccurdy
Copy link
Collaborator Author

@vadermemo please use the forum (https://discourse.threejs.org/) for help. You'll need to share additional details about your question as well, I think.

@vadermemo
Copy link

@vadermemo please use the forum (https://discourse.threejs.org/) for help. You'll need to share additional details about your question as well, I think.

Hi @donmccurdy, thanks for the tip. Here is the big question => https://discourse.threejs.org/t/opacity-transparent-animation-big-problem/20149

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.

10 participants