-
Notifications
You must be signed in to change notification settings - Fork 1.3k
AlphaBlendModeTest assumes ALPHA_TO_COVERAGE is disabled #221
Comments
My understanding of the current spec is that the fully-opaque portion of the gradient is intentional: https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#alpha-coverage
|
The spec is poorly specified here because it makes no mention of ALPHA_TO_COVERAGE. If the test stays as-is, renderers will need to either: (a) keep ALPHA_TO_COVERAGE enabled but "fail" this test. (b) disable ALPHA_TO_COVERAGE to "pass" the test, and live with edge aliasing that will arise in real-world use cases such as tree foliage. My suggestion is to remove this particular gradient from the model, and instead test masking via the text labels. If an implementation does not support masking properly, the text labels would have a black background. |
I think we would prefer to have visual consistency here, not just remove a test. This particular test is one that many viewers fail by enabling full blend mode here in addition to the cutoff, and this model has helped cut down on that behavior. /cc @bghgary Any thoughts here? |
There is a conflict between two different uses of alpha when in MASK mode.
This is a spec issue as there is no way to know from a glTF 2.0 material which is the intended scenario. I think 2 needs to be supported to have better quality alpha masked geometry which implies that 1 should be disallowed. |
@bghgary Agreed. And just to clarify, this sample model isn't attempting to show progressively faded geometry on the MASK tests, it's simply trying to visually show what each engine does with the alphaCutoff value. The cards are intended to be fully opaque for alpha values that survive the MASK cutoff. @prideout I think if an ALPHA_TO_COVERAGE mode is desired, there would need to be a new glTF extension proposed for it, as the core spec doesn't include it. |
I don't think I agree. The spec is currently ambiguous. An extension can certainly clarify, but the core spec should not have any ambiguity. |
If we clarify the core spec by allowing ALPHA_TO_COVERAGE, most/all of the following rendering engines will need to change: https://cx20.github.io/gltf-test/#tutorialFeatureTestModelTable I'm not saying ALPHA_TO_COVERAGE is a bad idea, just that the ecosystem has been well established without it. It is a breaking change to add it at this point. |
I don't follow. What has to change in the rendering engines if we allow alpha to coverage? But regardless, I don't think leaving the spec ambiguous is good. Either we say alpha to coverage is allowed or not allowed. At the minimum there should be an implementation note. |
We should specify it, but I don't think we should specify it as optional, because it dramatically changes the look of the result when compared to a simple alpha cutoff value. If we're going for visual consistency, we should either forbid it or enforce it, not leave it up to the client. If we decide to enforce it, that means code changes to ThreeJS, Cesium, Babylon, Office, STK, the new Khronos Sample Viewer, the Blender importer, and probably a half-dozen other implementations, doesn't it? These implementations have been in the wild, some of them for up to 2 years, with this option turned off. I would think a new glTF extension would be preferable to such a sweeping change. |
I'm not sure. General postprocess antialiasing is already optional for most renderer. We don't enforce that all renderers use antialiasing. It changes the result, but not dramatically. It is similar for alpha to coverage, no? |
That's a great point. But I think my issue is, general postprocess antialiasing happens only at the fringes, and while this alpha coverage issue is intended to happen only at the fringes, the reality is that a swath of medium alpha values can cause this to happen in an arbitrarily large central portion of a surface, as this test model does. Removing the test for consistency is not a good solution, particularly after the portion of the test in question has a year-long track record of exposing ALPHA_BLEND bugs in MASK implementations across lots of engines. Removing the test won't prevent models in the wild from doing it. Theoretically, if we specify that it's optional, and we keep this test, we could update the test's "pass" screenshots to show what happens when alpha coverage is enabled. It may be difficult to visually distinguish between BLEND and coverage when only one polygon is involved, but I suppose we could live with that. Even so, models in the wild that do unorthodox things with MASK and alpha values could behave very differently depending on this setting. That's why my instinct is to make it a deliberate choice in the spec, and indeed I think this choice has already been well established with tests and implementations and sample/reference code. |
I am not even remotely suggesting that we remove this test. There may be other ways to test what it is doing currently? I haven't given this much thought. I'm more worried about the spec then the test at this point.
Are you saying we should not allow alpha to coverage in the spec? IMO, alpha to coverage not being turned on (or in some cases, not implemented) in viewers doesn't imply that better solutions should be disallowed. |
Sorry, was referencing a comment from earlier in this thread before you joined.
Yes, certainly I want viewers to look their best, and it sounds like this falls into that camp. And to be fair, we do have other places in the spec (notoriously, the choice of default min/mag filter) where clients are free to make decisions that affect the visual appearance of the model itself, independently of any post-processing. I think my lack of experience with this particular option is hurting my understanding here. If I had a clearer picture of how to visually distinguish a client that uses ALPHA_TO_COVERAGE with a mask as opposed to one that inappropriately uses BLEND with a mask, that would help a lot. |
This has not been commented on in over 4 years. This can either be closed or transferred to Sample-Assets. The default operation is to close this issue by 1 Dec. |
I think we reached a stalemate here. The current trend in the PBR TSG is towards specifying real physical properties of materials, and avoiding specifying particular rendering techniques such as |
For the record, the spec explicitly allows use of A2C for the "masked" blend mode. |
Oh, I missed that. Was that added as part of the ISO cleanup? It's good to see it's explicitly optional in the spec. |
Yes, in KhronosGroup/glTF#2067. |
I think the primary use case for the "masked" blend mode is for nice edges (e.g., foliage), therefore many engines will want to enable ALPHA_TO_COVERAGE for this.
However, in this screenshot, alpha-to-coverage is clearly disabled, since the gradient appears totally opaque until the hard cutoff.
If alpha-to-coverage and MSAA enabled, the screenshot might look more like this:
It might be a better test if it were more representative, e.g. if it used alpha cutoff for the text labels, rather than applying to a large gradient. Alternatively maybe the test's README should simply make mention of alpha-to-coverage.
The text was updated successfully, but these errors were encountered: