search

GodotVR / godot_openxr_vendors

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

Add Meta scene capture api to the Godot OpenXR loader plugin #40

Closed m4gr3d closed 10 months ago

m4gr3d commented 1 year ago

Follow up to my comment in https://github.com/godotengine/godot/pull/68259#discussion_r1266398110 about moving the anchor api options to the Godot OpenXR loader plugin.

This is set as a draft for now because the OpenXR Extension Wrapper needs to be added to the Godot OpenXR loader plugin to actually provide the functionality.

cc @konczg

konczg commented 1 year ago

@m4gr3d Seems good! Actually it's the exact same changes that I made locally when I realized that you already created the PR :)

m4gr3d commented 1 year ago

@m4gr3d Seems good! Actually it's the exact same changes that I made locally when I realized that you already created the PR :)

@konczg Per your comment, can I close this PR since you've made the changes locally?

konczg commented 1 year ago

@m4gr3d Sorry, by "locally" i meant that I've implemented the exact same changes, before seeing that you have already made them :) So this PR should still be merged

m4gr3d commented 11 months ago

@m4gr3d Sorry, by "locally" i meant that I've implemented the exact same changes, before seeing that you have already made them :) So this PR should still be merged

Sounds good! I need to rebase the PR, then I'll merge it for the 2.0 update.

@konczg @kisg On that topic, are you okay with moving the anchor / scene APIs gdextension implementation to this repo. I plan to add additional features to these APIs and it'll be simpler if the changes were made in a single repo.

BastiaanOlij commented 11 months ago

Sorry guys, missed this discussion for some reason.

I'm on the fence here. I think it doesn't make sense to include options here for features that aren't explicitly supported in this plugin or in the core. We could easily end up misleading users who download the plugin, see the option and think they can enable support for this not knowing that another plugin is needed.

To me it makes more sense that these options are exposed through the plugin that implements the feature. Unless there is a technical limitation here that does not make this possible.

That said, the ideal situation as Fredia already eludes to in his last comment, is to have the scene API logic embedded in this plugin. However this is up to Migeran whether they are ok with donating their logic to this project. I don't know their standpoint on whether they purely implemented the logic in a plugin because we can't (currently) be accepted it in core, or whether they also want to retain ownership of the implementation.

kisg commented 11 months ago

@m4gr3d @BastiaanOlij

We (Migeran company leadership) discussed it internally. The Migeran company can agree to donating the Scene Capture API implementation to Godot, provided that the contribution is attributed properly to Migeran - Software Development & Consulting (https://migeran.com) as Migeran designed and implemented the feature - and paid the costs of development.

I can send a PR for this.

However, it might make sense to rename this repository to either e.g. godot_openxr_device_support or to create separate repositories for each headset brand, (e.g. godot_openxr_meta) that would host the device support code for the specific brand. (OpenXR loader, device specific OpenXR extensions ... etc.)

What do you think?

m4gr3d commented 11 months ago

@m4gr3d @BastiaanOlij

We (Migeran company leadership) discussed it internally. The Migeran company can agree to donating the Scene Capture API implementation to Godot, provided that the contribution is attributed properly to Migeran - Software Development & Consulting (https://migeran.com) as Migeran designed and implemented the feature - and paid the costs of development.

I can send a PR for this.

Thanks @kisg, we appreciate the donation! There are several examples of attributions based on code donation (including from W4), so that should definitely be possible.

However, it might make sense to rename this repository to either e.g. godot_openxr_device_support or to create separate repositories for each headset brand, (e.g. godot_openxr_meta) that would host the device support code for the specific brand. (OpenXR loader, device specific OpenXR extensions ... etc.)

What do you think?

This is a topic we've discussed a few times especially as the scope of the repo and plugin grows beyond just providing loader capabilities, and something I'm in favor of. I think it makes sense to rename the plugin following the inclusion of the Scene API extension as it'll be the first vendor extension offered and supported by this plugin.

With regard to creating a separate repo for each headset branch, this is not on the table at the moment due to the overhead if would create for what would mostly be identical work for each vendor, but were a vendor to step forward and device they want to handle support for their headset, we would not be opposed to let them fork from this repo and point users to their 'official' plugin.

m4gr3d commented 10 months ago

Superseded by https://github.com/GodotVR/godot_openxr_loaders/pull/53