Skip to content

Added support for ignoreDepthValues to XRWebGLBinding #269

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

Merged
merged 1 commit into from
Nov 3, 2021

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented Oct 22, 2021

@cabanier cabanier requested review from toji and Manishearth October 22, 2021 23:33
@cabanier cabanier marked this pull request as ready for review October 22, 2021 23:34
@toji
Copy link
Member

toji commented Oct 25, 2021

This is good overall, but the name seems a little off to me. When we use ignoreDepthValues elsewhere it's something that the user had the opportunity to set. Here it only functions as a way to communicate the system's capabilities, so a name like willIgnoreDepthValues or alwaysIgnoresDepthValues feels more appropriate.

That said, during the TPAC call we also discussed that it may be more useful to communicate that a system prefers to have depth values provided, so perhaps we should consider flipping the logic and using a name like prefersDepthValues or, more generally, usesDepthValues.

@cabanier
Copy link
Member Author

cabanier commented Nov 1, 2021

That said, during the TPAC call we also discussed that it may be more useful to communicate that a system prefers to have depth values provided, so perhaps we should consider flipping the logic and using a name like prefersDepthValues or, more generally, usesDepthValues.

Thanks! I updated the spec so it reads usesDepthValues

Should XRProjectionLayer use the same attribute name?

@toji
Copy link
Member

toji commented Nov 3, 2021

I could go either way on that. It's nice to have them be aligned but the context is a bit different and the attrib in the XRProjectionLayer has already shipped in at least one browser, right? I don't think that makes it unchangeable, but I also don't feel the need to break things in this case.

@cabanier
Copy link
Member Author

cabanier commented Nov 3, 2021

I could go either way on that. It's nice to have them be aligned but the context is a bit different and the attrib in the XRProjectionLayer has already shipped in at least one browser, right? I don't think that makes it unchangeable, but I also don't feel the need to break things in this case.

Yes, we already shipped it and three is checking it.

@cabanier cabanier merged commit 398175f into immersive-web:main Nov 3, 2021
@cabanier cabanier deleted the usedepthvalues branch November 3, 2021 16:45
github-actions bot added a commit that referenced this pull request Nov 3, 2021
SHA: 398175f
Reason: push, by @cabanier

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Communicate earlier that the UA doesn't need a depth texture
2 participants