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 support for XR_FB_android_surface_swapchain_create #208

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Sep 20, 2024

This adds support for XR_FB_android_surface_swapchain_create which gives two new flags that can be used to customize the creation of Android surface swapchains.

It depends on Godot PR godotengine/godot#97252 (so setting as a draft for now)

That means that this will require Godot 4.4, which is why it is also updating the extension_api.json and godot-cpp. After this is merged, then the master branch will only work with Godot 4.4 (relating to our discussion from the last XR team meeting).

@dsnopek dsnopek added the enhancement New feature or request label Sep 20, 2024
@dsnopek dsnopek added this to the 4.x milestone Sep 20, 2024
@dsnopek dsnopek marked this pull request as draft September 20, 2024 21:38
@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch 2 times, most recently from 74d2333 to bfeacca Compare September 20, 2024 22:30
@dsnopek dsnopek marked this pull request as ready for review September 23, 2024 14:07
@dsnopek
Copy link
Collaborator Author

dsnopek commented Sep 23, 2024

Taking this out of draft now that the Godot PR has been merged. However, this actually won't pass tests on the XR Simulator until after 4.4-dev3 is released and I change the CI to point to that, so I guess this will still need to wait a bit before we can merge it.

Copy link
Collaborator

@devloglogan devloglogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Minus my one little nit.

@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch from bfeacca to 2166ac9 Compare September 25, 2024 11:09
@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch from 2166ac9 to 027116e Compare October 2, 2024 13:36
@dsnopek
Copy link
Collaborator Author

dsnopek commented Oct 29, 2024

FYI, this one is failing testing because of a bug in Godot 4.4-dev3 - see godotengine/godot#98102

It was fixed in PR godotengine/godot#98187 which is already merged and should be included in Godot 4.4-dev4.

I'll update the CI to use 4.4-dev4 once it's released!

@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch 2 times, most recently from 858ac5f to 83687e5 Compare November 25, 2024 21:42
@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch 2 times, most recently from 0177dfc to fc645f6 Compare December 9, 2024 17:34
@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 9, 2024

I've been trying to figure out why the XR Simulator tests on CI are crashing - here's some notes on it so far:

  • It appears to only be the composition layer sample that is currently crashing (even without the changes from this PR - using the latest stable release from the asset library crashes)
  • The latest Godot dev release (dev6) is crashing too
  • The last working dev release was dev1
  • Dev3 crashed due to a Vulkan issue that is fixed
  • The crash in dev2 doesn't look exactly like dev4 through dev6: it doesn't give a backtrace, the window just immediately closes. But the issue could potentially go back as far as dev2
  • I tried to grab a recent build from GitHub Actions and it didn't crash, although, I'm not 100% confident it was from master and not a PR that hadn't been rebased or something like that. I need to try latest master for real, ideally, with a dev build so that I can get more information from the backtrace

Anyway, it doesn't seem like the CI issues really have anything to do with this PR, but do seem to represent an issue with upstream Godot and the XR Simulator

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 10, 2024

Unfortunately, it looks like a custom compiled master and 4.4-dev6 work fine. It's only the official build of 4.4-dev6 that is crashing... :-(

UPDATE: It appears that builds with dev_build=yes work, and those with dev_build=no don't. And, even with debug_symbols=yes, I'm not getting a nice backtrace (it shows some frames but with no useful info like function names), but I suspect that's related to current Godot issues with backtraces on Windows

@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 16, 2024

Once PR godotengine/godot#100471 is merged, and there's a new 4.4-dev release, and the CI is updated to use it - then this should finally pass CI :-)

@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch 3 times, most recently from 9fcff44 to 74b20d7 Compare December 20, 2024 17:05
@dsnopek
Copy link
Collaborator Author

dsnopek commented Dec 20, 2024

This is FINALLY passing tests!

It already has approvals (based on the non-CI code) so I suppose I could merge it. :-) However, this will mean the master branch will become Godot 4.4+ (whereas right now it's 4.3+). Does anyone object to this happening now? (It would happen inevitably at some point)

@dsnopek dsnopek force-pushed the fb-android-surface-swapchain-create branch from 74b20d7 to 183a512 Compare January 21, 2025 15:58
@dsnopek dsnopek merged commit ebb81f4 into GodotVR:master Jan 21, 2025
10 checks passed
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.

3 participants