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

fix for android surface acquiring #1868

Merged
merged 1 commit into from Apr 5, 2022
Merged

fix for android surface acquiring #1868

merged 1 commit into from Apr 5, 2022

Conversation

Dimkar3000
Copy link
Contributor

fixes issue#1417

  • Entries for Vulkano changelog:
    • Fix for android surface acquiring.
  1. Run cargo fmt on the changes.
  1. Make sure that the changes are covered by unit-tests.

  2. Update documentation to reflect any user-facing changes - in this repository.

use winit::platform::android::WindowExtAndroid;

Surface::from_android(instance, win.borrow().native_window(), win)
use raw_window_handle::HasRawWindowHandle;
Copy link
Contributor

Choose a reason for hiding this comment

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

raw_window_handle is behind a feature gate in Cargo.toml. It's enabled by default, but if the user decides to disable it and use only the winit feature, this code won't work anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, the reason I chose this solution is because the other one was silently deprecated. This seams to be the way, the developer intended for this to work. I don't know how to fix this properly, maybe require the feature for android?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since they suggested always using raw_window_handle (on any platform), I think removing optional = true from the dependency and having it always there makes the most sense. Then the code in winit.rs can use this for all platforms instead of Winit's various custom traits like WindowExtUnix and WindowExtWindows. It's a bit more of a change so if you want to do it only for Android that's fine, I can do it for the other platforms later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont know why it is optional, maybe they have their reasons. On this end we could probably add the requirement in the README so everyone knows that the feature is required. I don't have access to all the platforms to properly test a change like that. I could do it for windows.

@Rua
Copy link
Contributor

Rua commented Apr 5, 2022

I've merged it, and will look at the things I mentioned myself sometime. Thank you for the fix!

@Rua Rua merged commit c794371 into vulkano-rs:master Apr 5, 2022
Rua added a commit that referenced this pull request Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile error in vulkano-win on Android
2 participants