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 building with use_volk=yes on MacOS #93331

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jun 18, 2024

Without the change in this PR, running scons platform=macos arch=arm64 dev_build=yes use_volk=yes I get these errors:

platform/macos/rendering_context_driver_vulkan_macos.mm:42:9: error: use of undeclared identifier 'VK_EXT_METAL_SURFACE_EXTENSION_NAME'
        return VK_EXT_METAL_SURFACE_EXTENSION_NAME;
               ^
platform/macos/rendering_context_driver_vulkan_macos.mm:48:2: error: unknown type name 'VkMetalSurfaceCreateInfoEXT'; did you mean 'VkHeadlessSurfaceCreateInfoEXT'?
        VkMetalSurfaceCreateInfoEXT create_info = {};
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~
        VkHeadlessSurfaceCreateInfoEXT
thirdparty/vulkan/include/vulkan/vulkan_core.h:14897:3: note: 'VkHeadlessSurfaceCreateInfoEXT' declared here
} VkHeadlessSurfaceCreateInfoEXT;
  ^
platform/macos/rendering_context_driver_vulkan_macos.mm:50:14: error: no member named 'pLayer' in 'VkHeadlessSurfaceCreateInfoEXT'
        create_info.pLayer = *wpd->layer_ptr;
        ~~~~~~~~~~~ ^
platform/macos/rendering_context_driver_vulkan_macos.mm:53:17: error: use of undeclared identifier 'vkCreateMetalSurfaceEXT'; did you mean 'vkCreateHeadlessSurfaceEXT'?
        VkResult err = vkCreateMetalSurfaceEXT(instance_get(), &create_info, nullptr, &vk_surface);
                       ^~~~~~~~~~~~~~~~~~~~~~~
                       vkCreateHeadlessSurfaceEXT
thirdparty/volk/volk.h:1350:39: note: 'vkCreateHeadlessSurfaceEXT' declared here
extern PFN_vkCreateHeadlessSurfaceEXT vkCreateHeadlessSurfaceEXT;

With the changes in this PR, compilation succeeds, and loading MoltenVK via Volk seems to work in my testing!

However, I'm not terribly knowledgeable of either MacOS or Vulkan, so I'm not sure if this is the correct fix, or I've got some issue on my system.

@dsnopek dsnopek added this to the 4.3 milestone Jun 18, 2024
@dsnopek dsnopek requested review from akien-mga and bruvzg June 18, 2024 22:35
@dsnopek dsnopek requested a review from a team as a code owner June 18, 2024 22:35
Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

Have not tested it, but it probably can be set without if env["use_volk"]:, and for both iOS and macOS.

@dsnopek dsnopek force-pushed the macos-fix-use-volk branch from 9c20199 to 18eb78b Compare June 19, 2024 11:03
@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 19, 2024

Ah, I haven't tested building for iOS, but looking at rendering_context_driver_vulkan_ios.mm, I think you're right that this is needed for iOS as well.

In my latest push, the define is set without if env["use_volk"] for both platforms.

EDIT: I just tested building for iOS with scons p=ios target=template_debug use_volk=yes and it built fine! I didn't try actually running it, though.

@dsnopek dsnopek force-pushed the macos-fix-use-volk branch from 18eb78b to e018eab Compare June 19, 2024 13:06
@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 19, 2024

I added some other minor changes that appear to be necessary to actually run Godot normally when loading MoltenVK via Volk - there's a flag and extra extension needed.

(It was working in my testing before because I was running a test project using the Meta XR Simulator, which apparently doesn't care about this extension and flag. However, when running Godot without using the Meta XR Simulator, these extra changes are needed.)

Some docs about the flag and extension:

image

From https://docs.vulkan.org/tutorial/latest/03_Drawing_a_triangle/00_Setup/01_Instance.html#_encountered_vk_error_incompatible_driver

I'm not sure why this isn't needed when not loading via Volk, though.

@dsnopek dsnopek added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Jun 24, 2024
@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 25, 2024

I'm not sure why this isn't needed when not loading via Volk, though.

Looking into this a little more, I think I now understand why that extension isn't needed when MoltenVK is statically linked. From the MoltenVK docs:

Selection_170

That extension is only used by the Vulkan loader, and when MoltenVK is statically linked, there's no proper Vulkan loader involved.

However, since this PR is about getting Godot working with Volk (a Vulkan loader!), it is needed in this case. In fact, if we attempt to use that extension when not using Volk (ie when statically linking MoltenVK), then vkCreateInstance() will fail due to that extension not being supported.

So, I'm a little bit more confident about that change in this PR!

@akien-mga akien-mga merged commit 643da5d into godotengine:master Jul 4, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dsnopek dsnopek deleted the macos-fix-use-volk branch July 22, 2024 15:34
@akien-mga akien-mga removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:macos topic:buildsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants