Skip to content

Conversation

@cabanier
Copy link
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
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

2 participants