-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 OpenXR local floor extension #87235
Conversation
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.
Code looks good and I tested and it works as expected!
For the fallback, the spec recommends an alternative application specific space in https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_EXT_local_floor. I'd suggest we use that instead and add it to this PR.
Are you referring to the note below? "Note: The EDIT: Oh, and I see the code sample a little further down in the spec as well. I'll see how we can integrate that! |
Yeah, that's the one I was referring to. |
db440aa
to
13cbab6
Compare
I've hit a little bit of a snag in implementing the example code to emulate I think the solution is just to defer doing this until after the first |
13cbab6
to
c76bd93
Compare
Nevermind! It looks like I was making a very small and very dumb mistake - the emulation code code does, in fact, work during initialization when using the arbitrary time value of However, it may still make sense to refactor it so we can re-setup the reference space with a new floor height from |
c76bd93
to
4bade79
Compare
Alright! I've refactored this so that (a) we can reset the floor height when using the emulated I've tested all the different branches (including real @m4gr3d @BastiaanOlij What do you guys think? Another question: Since it seems that using |
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.
The code looks good!
The problem here is that this is another example where OpenXR offers a solution that is ideal when scratch building an game/application, but which clashes with existing logic in game engines that have already provided solutions 5 or more years ago. Quest also has a one up on the competition, the player is already wearing the headset and already positioned where they want to engage with a game or application, when the game or application starts. So for them these modes make sense. Other platforms, PCVR, PSVR2, etc. can't make this assumption, the user often is interacting with a computer directly, with a headset set on the floor or on a table or chair. The game starts, and by the time the user has walked to the center of their play space and put the headset on, I've found this experience really annoying with say Elite Dangerous which has attempted to automate this, and do their calibration once the player puts on their headset. It gets it wrong 99% of the time simply because it takes seconds before the user has adjusted the headset to sit comfortably on their head. It is for this reason that many games, and we have this standard in XR tools, will have a beginning environment that is not susceptible to a tracking mode (usually by being in a LARGE room) and you get the infamous, standard on console games for a few decades, "press any key/button to continue". As this happens when the player is ready, this is where you perform calibration, and that calibration can be heavily customised and often is. So I'm not so fussed with a fallback, for most of our users neither |
@BastiaanOlij Are you saying that in your opinion we shouldn't be attempting to emulate If we do go that route, and just try |
We already default to
On recenter you simply:
What you often also do an automatic recenter, but either on the infamous "press a button to continue" approach, or after a given time where you can assume the user is now in the right spot wearing their headset (not a fan). Like I said, in XR tools we've used option 1 for many years, far before OpenXR introduced reference spaces. That works on all platforms consistently and solves the issue when you can't be guaranteed the user is ready when your game starts. Now if you do want to use
So we assume the XR runtime takes care of things unless our mode is roomscale, else we fall back on our existing logic. In writing that I do realise we never fully implemented the play area mode. It's there in the background but ignored in the OpenXR implementation, so that needs to be added. (sorry for the edits, this was going down memory lane and verifying a few things in code) |
Ok, thanks! I generally agree with that, and exposing the selected reference space to the developer was what I had in mind, so I can try and finish implementing that bit. I'll update this PR a little later. :-) However, I think we may be a little misaligned on the use case for the different reference space types, and which should be manually recentered using
I think there are still valid use-cases for
Both Or, are you basically just explaining how you'd do something similar to those spaces while using |
4bade79
to
698a1cf
Compare
In my latest push:
Anyway, I'm feeling pretty happy with this implementation now! It should allow @m4gr3d @BastiaanOlij Please let me know what you think! |
Yeah basically that is the point. We have an existing system long before OpenXR and reference spaces were introduced, and that should remain the default behavior as to not break peoples existing games and projects. Alternatively you can use However I think it is very important to underline that
This btw is in no means a reason not to implement or use it. It is just a weakness that needs to be clearly documented so developers know what the pitfalls are and don't draw the wrong conclusion. All these issues are solvable. |
modules/openxr/openxr_interface.cpp
Outdated
@@ -690,6 +690,20 @@ bool OpenXRInterface::supports_play_area_mode(XRInterface::PlayAreaMode p_mode) | |||
} | |||
|
|||
XRInterface::PlayAreaMode OpenXRInterface::get_play_area_mode() const { | |||
if (!openxr_api) { |
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.
Don't forget to also implement supports_play_area_mode
and set_play_area_mode
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.
Do we want to support changing the reference space mid-game? Right now it takes this value from a project setting that's only used during initialization.
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.
I wonder if we can, forgot that we do this during init. If we need to tear down a session and rebuild it, it will be overkill and it indeed makes more sense that set_play_area_mode
gives and error after OpenXR is initialised. But if it can change on the fly that would be cool.
Imagine switching between LOCAL
and LOCAL_FLOOR
as you enter/exit a vehicle
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.
I think we can. But, personally, I'd be fine with this only being allowed during init (or like you suggest, giving an error if set when OpenXR is already initialized), at least until we hit a compelling use case that really needs it. I worry that giving developers this power would cause more problems than it would solve. :-)
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.
Agreed :)
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.
@dsnopek @BastiaanOlij I ran into this while working on the XR editor and I'd suggest removing the restriction (if possible) that the api can only be used prior to initialization.
The project setting applies to the entire project, which doesn't provide much granularity. And trying to use this api prior to the XRInterface
being initialized is practically impossible since by the time gdscript
can get a handle to the XRInterface
instance, it's already initialized.
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.
Hm. In a normal project, you'd set it in the "Project Settings" so that it's at the value you want during initialization, however, I can see how that would be an issue in the XR editor.
From a technical perspective, I think we are able to change reference space after OpenXR is initialized, and so we could remove the restriction. However, I don't know what the experience would be in the headset - I can do some experimentation. In the worst case (like if it's really weird in headset), I guess we can recommend in the docs that developers are careful with this function and maybe black out the camera before doing it or re-center on the HMD after doing it, or something like that?
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.
We can also recommend switching the reference space only during transition from one scene to another.
For now for the XR editor, I'm creating an editor override for the reference space project setting in m4gr3d@6dd167a.
Ideally, it should be possible to update the reference space while the editor is being initialized in https://github.com/m4gr3d/godot/blob/6dd167a3bcc88dc789e36fdc6b9d9b967b6d7321/modules/vr_editor/vr_editor.cpp#L44
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.
@@ -256,7 +256,7 @@ | |||
Player is free to move around, full positional tracking. | |||
</constant> | |||
<constant name="XR_PLAY_AREA_STAGE" value="4" enum="PlayAreaMode"> | |||
Same as [constant XR_PLAY_AREA_ROOMSCALE] but origin point is fixed to the center of the physical space, [method XRServer.center_on_hmd] disabled. | |||
Same as [constant XR_PLAY_AREA_ROOMSCALE] but origin point is fixed to the center of the physical space. In this mode, system-level recentering may be disabled, requiring the use of [method XRServer.center_on_hmd]. |
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.
I would suggest doing this the other way around. XR_PLAY_AREA_ROOMSCALE
is the mode where the center of the playspace is locked in place as this is the default room scale behavior most headsets adhere to.
The new mode could then be called XR_PLAY_AREA_ROOMSCALE_LOCAL
which then has a description such as Same as XR_PLAY_AREA_ROOMSCALE
but the center of the play space is calibrate to the position of the player at startup, and when the player does a system recenter. It is important to note that the player can walk away from this center`.
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.
So, you're saying that XR_REFERENCE_SPACE_TYPE_STAGE
should map to XR_PLAY_AREA_ROOMSCALE
? We certainly could do that, the terminology can be whatever we want. :-)
However, it feels weird that XR_PLAY_AREA_STAGE
would not equal XR_REFERENCE_SPACE_TYPE_STAGE
, given that they both contain STAGE
(and the project setting for this also calls it "Stage"). And given the original description of this constant (I only changed the end bit about center_on_hmd()
), it certainly seems like this value was meant to be meant to be used for XR_REFERENCE_SPACE_TYPE_STAGE
.
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.
Owh, XR_PLAY_AREA_STAGE
already exists... hmmm weird.. I see your point.. I think I really badly chose these names way back when :)
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.
I'm tempted to rename them to XR_PLAY_AREA_ROOMSCALE_LOCAL
and XR_PLAY_AREA_ROOMSCALE_STAGE
, just to set them apart as two roomscale variants..
We'll get in trouble with the production team for a breaking rename but I don't think anyone will even have been aware that we had these options seeing it was never implemented.
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.
If totally renaming them is a possibility, I think having just XR_PLAY_AREA_LOCAL
and XR_PLAY_AREA_LOCAL_FLOOR
would be great. Those do seem to be sort of standard names - they also exist in WebXR.
Do any low-level XR APIs even have a "space type" for "sitting" or "3dof"? Those are the other values on this enum, but they're not really mappable to anything in OpenXR or WebXR.
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.
I believe the mobile VR interface exposes itself as 3DOF as it doesn't have positional tracking, so I'd keep that. I'm happy with having that and the 3 suggestions you make, and removing the rest.
We'll have to make a clear statement as to why we're making this breaking change and why we think we can get away with it (this feature having been unimplemented is a strong indicator that nobody is currently using it:P ).
(obviously if we need to keep the old ones and give them the same const value and mark them as deprecated, that could be a way to make any critics happy, but I rather see them removed).
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.
The updated logic looks good!
IMO, providing a fallback reinforce our intent for those apis to be cross platform; i.e: the developer can select |
I see your point, I think what I worry about is that there is no guarantee we can support fallbacks in the future, and its a fair bit of code that at some point will become obsolete as vendors support this extension. But I guess it does offer the most consistent behaviour and removes platform differences which indeed is exactly what I'm trying to achieve :) |
@dsnopek this all looks good, I think the only things that should still be added in are:
I'm still not 100% convinced Time will tell which one will be preferred by users. I think people will be caught out in games if they don't realise the effects of recentering in |
698a1cf
to
cc71d9c
Compare
cc71d9c
to
a8690cb
Compare
@BastiaanOlij Thanks! I've implemented |
Yeah looks good to me, happy for this to be merged. |
Thanks! |
This adds support for the OpenXR local floor extension:
https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_EXT_local_floor
And allows the user to select it as their reference space.
Something that isn't ideal: if the developer picks "Local Floor", and it isn't supported, it'll probably fallback on the "Local" reference space, where I'd expect most developers to actually want "Stage" as the fallback. Should we try adding a more controlled fallback system in this PR? Or, save that for a follow-up?UPDATE: This PR will now use the method of emulating
LOCAL_FLOOR
(if it's not supported) usingSTAGE
andLOCAL
that is described in the OpenXR spec at the end of this section.This works in my testing on a Meta Quest 3.