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 game of life example on macos #1829

Merged
merged 4 commits into from Mar 28, 2022
Merged

Fix game of life example on macos #1829

merged 4 commits into from Mar 28, 2022

Conversation

hakolao
Copy link
Contributor

@hakolao hakolao commented Feb 17, 2022

  1. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

  2. Run cargo fmt on the changes.

I noticed that on my macos the create_vk_surface_from_handle encounters an error:

[mvk-error] VK_ERROR_INITIALIZATION_FAILED: vkCreateMacOSSurfaceMVK(): On-screen rendering requires a layer of type CAMetalLayer.

What is done in winit_to_surface prevents this. I wasn't even sure why the handle version was used anyway...

@Rua
Copy link
Contributor

Rua commented Feb 19, 2022

I'm not on MacOS so I can't test this, but it seems like a simple enough fix. Can you explain why this fixes it though, and is it maybe something that should be documented in these two functions?

@Eliah-Lakhin
Copy link
Contributor

@hakolao Can you address @Rua's question, please?

@hakolao
Copy link
Contributor Author

hakolao commented Mar 28, 2022

Apologies for the delay! @Eliah-Lakhin @Rua

CAMetalLayer is the native rendering surface of Apple’s Metal framework. It is required to be able to render on mac. It looks like that the winit_to_surface function ensures this layer is set to the ns_view before passing it to Surface::from_mac_os.

Without that, the direct create_surface_from_handle fails to above error.

Some references: KhronosGroup/MoltenVK#78
https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/VK_EXT_metal_surface.html

This could be documented somewhere, however I'd think that having it searchable on github might also be enough, and those who use or should use create_surface_from_handle are probably very few. I thought I needed it, but turned out I did not...

@Rua
Copy link
Contributor

Rua commented Mar 28, 2022

I think this should be included as documentation on create_surface_from_handle so that users know they may need to do this manually. It's not very user friendly to ask them to search on Github.

@hakolao
Copy link
Contributor Author

hakolao commented Mar 28, 2022

Ok, I’ll add it there!

@hakolao
Copy link
Contributor Author

hakolao commented Mar 28, 2022

@Rua would this be better now? I extracted the layer setting to its own function, so if someone wants to use the from_handle they can then call that function also on macos. And added a note to the docs.

/// which implements HasRawWindowHandle and thus can reveal the os-dependent handle
/// which implements HasRawWindowHandle and thus can reveal the os-dependent handle.
/// - Note that if you wish to use this function with MacOS, you will need to ensure that the
/// `CAMetalLayer` is set to the ns_view. You can do that by calling `vulkano_win::set_ca_metal_layer_to_winit`
Copy link
Contributor

Choose a reason for hiding this comment

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

create_surface_from_handle is not used with Winit, and it doesn't make sense to tell the user to use a function that requires Winit, if they aren't using Winit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. I updated once more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although... One could if they want to retain the ownership of the window else where? Though that's probably not necessary...

@Rua Rua merged commit d1ab3f6 into vulkano-rs:master Mar 28, 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.

None yet

3 participants