-
Notifications
You must be signed in to change notification settings - Fork 400
Introduce XRViewGeometry mixin concept #1408
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
Conversation
Required for immersive-web/depth-sensing#59 |
@toji PTAL (and LMK if you have a better name suggestion), and then I'll send to Rik for review. |
|
I think XRViewIntrinsics is incomplete as I think technically the Thinking "aloud" here, the prose about this object representing some piece of physical hardware, and this being a "subset" of data for that camera/sensor was kind of how I was trying to go through this. Then again, the other item we're attaching this to is something like a sensor, so maybe dropping "View" out of this name entirely would help clarify some of it. Really everything is kind of like a Camera, and that makes it unfortunate that XRCamera is already used :(. I think from what you've proposed "XRViewTransforms" is maybe my favorite (or sticking with XRViewCalibration), but I don't have strong thoughts. It'd be interesting if we could remove "View" from the name. |
0912ab9
to
cfe0fdb
Compare
Another name suggestion that I think I really like is |
I'm not against dropping "View" from the name. I guess that the awkward part is that in most cases these values will be handed off to the GPU as a projection and view matrix. While that view isn't exactly the same as WebXR's view concept they are related. As such it feels like it's difficult to get away from the term. If you wanted to, though, I'd see something like
|
Going with XRViewGeometry for now. I don't think any of the definitions or embedded terms need changing, though I'm maybe less sure of that with the "containing object" definition. @cabanier PTAL, and let me know if you have thoughts on a better name for the mixin. Note that this name isn't exposed to developers at all; and is purely an in-spec concept. |
LGTM, since it's not exposed, we can always change it later. |
SHA: 16ffa1e Reason: push, by alcooper91 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
My 2 cents: XRViewProjectionTransform (or some variation of it) is nice because that's precisely what it's intended to be used for ("ok, uhmm, so I need a model-view-projection matrix for my shader for this object, WebXR doesn't manage my scene so the 'model' matrix is my responsibility, now where is this 'view-projection' thingy... oh, it's here!"). |
My hesitation with something like "ProjectionTransform" was that it felt a bit too much like we were just appending the names of the attributes, rather than coming up with a name that represented what it was meant to be, which could lead to less of a standard name in the future. We can certainly still change this at any time in the future since it is only a spec concept (though it'll have a correlation in code); but the easiest time is certainly now before it's implemented anywhere. Would it be worth chatting about next week? |
@toji suggested using a mixin as the editorial concept we discussed in the latest IWWG call. I quickly tested this in Chrome and it doesn't seem to trigger any unexpected web-observable changes.
However, when I run the spec through https://api.csswg.org/bikeshed/ after my changes, I see:
Not sure what if anything needs to be done to resolve this, but I welcome any insights
Preview | Diff