8000 Introduce XRViewGeometry mixin concept by alcooper91 · Pull Request #1408 · immersive-web/webxr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 3 commits into from
Apr 17, 2025
Merged

Conversation

alcooper91
Copy link
Contributor
@alcooper91 alcooper91 commented Apr 16, 2025

@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:

WARNING: Skipped generating some MDN panels, because the following IDs weren't present in the document. Use `Ignore MDN Failure` if this is expected.
  #dom-xrview-projectionmatrix
  #dom-xrview-transform

Not sure what if anything needs to be done to resolve this, but I welcome any insights


Preview | Diff

@alcooper91
Copy link
Contributor Author

Required for immersive-web/depth-sensing#59

@alcooper91 alcooper91 requested a review from toji April 16, 2025 20:34
@alcooper91
Copy link
Contributor Author

@toji PTAL (and LMK if you have a better name suggestion), and then I'll send to Rik for review.

@toji
Copy link
Member
toji commented Apr 16, 2025

XRViewCalibration isn't my favorite name, though it doesn't matter too much since it'll never be developer-visible. I don't have anything I feel is a really strong replacement, but to throw out some possible alternatives:

  • XRViewBase
  • XRViewTransforms (Projection is a transform, just not a RigidTransform. 🤷)
  • XRViewMatrices (Transform has a matrix. 🤷)
  • XRViewProjectionTransform
  • XRViewIntrinsics
  • XRAbstractView

@alcooper91
Copy link
Contributor Author

I think XRViewIntrinsics is incomplete as I think technically the transform attribute is an extrinsic (the combination of which seemingly often being referred to as a "calibration" was how I arrived at my current name).

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. XRViewBase doesn't seem terrible; but it does make it feel more like a base class setup.

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.

@alcooper91 alcooper91 force-pushed the alcooper/xr_view_calibration branch from 0912ab9 to cfe0fdb Compare April 16, 2025 22:34
@alcooper91
Copy link
Contributor Author

Another name suggestion that I think I really like is XRViewGeometry

@toji
Copy link
Member
toji commented Apr 17, 2025

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 XRProjectionTransform to be the most straightforward. (Hm... I don't mind that, actually.)

XRViewGeometry would be fine? I think it describes the projection matrix more so than the transform, but that's OK. I don't see it as strictly better than something like XRViewTransforms but I do have a mild preference for it over XRViewCalibration.

@alcooper91 alcooper91 requested a review from cabanier April 17, 2025 18:22
@alcooper91
Copy link
Contributor Author

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.

@alcooper91 alcooper91 requested a review from toji April 17, 2025 18:23
@alcooper91 alcooper91 changed the title Introduce XRViewCalibration mixin concept Introduce XRViewGeometry mixin concept Apr 17, 2025
@cabanier
Copy link
Member

@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.

@alcooper91 alcooper91 merged commit 16ffa1e into main Apr 17, 2025
2 checks passed
8000
github-actions bot added a commit that referenced this pull request Apr 17, 2025
SHA: 16ffa1e
Reason: push, by alcooper91

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@bialpio
Copy link
Contributor
bialpio commented Apr 18, 2025

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!").

@alcooper91
Copy link
Contributor Author

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?

@alcooper91 alcooper91 deleted the alcooper/xr_view_calibration branch May 1, 2025 22:28
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.

4 participants
0