Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

AlphaBlendModeTest assumes ALPHA_TO_COVERAGE is disabled #221

Closed
prideout opened this issue Apr 3, 2019 · 18 comments
Closed

AlphaBlendModeTest assumes ALPHA_TO_COVERAGE is disabled #221

prideout opened this issue Apr 3, 2019 · 18 comments

Comments

@prideout
Copy link

prideout commented Apr 3, 2019

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:
bad
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.

@emackey
Copy link
Member

emackey commented Apr 3, 2019

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 rendered output is either fully opaque or fully transparent depending on the alpha value and the specified alpha cutoff value.

A depth value is not written for a pixel that is discarded after the alpha test. A depth value is written for all other pixels. Mesh sorting is not required for correct output.

@prideout
Copy link
Author

prideout commented Apr 5, 2019

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.

@emackey
Copy link
Member

emackey commented Apr 5, 2019

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?

@bghgary
Copy link
Contributor

bghgary commented Apr 17, 2019

There is a conflict between two different uses of alpha when in MASK mode.

  1. Simulate geometry that is progressively shown/hidden, like this sample model, for variations or possibly for animation with an extension.
  2. Alpha to coverage for anti-aliasing the edges of an alpha masked geometry.

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.

@emackey
Copy link
Member

emackey commented Apr 17, 2019

@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.

@bghgary
Copy link
Contributor

bghgary commented Apr 17, 2019

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.

@emackey
Copy link
Member

emackey commented Apr 17, 2019

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.

@bghgary
Copy link
Contributor

bghgary commented Apr 17, 2019

most/all of the following rendering engines will need to change

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.

@emackey
Copy link
Member

emackey commented Apr 17, 2019

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.

@bghgary
Copy link
Contributor

bghgary commented Apr 17, 2019

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.

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?

@emackey
Copy link
Member

emackey commented Apr 17, 2019

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.

@bghgary
Copy link
Contributor

bghgary commented Apr 17, 2019

Removing the test for consistency is not a good solution

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.

I think this choice has already been well established with tests and implementations and sample/reference code

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.

@emackey
Copy link
Member

emackey commented May 1, 2019

I am not even remotely suggesting that we remove this test.

Sorry, was referencing a comment from earlier in this thread before you joined.

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.

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.

@DRx3D
Copy link
Contributor

DRx3D commented Nov 25, 2023

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.

@emackey
Copy link
Member

emackey commented Nov 27, 2023

I think we reached a stalemate here. ALPHA_TO_COVERAGE is a cool technique, but glTF doesn't explicitly enable or deny its use, and this particular test model isn't easily adjusted to allow it without damaging the cutoff tests.

The current trend in the PBR TSG is towards specifying real physical properties of materials, and avoiding specifying particular rendering techniques such as ALPHA_TO_COVERAGE. The latter is left as an implementation detail, so that engines may use whatever technique works best for them. So, I think we're unlikely to see movement here in the near term.

@emackey emackey closed this as completed Nov 27, 2023
@lexaknyazev
Copy link
Member

For the record, the spec explicitly allows use of A2C for the "masked" blend mode.

@emackey
Copy link
Member

emackey commented Nov 27, 2023

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.

@lexaknyazev
Copy link
Member

Yes, in KhronosGroup/glTF#2067.

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

No branches or pull requests

5 participants