Skip to content
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 Meta scene capture api to the Godot OpenXR loader plugin #40

Closed
wants to merge 1 commit into from

Conversation

m4gr3d
Copy link
Collaborator

@m4gr3d m4gr3d commented Jul 19, 2023

Follow up to my comment in godotengine/godot#68259 (comment) 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

@m4gr3d m4gr3d added the enhancement New feature or request label Jul 19, 2023
@m4gr3d m4gr3d added this to the 2.0.0 milestone Jul 19, 2023
@m4gr3d m4gr3d requested review from kisg and BastiaanOlij July 19, 2023 06:40
@konczg
Copy link

konczg commented Jul 19, 2023

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

@m4gr3d
Copy link
Collaborator Author

m4gr3d commented Aug 18, 2023

@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
Copy link

konczg commented Aug 21, 2023

@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 m4gr3d marked this pull request as ready for review October 13, 2023 15:36
@m4gr3d
Copy link
Collaborator Author

m4gr3d commented Oct 13, 2023

@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
Copy link
Member

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
Copy link
Collaborator

kisg commented Oct 16, 2023

@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
Copy link
Collaborator Author

m4gr3d commented Oct 16, 2023

@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
Copy link
Collaborator Author

m4gr3d commented Oct 21, 2023

Superseded by #53

@m4gr3d m4gr3d closed this Oct 21, 2023
@m4gr3d m4gr3d deleted the add_anchor_options branch October 21, 2023 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants