Allow depth&&stencil result if depth||stencil requested. by kdashg · Pull Request #987 · immersive-web/webxr · GitHub
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

Allow depth&&stencil result if depth||stencil requested. #987

Merged
merged 1 commit into from
Mar 24, 2020

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented Mar 18, 2020

We don't want to encourage !depth&&!stencil result if depth xor stencil.

We don't want to encourage !depth&&!stencil result if depth xor stencil.
@cabanier
Copy link
Member

That sounds reasonable and likely already the way that this is implemented.

@kdashg
Copy link
Contributor Author

kdashg commented Mar 18, 2020

Actually I had forgotten, but WebGL 1 went the other way on this for portability reasons:

The depth, stencil and antialias attributes, when set to true, are requests, not requirements. The WebGL implementation should make a best effort to honor them. When any of these attributes is set to false, however, the WebGL implementation must not provide the associated functionality.

I was remembering the original behavior before we ran into the portability issues that caused the wording we have today.

WebGL 2 made them strict, non-optional:

Different from WebGL 1.0, the depth, stencil, and antialias attributes in WebGL 2.0 must be obeyed by the WebGL implementation.

Behind the scenes depth-xor-stencil are almost always DEPTH24_STENCIL8 with special handling.
I want to follow up on this PR to propose the barely-backwards-incompatible depth||stencil => depth&&stencil requirement instead.

@toji
Copy link
Member

toji commented Mar 18, 2020

I'm fine with this, as it trends towards my overall desire to have the opaque framebuffer mimic the default framebuffer behavior. Especially since any backwards compatibility issues introduced by the change would be vanishingly rare in practice. You're only going to be surprised by having a stencil buffer when you didn't expect one if you've enabled stencil test, which you really shouldn't be doing if you don't think you have a stencil buffer. On the flip side, I have a hard time imagining ANY type of XR content that will ask for a stencil buffer but explicitly not a depth buffer.

I'll take a pass at updating the spec text like @jdashg suggested above.

Worth noting that it seems like the WebGL spec is mildly self-contridictary on this subject, as in one location it says:

The depth, stencil and antialias attributes, when set to true, are requests, not requirements. The WebGL implementation should make a best effort to honor them.

Then further down, in the Context creation parameters section, states:

depth
If the value is true, the drawing buffer has a depth buffer of at least 16 bits. If the value is false, no depth buffer is available.
stencil
If the value is true, the drawing buffer has a stencil buffer of at least 8 bits. If the value is false, no stencil buffer is available.

Which implies more strictness than the prior paragraph.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh

I'll take a pass at updating the spec text like @jdashg suggested above.

... I say, not realizing that I'm making a comment in a pull request. 🙄

Change LGTM, I'd like @Manishearth to give a thumbs up as well prior to merging.

@toji toji requested a review from Manishearth March 18, 2020 21:29
@toji
Copy link
Member

toji commented Mar 20, 2020

Hey Jeff, can you make sure your GitHub account is linked to the Mozilla org so that our ipr process is happy with the change? Details here

@kdashg
Copy link
Contributor Author

kdashg commented Mar 24, 2020 via email

@kdashg
Copy link
Contributor Author

kdashg commented Mar 24, 2020 via email

@toji
Copy link
Member

toji commented Mar 24, 2020

Great, thanks! Revalidate was finicky but it looks like it worked.

@toji toji merged commit 99b9991 into immersive-web:master Mar 24, 2020
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.

4 participants