Skip to content

Added support for ignoreDepthValues to XRWebGLBinding#269

Merged
cabanier merged 1 commit intoimmersive-web:mainfrom
cabanier:usedepthvalues
Nov 3, 2021
Merged

Added support for ignoreDepthValues to XRWebGLBinding#269
cabanier merged 1 commit intoimmersive-web:mainfrom
cabanier:usedepthvalues

Conversation

@cabanier
Copy link
Copy Markdown
Member

@cabanier cabanier commented Oct 22, 2021

@cabanier cabanier requested review from Manishearth and toji October 22, 2021 23:33
@cabanier cabanier marked this pull request as ready for review October 22, 2021 23:34
@toji
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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