-
Notifications
You must be signed in to change notification settings - Fork 79
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
Added initial WebXR entry UI and export target. #312
Conversation
@dsnopek Could you review this PR. I played with it on my Quest 2 and it seems to work fine. |
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.
lgtm, but I think @dsnopek should review too :)
I attempted to test this really quick but was getting an error about first_person_controller_vr.tscn being missing? But I don't think that has anything to do with this PR. Just from skimming the code, this looks great! |
This is because we have a dependency on the godot_openxr library for that scene. @BastiaanOlij can you think of any way of removing this dependency when it's a WebXR only situation? |
Or maybe xr-tools should have it's own copy of the first person controller scene? This would come up with any other XR API as well. |
Owh yeah, using the For a multi platform setup one could even argue that you don't want to go this way. There is nothing stopping us from just adding a normal
TBH at this point in time, I don't even mind that being part of the plugin |
There are a few things the first_person_controller_vr.tscn from godot_openxr includes:
I'm guessing the "configuration" node might be necessary, but it also can't be present in the scene if it's a WebXR only version and godot_openxr hasn't been placed in the assets folder. I assume what triggers Godot to actually use godot_openxr is the presence of the "godot_openxr.gdnlib" with the "singleton" option enabled. |
ce0eb84
to
2753d3e
Compare
@dsnopek Could you try this again. |
Works perfectly in my testing! Skimming the diff, the rework seems great to me too :-) |
I'm really liking this, I am wondering if we shouldn't go a little further and just add the logic into our staging scene in the plugin itself. The reason I didn't add the ARVROrigin node in there was because it would be possible to use it with different plugins, but seeing OpenXR and WebXR are really the only ones we'll likely use and we now have most of the logic in the plugin already anyway, might as well just put the whole thing in staging and make it really easy to use it. |
Are there any use-cases where projects would want to start XR but not use the staging infrastructure? We could move the StartXR and the ARVROrigin/camera/controllers into the plugin staging; so staging-based projects get it for free, but non-staging projects can drop in the StartXR node. |
Personally, I don't always use the staging infrastructure when prototyping something simple, or just testing an individual scene.
This sounds good! |
2753d3e
to
a789229
Compare
I've updated the PR with moving the StartXR node as well as the ARVROrigin/camera/controllers into the base staging scene. This makes creating a customized staging scene significantly easier - in fact the base one will work unless the developer wishes to customize the prompt_for_continue and pause/resume logic. The StartXR node is still available for projects which don't want to use staging. |
cb8f6be
to
9f0cc85
Compare
Merged with Godot 4 equivalent functionality. |
9f0cc85
to
7845630
Compare
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.
Just a few nitpicking things but I think this is a great change making it easier to start with a cross platform solution.
progress = 0.0 | ||
|
||
[node name="Scene" type="Spatial" parent="."] | ||
|
||
[node name="Tween" type="Tween" parent="."] | ||
|
||
[node name="ARVROrigin" type="ARVROrigin" parent="."] |
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 should rename the nodes to XROrigin
and XRCamera
, might make things more recognizable between 3 and 4? Or am I over thinking it?
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.
It has benefits for compatibility, but it may also be a source of confusion as the developers will still have to use the ARVRxxxx names in code. If we choose to do this then I would probably make it a separate PR as we'd want to sweep through other files and comments.
Added thread support to handle resource queue loading. Added new StartXR scene to manage starting the OpenXR or WebXR instance. Fixed loading_screen so it doesn't produce NANs when the headset is off for a while. Fixed minor gdlint warnings. Move ARVROrigin/camera/controllers into base staging scene. Move StartXR into base staging scene. Modified base staging scene to emit xr_started/xr_ended signals. Synchronized with Godot 4 equivalent functionality and events. Formatting fixes Synchronize more changes with Godot 4 version. Applied changes from code review feedback.
7845630
to
35f8b3b
Compare
This pull request adds WebXR support to the godot-xr-tools demo project.
Note - this turns on Thread support in the WebXR HTML5 export presets as the godot-xr-tools project is set up to use resource_queue to load resources in a worker thread.