Add XRView to XRDepthInformation #50
Conversation
|
Spoke offline yesterday and proposed some way to make this not a breaking change, suggested adding a new dictionary member that can be default false and |
|
This was discussed at TPAC and the consensus was that we should return an |
cabanier
changed the title
|
Addresses #45 |
|
Overall this approach seems fine; except that I thought we had also discussed a way for the site to opt-in/acknowledge "Yes, I'm willing to handle this optional view" (Which may have been either a method to populate that view or to otherwise acquire it?) |
Yes, I didn't add that yet because the power outage stopped our meeting mid-way |
|
@alcooper91 do you want to address the optionality first before this is merged? |
|
Yeah, I'd prefer to land that all at once so we don't have a weird in-between state of the spec. I won't be able to attend the next meeting, but I'm happy to go along with whatever folks decide if you think we should continue the discussion at that? |
|
/agenda discuss adding an optional feature that accomodates slower depth refresh rate |
+ made reprojection optional
There was a problem hiding this comment.
matchDepthView seems fine, but I think depthNear and depthFar address a different issue, so I'd prefer if we could split them to their own PR (though I also thought we had discussed that maybe we didn't need them, but it somewhat depended on some math that we should potentially include in the spec if we do?).
I'm not sure that the isValid change is necessary, given that we can also simply return null, per the existing spec text? I worry that sites will not check this and will simply expect that if they receive an XRDepthInformation that it is valid.
| }; | ||
| </script> | ||
|
|
||
| The {{XRDepthStateInit/usagePreference}} is an ordered sequence of {{XRDepthUsage}}s, used to describe the desired depth sensing usage for the session. | ||
|
|
||
| The {{XRDepthStateInit/dataFormatPreference}} is an ordered sequence of {{XRDepthDataFormat}}s, used to describe the desired depth sensing data format for the session. | ||
|
|
||
| The {{XRDepthStateInit/matchDepthView}} requests that {{XRDepthInformation/view}} of the depth information must be in sync with the {{XRView}}. If this is <code>true</code>, the {{XRSystem}} SHOULD return depth information that reflects the current frame. If this is <code>false</code>, the {{XRSystem}} MAY return depth information that was captured at an earlier point in time. |
There was a problem hiding this comment.
If we're going to allow an earlier point in time, we should probably add a time element to the data?
| }; | ||
| </script> | ||
|
|
||
| The {{XRDepthStateInit/usagePreference}} is an ordered sequence of {{XRDepthUsage}}s, used to describe the desired depth sensing usage for the session. | ||
|
|
||
| The {{XRDepthStateInit/dataFormatPreference}} is an ordered sequence of {{XRDepthDataFormat}}s, used to describe the desired depth sensing data format for the session. | ||
|
|
||
| The {{XRDepthStateInit/matchDepthView}} requests that {{XRDepthInformation/view}} of the depth information must be in sync with the {{XRView}}. If this is <code>true</code>, the {{XRSystem}} SHOULD return depth information that reflects the current frame. If this is <code>false</code>, the {{XRSystem}} MAY return depth information that was captured at an earlier point in time. | ||
|
|
||
| NOTE: If {{XRDepthStateInit/matchDepthView}} is <code>false</code>, the author should do the reprojection using the [=XRDepthInformation/view=] from {{XRDepthInformation}}. |
There was a problem hiding this comment.
Do we want to provide any sample math for this? (I have some in Chromium C++)
There was a problem hiding this comment.
We could add that as an informative note but I prefer if we updated the sample.
There was a problem hiding this comment.
Per meeting this week, let's start with the sample.
| @@ -308,6 +314,10 @@ interface XRDepthInformation { | |||
| [SameObject] readonly attribute XRRigidTransform normDepthBufferFromNormView; | |||
| readonly attribute float rawValueToMeters; | |||
| readonly attribute XRView? view; | |||
|
|
|||
| readonly attribute boolean isValid; | |||
| readonly attribute float depthNear; | |||
There was a problem hiding this comment.
I think these maybe snuck in here from another change? Is it possible to keep them as a separate change (Issue for them is Issue 43, though I thought we'd come to a decision that maybe we didn't need to do this?) Per offline chats with Piotr, if we are going to merge them in we'd want some math on how to use them, and he thinks there may be additional spec text changes needed.
There was a problem hiding this comment.
In a previous discussion, I had mentioned that I would combine them in 1 PR.
Happy to pull it into separate ones if it's more clear.
There was a problem hiding this comment.
Per meeting this week, let's split so we can merge this, since I think more tweaks may be needed for that one.
| The optional {{XRDepthInformation/view}} attribute contains the {{XRView}} that was active when the {{XRSystem}} calculated the {{XRDepthInformation}}. This attribute MAY be used by experiences to better align with the real world. If the {{XRDepthInformation/view}} is not provided, the user MUST assume it is the same as the one from the current {{XRFrame}}'s {{XRViewerPose}}. | ||
| The {{XRDepthInformation/depthNear}}, {{XRDepthInformation/depthFar}} and optional {{XRDepthInformation/view}} attribute contain the near and far field and the {{XRView}} respectively that were active when the {{XRSystem}} calculated the {{XRDepthInformation}}. This attribute MAY be used by experiences to better align with the real world. If the {{XRDepthInformation/view}} is not provided, the user MUST assume it is the same as the one from the current {{XRFrame}}'s {{XRViewerPose}}. | ||
|
|
||
| The {{XRDepthInformation/isValid}} attribute is set to <code>true</code> if the {{XRSystem}} was able to generate depth information for the current frame. It is set to <code>false</code> otherwise. |
There was a problem hiding this comment.
I thought we had the ability to simply just not return it otherwise?
There was a problem hiding this comment.
I found some pages that didn't handle this correctly. It takes our system multiple frames after starting or resuming a session to provide depth depth and this breaks content since it didn't check.
There was a problem hiding this comment.
Per meeting this week, let's file bugs on those pages that break. We do currently return null in some situations today, and the risk is that sites that are following the current spec would start using invalid data without knowing it.
|
@cabanier what's the status on this? |
I'd still like to expose this through the depth API. Reprojection of the depth pixels is a lot more efficient if it is done by the developer. |
|
Oh I agree! I was just asking if there were any other blockers beyond the cleanups we had most recently discussed |
I don't think so; I've just been to busy to spend more time on it |
/tpac
Preview | Diff