GodotVR / godot_openxr_vendors

Godot 4 wrapper for OpenXR vendors loaders and extensions
MIT License
90 stars 19 forks source link

Add OpenXR fb_render_model extension wrapper and OpenXRFBRenderModel node #76

Closed devloglogan closed 6 months ago

devloglogan commented 7 months ago

Adds an extension wrapper for the OpenXR XR_FB_render_model extension and corresponding node implementing its usage. This extension provides access to gltf models of controllers/keyboards at runtime.

The extension wrapper caches a list of valid paths to these models (accessible with function get_valid_paths() in the OpenXRFBRenderModel node or the same function in the extension wrapper singleton). One of these paths can be assigned to the render_model_path property of the OpenXRFBRenderModel node, which will trigger that model to be loaded and accessible via the nodes get_render_model() function. The only other time the model is loaded is on startup via the OpenXrInterface session_begun signal.

At present (tested on Quest 2 and Quest 3), it seems only the controller models are able to be successfully loaded. The virtual keyboard requires the XR_META_virtual_keyboard extension to be enabled, but I believe the other two should be available. Unsure if there's another dependency missing for these. The controller models have been added to the demo main scene.

BastiaanOlij commented 7 months ago

Looks really good at an early glance.

I'm concerned about the requirement of the rotation of the grip pose to position the render model, that should match up or the spec should provide information about correct placement. If it doesn't that's going to be bad and bite us down the road if other vendors follow suit but do properly position the meshes.

On that subject, exposing the render models to the user should take a more global approach and not be FB centric. Here I'm unfortunately not at liberty to provide more detail yet, but we're going to back ourselves into a corner if we do not take a wider view.

devloglogan commented 7 months ago

I'm concerned about the requirement of the rotation of the grip pose to position the render model

To be clear, the render model isn't being repositioned by anything other than the setting of the grip pose. The hacky rotation is only there for repositioning the hand tablet correctly from one pose to the other. Would this not be expected? We could just set the pose for the controllers to be grip by default and reposition the hand tablet in the editor instead.

dsnopek commented 7 months ago

We could just set the pose for the controllers to be grip by default and reposition the hand tablet in the editor instead.

Yes, I think we should do exactly this!

devloglogan commented 7 months ago

Alright, I changed the default controller poses to grip and added some (hard coded) render_model_path suggestions

dsnopek commented 7 months ago

@BastiaanOlij:

On that subject, exposing the render models to the user should take a more global approach and not be FB centric. Here I'm unfortunately not at liberty to provide more detail yet, but we're going to back ourselves into a corner if we do not take a wider view.

If a vendor neutral OpenXR extension is added to the spec, we'd implement that in Godot itself. So, personally, I don't think having the Meta-specific one here in 'godot_openxr_vendors' paints us into any corners. Folks not specifically targeting Meta headsets won't use their vendor extensions (they'll wait for the vendor neutral one), and Meta's extension will keep working on Meta devices even after the vendor neutral one exists.

devloglogan commented 7 months ago

Updated based on the reparenting / model path comments, created an issue for the keyboard models.

BastiaanOlij commented 7 months ago

Just an intermediate update from me as I'm waiting on hearing back before we can make an informed decision on how to proceed here. Until I can share updates, best to press pause, hope to have info in the next couple days.

BastiaanOlij commented 7 months ago

If a vendor neutral OpenXR extension is added to the spec, we'd implement that in Godot itself. So, personally, I don't think having the Meta-specific one here in 'godot_openxr_vendors' paints us into any corners. Folks not specifically targeting Meta headsets won't use their vendor extensions (they'll wait for the vendor neutral one), and Meta's extension will keep working on Meta devices even after the vendor neutral one exists.

It's not that simple. The following scenario is fairly likely:

So we have a mandate to at least discuss whether we can come up with a solution that staves off this scenario (which is very likely due to reasons I'm not at liberty to share), or whether we should forge ahead and deal with the fallout later.

devloglogan commented 6 months ago

@BastiaanOlij rebased!

m4gr3d commented 6 months ago

@BastiaanOlij @dsnopek From our discussions, it's likely we'll move forward with this approach as a v0 api. Can you review in that context.

@devloglogan Can you rebase the PR for review.

devloglogan commented 6 months ago

Rebased! Per Fredia's request I've swapped the model selector on the OpenXRFbRenderModel node to be an enum type. After doing that, it seemed pointless storing path names and exposing them, so I removed that functionality.

devloglogan commented 6 months ago

@m4gr3d requests added!

m4gr3d commented 6 months ago

One thing we keep forgetting to do, can you update the CHANGES.md doc and reference the new feature.

m4gr3d commented 6 months ago

@BastiaanOlij Looks like all your feedback on the PR have been addressed. Are you good with merging the current implementation?

BastiaanOlij commented 6 months ago

@m4gr3d I haven't gotten anywhere near actually testing the latest version, as I was discussing with David the other day I am getting fairly concerned with this, especially as the testing I did get around doing last week show that a few things have snuck in that are causing issues and right now the plugin is in a broken state for me.

Purely based on the code and the discussion around this however, I am happy. But I would like to actually take it for a drive before giving my final thumbs up.

m4gr3d commented 6 months ago

@devloglogan Can you rebase the PR so we can proceed to merge it!

devloglogan commented 6 months ago

@m4gr3d rebased