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

Allow unbundling OpenXR (for Linux distros) #73443

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

akien-mga
Copy link
Member

Linux distros typically like to link dynamically against system libraries when packaging software, which applies to Godot packages too.

Like we do for other libraries which are usually available as system library, we add a builtin_openxr option which can be turned off to link to system libraries on Linux. Note that the system is a bit brittle, if someone tries to turn off this option on other platforms they'll likely get a build error - but the same applies to all other builtin_* options we have currently, one day I'll clean up all this.

I have an issue preventing me from finalizing this, which is that we actually use XrMatrix4x4f which is not part of the public API of OpenXR, but included in one of the private source headers. So to be able to building against system headers, we'd need to either use a different, public API, or copy the relevant bits of code (maybe port them to Godot's Projection matrix?).

@akien-mga akien-mga added this to the 4.0 milestone Feb 16, 2023
@akien-mga akien-mga requested a review from a team February 16, 2023 15:24
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 16, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Feb 16, 2023

Hm. We only appear to be using XrMatrix4x4f_CreateProcjectionFov() and then copying the result right into a Godot Projection.

Looking at the source for XrMatrix4x4f_CreateProjectionFov(), I don't think it'd be a big deal to copy that code into the openxr module?

Both OpenXRVulkanExtension and OpenXROpenGLExtension do the same thing - we could make our own utility function in OpenXRUtil for them both to call?

@dsnopek
Copy link
Contributor

dsnopek commented Feb 18, 2023

Actually, much of the math here is the same as Projection::set_frustum(real_t p_left, real_t p_right, real_t p_bottom, real_t p_top, real_t p_near, real_t p_far) already. Perhaps we could use that, or extend it to support the "far plane at infinity" branch from XrMatrix4x4f_CreateProjection()? Copying the 3rd party code into a new utility method in the openxr module would certainly be a faster and safer way (with regard to preventing regressions elsewhere) to implement it, though :-)

@akien-mga
Copy link
Member Author

akien-mga commented Mar 3, 2023

I had a quick go at copying the relevant code to our OpenXRUtil, it seems to work.

I think it would be best to port the code to rely on our own Projection::set_frustum as suggested, but I'll leave that for someone else more versed into matrix operations for rendering :P

(If one of you in the XR team want to work on it, feel free to amend my PR directly.)

@akien-mga akien-mga marked this pull request as ready for review March 3, 2023 08:41
@akien-mga akien-mga requested review from a team as code owners March 3, 2023 08:41
@BastiaanOlij
Copy link
Contributor

@akien-mga we may need to add it to our Projection class, our current implementations all focus on symmetrical frustums.
But it's not a bad addition. Poke me later this week and I'm happy to rework this

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
@akien-mga akien-mga marked this pull request as draft June 16, 2023 10:09
@akien-mga
Copy link
Member Author

Rebased the current approach.

@akien-mga we may need to add it to our Projection class, our current implementations all focus on symmetrical frustums. But it's not a bad addition. Poke me later this week and I'm happy to rework this

I think this would be ideal, but until it's done, I think we can go ahead with the approach I took bundling the relevant third-party code in our utils class.

@akien-mga akien-mga marked this pull request as ready for review June 16, 2023 10:15
Copy XrMatrix4x4f_CreateProjectionFov to our OpenXRUtil, instead of relying
on a private header.
@YuriSizov YuriSizov merged commit 661c395 into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 20, 2023
@akien-mga akien-mga deleted the unbundle-openxr branch October 12, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants