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: Could we re-think about how we deal depth buffer for BLEND materials? #17706

Closed
4 of 13 tasks
0b5vr opened this issue Oct 10, 2019 · 17 comments
Closed
4 of 13 tasks

Comments

@0b5vr
Copy link
Collaborator

0b5vr commented Oct 10, 2019

Description of the problem

Already discussed a bit about it on #13889

Currently, we are letting depthWrite: true for every material that is being loaded from GLTFLoader, even when the material is alphaTest: BLEND .
And glTF spec does not specify how we must set our depth write, even in non-normative sections, since the use of glTF is not restricted to the traditional realtime rasterizer.
It just says that we should try to make it look proper in as many situations as possible.

See: https://github.com/KhronosGroup/glTF/tree/master/specification/2.0/#alpha-coverage

I think we should follow other "majority" implementations for this situation. (uhhh is this good idea?)
Note that it's pretty easy to change the behavior, just adding a line to GLTFLoader would work I think.

Three.js version
  • Dev
  • r109
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@donmccurdy
Copy link
Collaborator

Currently, we are letting depthWrite: true for every material that is being loaded from GLTFLoader.

I think we came to the conclusion that #13889 was a modeling issue. Can you give other examples of models that require depthWrite: true? My impression is that without it, we handle transparency correctly roughly as often as babylonjs, although either may succeed and fail in different situations.

And glTF spec does not specify how we must set our depth write, even in non-normative sections, since the use of glTF is not restricted to the traditional realtime rasterizer.

glTF files are meant to be renderable by realtime rasterizers, but even with that, I'm not sure whether it's true that a majority of implementations set depthWrite: false for models with the "BLEND" alpha mode.


I'm OK with considering a change here, but (1) I don't know whether depthWrite: false is the right change, as opposed to (for example) a different sorting algorithm, and (2) if there is a change I don't think it should be in GLTFLoader alone. The glTF format does not specify depthWrite for alpha transparency because we don't think there's a single correct/consensus answer. GLTFLoader uses three.js defaults to be consistent with user expectations, so if we change anything here, it should probably be the three.js defaults – there's nothing particularly different about alpha transparency in glTF vs FBX, the same limitations apply.

@0b5vr
Copy link
Collaborator Author

0b5vr commented Oct 10, 2019

Yes that is pretty expected and reasonable answer, thank you @donmccurdy -san!
I think I need more examples of actual model and implementations.

Our current implementation that let end developers to decide whether or not use depth write for any materials (using .depthWrite) is more flexible, but make it consistent over the entire Three.js as BabylonJS does also sounds good, I can't judge about it, we need more opinion imo.

@Kagetsuki
Copy link

We've essentially run into this issue and I think we may have a very specific example. We have a semi transparent material under a mostly transparent material and it displays fairly well in babylon, but in three.js we have two scenarios:

  1. Default of depthWrite: true does not calculate the transparency of the material behind the other transparent material and is either culling it or giving it a very high alpha or something (left is three, right is babylon):
    defaults comparison
  2. Setting depthWrite to false seems to completely ruin materials:
    depthWrite disabled

Here's the glb of the model we're testing (we can provide the original Blend file if anyone wants it).
HumanEye-V2(concept).zip

For us it would be preferable if the display was like babylon by default - as the babylon version is very close to what we're looking for and it seems to have a high[er] compatibility with the Blender (2.81) glTF exporter. For others this may of course not be the case, but I feel like with so much effort being put into the Blender exporter by members of the Khronos Group(♥) it may be a better reference.?

If there's anything we can do to help with this issue please let me know.

@donmccurdy
Copy link
Collaborator

@Kagetsuki the problem visible in your third image comes from setting depthWrite=false on all materials. If you set the option only on the transparent materials, you'll get a reasonable result.

Screen Shot 2019-12-22 at 8 53 05 PM

@fms-cat Okay, I think I agree at this point. Let's set depthWrite=false for transparent materials in GLTFLoader. Opened #18235.

@Kagetsuki
Copy link

@donmccurdy That is correct. Thanks for the clarification and thanks for your continued work!

@donmccurdy
Copy link
Collaborator

