-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add support for rui16 and allow empty preference #49
Conversation
Just a small note, but in the rendering world usually the precision of data is defined in bits, not bytes. E.g. R32F - 32 bits single channel. |
Algorithm change seems good to me, with one NIT to consider; I'll defer to Piotr on the new enum naming. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd prefer Brandon/Piotr to weigh in on the enum naming.
It'd be nice to have some prose around this - mentioning that UA can have a preference for the format in the native device concepts section should be sufficient.
I seem to recall that luminance-alpha was the only 16bit format that is supported by WebGL1. |
Do we still care about being compatible with WebGL1? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd prefer Brandon/Piotr to weigh in on the enum naming.
Enum naming LGTM!
Do we still care about being compatible with WebGL1?
Luminance+Alpha has been part of this spec from the beginning, so I don't think we benefit from trying to remove it. That said, I'm not personally opposed to pursuing WebGL 2-only integrations in the future. WebGL 2 is effectively ubiquitous at this point, and certainly any devices that are attempting to use advanced WebXR functionality will have access to it.
I added definitions for preferred values. |
@bialpio does this look reasonable? If not, can we submit the PR and iterate on the language a bit more? I'd like to update browser as soon as possible so we're compliant. |
LGTM w/ nitpicks. |
SHA: dcb4d23 Reason: push, by cabanier Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I believe there is an issue with this change, that is related to lack of specifics in specs regarding to "unknown" or "not provided" options in This creates a nasty situation in that we don't know what values are available on the platform (XRDepthDataFormat - is not exposed in implementations as an array), and we have to use conditional This could easily be avoided and improved for future options changes, by specifying rules of how "unknown" or "missing" values in
|
This is a naturally enforced fallout of the fact that Quest is throwing because I believe it only supports A workaround added as of this spec change (though Chrome will likely still give an error, at least if it's a The reason that |
Chrome currently does not support an empty array of
That is not what I get (Browser version 34.0.0.121.29.615295309, OS version: 67.0.0.504.356.616599631). XRSession reports that |
That's a bug in Quest browser. It will be fixed in 34.2 |
If it's under Edit: Oops, missed his reply. |
Tested more cases, and apparently there are many implementation inconsistencies/issues:
|
I just tried this by passing an empty |
Yes, an empty |
Okay, that's a bug that the feature is still listed as enabled there. Can you test on Beta with depth under
IIUC, this is a known bug fixed in Oculus 34.2 |
Test locally and seems to be fixed. I think the fix went into M127. |
@bialpio , should I define the definitions of the user agent's preferred format and usage?
Also, luminance-alpha and unsigned-int are very similar. Is there a reason you chose luminance-alpha?
Preview | Diff