-
-
Notifications
You must be signed in to change notification settings - Fork 23.9k
Expose eye visibility controls for composition layers and set XrSpace based on parent type #113096
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
base: master
Are you sure you want to change the base?
Conversation
|
I wonder if we should auto detect the view pose based on the composition layer being a child of an Other than that, worth supporting I guess. cc @dsnopek |
modules/openxr/extensions/openxr_composition_layer_extension.cpp
Outdated
Show resolved
Hide resolved
f73f367 to
f9eb5aa
Compare
|
Please also fix the style issues before we go ahead and run CI again |
f9eb5aa to
a8fe0a5
Compare
Just curious to see if I understand, but if a composition layer is a child of the XR camera, it makes sense to default (or force) to using the |
At the moment composition layers can only be children of the |
|
I'm curious about your use case is for eye visibility?
I agree that this would be a better "API" for this, than exposing it as a property - it would be much more intuitive to Godot developers. As it is now, I think it'll be very confusing, because the position in Godot will be relative to the |
|
@BastiaanOlij @dsnopek I like the suggestion to automatically switch to View space when a composition layer is a child of the XR camera — I’ll update the PR accordingly! Regarding the use case for eye visibility: I’m currently working on a project that streams stereoscopic video from external cameras into head-locked composition layers to provide depth perception to the user. I needed a way to selectively render to each eye's panel or else the panels would cross over into the view of the other eye. I could also see this being useful for asymmetric UI/HUD elements where each eye needs different information. In the past, I’ve worked on XR projects for vision-science research as well, and I believe this feature could support similar experimental setups. Before implementing this, I found this repo and wanted to build upon that idea so other developers could achieve the same effect more easily, especially since the OpenXR layer structs already support configuring eye visibility. |
|
@dsnopek As @m4gr3d points out, to support automatically switching between pose spaces based on the layer’s parent, we would need to remove the following configuration warning: I’m not completely sure what the broader implications of removing this check would be, but I assume it was added for a good reason. Before making any changes here, I wanted to get your thoughts on whether this warning is still necessary or if it could be relaxed to allow the automatic behavior. |
We never use world locked poses with composition layers, they've always been local to the XROrigin3D node, hence that node being the parent. |
You wouldn't remove the check, just allow the parent to be either XROrigin3D or XRCamera3D node. |
Ah, very cool!
👍
Keeping the method is fine, but it shouldn't be exposed to users |
Oops, no I meant keeping it on the layer struct. I scrolled down too far :) I would remove it from the user facing node all together. |
a8fe0a5 to
49ab436
Compare
| Transform3D reference_frame = XRServer::get_singleton()->get_reference_frame(); | ||
| Transform3D transform = reference_frame.inverse() * p_transform; | ||
| Quaternion quat(transform.basis.orthonormalized()); | ||
| Transform3D xf; |
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.
Nothing to do with your PR but I just realised, the fact that composition layers are positioned with quaternion and position means we're loosing scale.
There was a bug report of that some time ago around world_scale not being applied, and this explains why :)
cc @dsnopek
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 not sure it makes sense to apply scale to composition layers. Or, at least, it's not what I would want in like 99% of circumstances, so I don't think it should happen automatically. Like, in the Alice demo you wouldn't want your settings panel to change size depending on whether your character is big or small - it should be sized based on real world sizes in either case.
To really scale a composition layer you'd need to scale the size of the shape (quad, equirect, cylinder). I suppose we could have a checkbox that, if enabled, would lead to multiplying the shape sizes by the world scale? But I really don't think this will be very commonly used
BastiaanOlij
left a comment
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.
Purely based on code review and earlier discussions, this looks good to me. Probably too late to merge into 4.6 but IMHO suitable for an early merge in 4.7
dsnopek
left a comment
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.
Thanks!
I haven't had a chance to test this yet, but the code looks pretty good to me! I just have a couple nitpicks.
I agree that this should be saved for Godot 4.7
49ab436 to
e9a7446
Compare
e9a7446 to
e742d9d
Compare
dsnopek
left a comment
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.
Thanks! Looks good to me :-)
| @@ -686,6 +729,7 @@ void OpenXRCompositionLayer::_notification(int p_what) { | |||
| } break; | |||
| case NOTIFICATION_LOCAL_TRANSFORM_CHANGED: { | |||
| update_transform(); | |||
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 still need the call to update_transform() given that _update_pose_space() also calls composition_layer_extension->composition_layer_set_transform(...).
Following on that thought, do we need to deprecate and remove update_transform() and just use _update_pose_space() instead?
Automatically set pose space for OpenXR composition layers and improve transform handling
This PR improves
OpenXRCompositionLayerby automatically selecting pose space based on the parent and ensuring robust transform updates.✔ Automatic Pose Space Selection
XRCamera3D→ layer becomes head-locked (POSE_HEAD_LOCKED)XROrigin3Dor other → layer is world-locked (POSE_WORLD_LOCKED)Removes the previous requirement for layers to be parented under
XROrigin3Dwhile maintaining correct OpenXR behavior.✔ Robust Transform Handling
XrPosefwith quaternion normalization._update_pose_space()updates transforms on node movement, re-parenting, or visibility changes.✔ Eye Visibility
Allows controlling which eye(s) the layer renders in via
eye_visibility:OpenXR refs: XrCompositionLayerBaseHeader, XrSpace, XrEyeVisibility
✅ Summary