Fixed: #18235.

@donmccurdy
Copy link
Collaborator

Thanks for the examples and discussion, @fms-cat and @Kagetsuki!

@Kagetsuki
Copy link

Thank you @donmccurdy .

@0b5vr
Copy link
Collaborator Author

0b5vr commented Mar 2, 2020

Ohhhh I didn't notice to this! Thank you ❤️

@greggman
Copy link
Contributor

greggman commented May 18, 2020

FTW, this change broke stuff. So while it may have worked for the example above it is not clear it's the correct thing to do?

I didn't search that hard but some scenes that broke (testing here)

https://sketchfab.com/models/a1d315908e9f45e5a3bc618bdfd2e7ee

https://sketchfab.com/3d-models/izzy-animated-female-character-free-download-8476e3679aa442c3ad811727705524ac

https://sketchfab.com/3d-models/fantasy-sea-keep-daa54937ce844ed390101fa9318843ba ?

https://sketchfab.com/3d-models/sweet-tree-b8e30834ed2e4fd3a12fa24075bbc914

https://sketchfab.com/3d-models/tree-f0b457a918c046a0b9508ba86afe899b

https://sketchfab.com/3d-models/pink-rock-garden-f2f4ddd3657542c8a9fd86dcd7a1699c

I don't know how many others might break. It's really hard to find scenes that I have permission to download that have any kind of transparency at all to test with.

@mrdoob
Copy link
Owner

mrdoob commented May 18, 2020

@greggman Any chance you can share screenshots?

@donmccurdy
Copy link
Collaborator

Checking the first three examples, they are all cases where the material has enabled transparency for objects with 100% opacity. Unfortunately that seems to be pretty common on Sketchfab; I'd be curious if we can ask them to fix that in their generated glTF models.

@greggman
Copy link
Contributor

greggman commented May 18, 2020

Maybe you have different experience. In games I've rarely worked on a project that used depthWrite = false except for particles effects. It's extremely common to enable gl.BLEND for things like leaves and grass and use an alpha test discard and the depth test has to be on for it work. I'm sure there are a ton more scenes that will break where in three.js terms it needs to be (depthWrite = true, transparency = true, alphaTest = > 0) but unfortunately most of the scenes that would show this are not downloadable from sketchfab

wjwiGLP8Uo
KttvBSkioM
8BtBhCpOLr
yTiAxZYKVd
qU9P1XwPXH
WaJe6EwygL

@donmccurdy
Copy link
Collaborator

I think all of these models would be better served by transparent = false than transparent = true. Artists can't just enable .transparent=true on an entire opaque model, that will cause other kinds of problems, including overdraw at least.

The glTF format gives three alpha options:

a) alphaMode=BLEND: disable alpha test, enable alpha blend
b) alphaMode=MASK: enable alpha test, disable alpha blend
c) alphaMode=OPAQUE: disable alpha test, disable alpha blend

glTF does not specify when depthWrite should be enabled/disabled, but (with this change) we apply it only to the BLEND case by default. If these models were using the mask mode, which I think they should be, then depthWrite would not be disabled.


All that said, yes, I realize that transparency sometimes requires depthWrite=true, and in other cases requires depthWrite=false. Whichever default we choose, some things will be broken. #18235 lists other cases that were fixed by the change. I think those cases are more compelling: some of those models were designed as well as they could be, and were still broken, whereas these ones generated by Sketchfab are unfortunately not using particularly good material settings.

@greggman
Copy link
Contributor

I realize things will break either way. In my experience I expect the new way to break things far more often than visa-versa just given that I've all the games I've worked and having never turned off depth testing except for particles. But, I guess we'd have to find more scenes find out if the there is an advantage to defaulting to one way or the other. It's possible it's 50/50. It's also possible it's 90/10 one way or the other.

@donmccurdy
Copy link
Collaborator

For comparison (details in #18235) this behavior appears to be recommended by Unity's docs, and enabled by default in Unreal Engine and BabylonJS. Probably there's more of a grey area for models that need both alpha test and alpha blend, but these models don't enable alpha test. 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants
@mrdoob @greggman @Kagetsuki @donmccurdy @0b5vr and others