search

godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
85.17k stars 18.71k forks source link

vulkan: Update all components to Vulkan SDK 1.3.183.0 #92010

Open akien-mga opened 2 weeks ago

akien-mga commented 2 weeks ago

Draft as I need help to rediff the SPIRV-Reflect specialization constant patch. CC @godotengine/rendering

You can help me by checking out this PR, trying to apply thirdparty/spirv-reflect/patches/specialization-constants.patch with patch -p3 < thirdparty/spirv-reflect/patches/specialization-constants.patch, and then figuring out how to resolve the hunks that couldn't be applied. Once the changes are good, you can do git diff > mydiff and copy that file on top of thirdparty/spirv-reflect/patches/specialization-constants.patch, to include in the commit.

It seems like upstream added a (more minimal?) spec constant struct with the same name, so we likely need to fold our changes into their new API:

Worth noting that there are two open PRs upstream to add the kind of spec constants support we have, which we could help drive to completion / merge:

RandomShaper commented 2 weeks ago

Re-done patch to spirv-reflect: specialization-constants.patch

akien-mga commented 2 weeks ago

Re-done patch to spirv-reflect: specialization-constants.patch

Thanks, added to the PR. Should be good to go then.

akien-mga commented 2 weeks ago

@bruvzg The macOS and iOS builds for glslang are failing with:

In file included from thirdparty/glslang/glslang/GenericCodeGen/Link.cpp:40:
In file included from thirdparty/glslang/glslang/GenericCodeGen/../Include/ShHandle.h:48:
thirdparty/glslang/glslang/GenericCodeGen/../Include/InfoSink.h:104:37In file included from thirdparty/glslang/glslang/GenericCodeGen/CodeGen.cpp:36:
In file included from thirdparty/glslang/glslang/GenericCodeGen/../Include/ShHandle.h:48:
thirdparty/glslang/glslang/GenericCodeGen/../Include/InfoSink.h:104:37: error: 'absolute' is unavailable: introduced in macOS 10.15
            append(std::filesystem::absolute(shaderFileName).string());
                                    ^
: error: 'absolute' is unavailable: introduced in macOS 10.15
/Applications/Xcode_15.0.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.0.sdk/usr/include/c++/v1/__filesystem/operations.h:65:35: note: 'absolute' has been explicitly marked unavailable here
inline _LIBCPP_HIDE_FROM_ABI path absolute(const path& __p) { return __absolute(__p); }
                                  ^
In file included from thirdparty/glslang/glslang/GenericCodeGen/Link.cpp:40:
In file included from thirdparty/glslang/glslang/GenericCodeGen/../Include/ShHandle.h:48:
thirdparty/glslang/glslang/GenericCodeGen/../Include/InfoSink.h:104:37: error: 'absolute' is unavailable: introduced in iOS 13.0 [2]
             append(std::filesystem::absolute(shaderFileName).string());
                                     ^

Sounds like using this glslang version would force us to bump our min macOS and iOS releases. Should we send a patch upstream to restore support for older versions?

Edit: Filed a bug report upstream:

bruvzg commented 2 weeks ago

Sounds like using this glslang version would force us to bump our min macOS and iOS releases. Should we send a patch upstream to restore support for older versions?

Vulkan (with a feature set used by Godot) won't properly work on macOS 10.14 and iOS 12, it's only for OpenGL backend. Both versions are unsupported for a long time (but current Xcode still can target both), so I'm not sure if there's much reason to keep supporting it, or bump the min versions.

akien-mga commented 2 weeks ago

Sounds like using this glslang version would force us to bump our min macOS and iOS releases. Should we send a patch upstream to restore support for older versions?

Vulkan (with a feature set used by Godot) won't properly work on macOS 10.14 and iOS 12, it's only for OpenGL backend. Both versions are unsupported for a long time (but current Xcode still can target both), so I'm not sure if there's much reason to keep supporting it, or bump the min versions.

It probably doesn't make sense to attempt using our Vulkan/Metal renderers on macOS 10.14 or iOS 12 indeed, but for OpenGL I think it may still make sense if we can preserve compatibility.

Judging by https://gs.statcounter.com/ios-version-market-share/ stats, iOS 12.5 (which seemed to be a LTS and only got EOL'ed in Jan 2023) still has around 1.5% market share among iOS users: image

For macOS, stats from the same website: https://gs.statcounter.com/macos-version-market-share/ image So we'd lose 2.5% of macOS users by dropping 10.13 and 10.14.

bruvzg commented 2 weeks ago

Seems like it only for output prints with the specific flag, so we can patch it locally by removing if (absolute) condition.

bruvzg commented 2 weeks ago

For macOS, stats from the same website

That's some strange data, seem to include only outdated and unsupported versions of macOS, currently only Monterey (12) and newer are getting security updates.

akien-mga commented 2 weeks ago

For macOS, stats from the same website

That's some strange data, seem to include only outdated and unsupported versions of macOS, currently only Monterey (12) and newer are getting security updates.

Yeah I think they simply didn't update their heuristics when macOS bumped from 10.15 to 11.0, so they seem to track all >= 10.15 into "Catalina". The data for older releases may still be somewhat reliable.

akien-mga commented 2 weeks ago

Getting a test failure which does sound related to this PR:


#################### RenderSceneBuffersRD ####################

RenderSceneBuffersRD.has_texture --- executing with 2 parameters [&"", &""]
GDSCRIPT CODE:     RenderSceneBuffersRD.new().has_texture(StringName(""), StringName(""))

RenderSceneBuffersRD.create_texture_from_format --- executing with 5 parameters [&"", &"", <RDTextureFormat#-9223371657488353317>, <RDTextureView#-9223371657471576100>, false]
GDSCRIPT CODE:     RenderSceneBuffersRD.new().create_texture_from_format(StringName(""), StringName(""), RDTextureFormat.new(), RDTextureView.new(), false)
godot.linuxbsd.editor.dev.double.x86_64.san: thirdparty/vulkan/vk_mem_alloc.h:3931: bool FindMemoryPreferences(bool, const VmaAllocationCreateInfo&, VmaBufferImageUsage, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&): Assertion `0 && "VMA_MEMORY_USAGE_AUTO* values can only be used with functions like vmaCreateBuffer, vmaCreateImage so that the details of the created resource are known." " Maybe you use VkBufferUsageFlags2CreateInfoKHR but forgot to use VMA_ALLOCATOR_CREATE_KHR_MAINTENANCE5_BIT?"' failed.
Aborted (core dumped)
akien-mga commented 2 days ago

I just rebased, and updated VMA to the newly tagged 3.1.0 release (no change compared to previous git snapshot in that PR). So I assume the CI will still fail similarly.

Then I'll try reverting VMA to the previous version (that we have in the master branch) to see if that's indeed what causes our issue.

akien-mga commented 2 days ago

So reverting VMA to an older commit fixed it, i.e. this commit (which I'll now remove): https://github.com/godotengine/godot/pull/92010/commits/2b466bd5b1eb50de699f0d75b38f325a940281f4

The error on CI is:

godot.linuxbsd.editor.dev.double.x86_64.san: thirdparty/vulkan/vk_mem_alloc.h:3931: bool FindMemoryPreferences(bool, const VmaAllocationCreateInfo&, VmaBufferImageUsage, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&): Assertion `0 && "VMA_MEMORY_USAGE_AUTO* values can only be used with functions like vmaCreateBuffer, vmaCreateImage so that the details of the created resource are known." " Maybe you use VkBufferUsageFlags2CreateInfoKHR but forgot to use VMA_ALLOCATOR_CREATE_KHR_MAINTENANCE5_BIT?"' failed.

And that's consistent with the fact that the VMA update mostly adds support for KHR_MAINTENANCE5.

I don't know much about that, but maybe @DarioSamo or @RandomShaper could have a look at what we're missing?

RandomShaper commented 1 day ago

This patch may do the trick:

diff --git a/drivers/vulkan/rendering_device_driver_vulkan.cpp b/drivers/vulkan/rendering_device_driver_vulkan.cpp
index 131e0e4a8a..d38eb5a88d 100644
--- a/drivers/vulkan/rendering_device_driver_vulkan.cpp
+++ b/drivers/vulkan/rendering_device_driver_vulkan.cpp
@@ -942,6 +942,10 @@ Error RenderingDeviceDriverVulkan::_initialize_allocator() {
    allocator_info.physicalDevice = physical_device;
    allocator_info.device = vk_device;
    allocator_info.instance = context_driver->instance_get();
+   const bool use_1_3_features = physical_device_properties.apiVersion >= VK_API_VERSION_1_3;
+   if (use_1_3_features) {
+       allocator_info.flags |= VMA_ALLOCATOR_CREATE_KHR_MAINTENANCE5_BIT;
+   }
    VkResult err = vmaCreateAllocator(&allocator_info, &allocator);
    ERR_FAIL_COND_V_MSG(err, ERR_CANT_CREATE, "vmaCreateAllocator failed with error " + itos(err) + ".");

Explanation: with Vulkan >= 1.3, KHR_MAINTENANCE5 extension is implicitly enabled. VMA needs to know if that's the case so it uses the right code path in the algo to find the right memory type.

Not tested, so, well, TIWAGOS.

PS: There are other VMA creation flags related to extensions that we may want to set in case those are either implicitly enabled or explicitly by us in case they're available so maybe VMA can do a better job somehow.

akien-mga commented 1 day ago

I gave it a try in the latest commit (7479f84), but that doesn't seem to solve it, it still fails at the same API test:

RenderSceneBuffersRD.create_texture_from_format --- executing with 5 parameters [&"", &"", <RDTextureFormat#-9223371657404467237>, <RDTextureView#-9223371657387690020>, false]
GDSCRIPT CODE:     RenderSceneBuffersRD.new().create_texture_from_format(StringName(""), StringName(""), RDTextureFormat.new(), RDTextureView.new(), false)
godot.linuxbsd.editor.dev.double.x86_64.san: thirdparty/vulkan/vk_mem_alloc.h:3931: bool FindMemoryPreferences(bool, const VmaAllocationCreateInfo&, VmaBufferImageUsage, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&, VkMemoryPropertyFlags&): Assertion `0 && "VMA_MEMORY_USAGE_AUTO* values can only be used with functions like vmaCreateBuffer, vmaCreateImage so that the details of the created resource are known." " Maybe you use VkBufferUsageFlags2CreateInfoKHR but forgot to use VMA_ALLOCATOR_CREATE_KHR_MAINTENANCE5_BIT?"' failed.
RandomShaper commented 1 day ago

I can't reproduce on Windows. However, I've found that this is something else we'd want to do when initializing allocator_info: allocator_info.vulkanApiVersion = physical_device_properties.apiVersion;

It stops VMA from assuming we're using Vulkan 1.0, and so lets it collect pointers to certain functions that make some operations more efficient.

Can you test it and see if it interacts with the issue at hand?

akien-mga commented 1 day ago

Tried it, that doesn't seem to fix it either.

I managed to reproduce the issue locally on Linux with a regular dev build from this branch, running the command from the test project in a new project:

RenderSceneBuffersRD.new().create_texture_from_format(StringName(""), StringName(""), RDTextureFormat.new(), RDTextureView.new(), false)

It's not showing me the assert message for some reason but it crashes all the same:

(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00000000073e3052 in vmaFindMemoryTypeIndexForImageInfo (allocator=0xbc830e0, pImageCreateInfo=0x7fffffff3c40, pAllocationCreateInfo=0x7fffffff3c00, pMemoryTypeIndex=0x7fffffff3b54)
    at thirdparty/vulkan/vk_mem_alloc.h:15223
#2  0x00000000073f7caa in RenderingDeviceDriverVulkan::texture_create (this=0xbc37df0, p_format=..., p_view=...) at drivers/vulkan/rendering_device_driver_vulkan.cpp:1464
#3  0x0000000009cd0e78 in RenderingDevice::texture_create (this=0xbc37110, p_format=..., p_view=..., p_data=...) at ./servers/rendering/rendering_device.cpp:778
#4  0x0000000009ffe5d8 in RendererRD::TextureStorage::TextureStorage (this=0xb345730) at ./servers/rendering/renderer_rd/storage_rd/texture_storage.cpp:114
#5  0x0000000009ec9f79 in RendererCompositorRD::RendererCompositorRD (this=0xbf551f0) at ./servers/rendering/renderer_rd/renderer_compositor_rd.cpp:305
#6  0x0000000005b14cdc in RendererCompositorRD::_create_current () at ./servers/rendering/renderer_rd/renderer_compositor_rd.h:138
#7  0x0000000009cacf11 in RendererCompositor::create () at ./servers/rendering/renderer_compositor.cpp:42
#8  0x0000000009d2acbd in RenderingServerDefault::_init (this=0xb347750) at ./servers/rendering/rendering_server_default.cpp:220
#9  0x0000000009d2b0f2 in RenderingServerDefault::init (this=0xb347750) at ./servers/rendering/rendering_server_default.cpp:259
#10 0x0000000005b8e872 in Main::setup2 () at main/main.cpp:2790
#11 0x0000000005b8ca8b in Main::setup (execpath=0x7fffffffdba9 "/home/akien/.local/bin/godot-git", argc=1, argv=0x7fffffffd730, p_second_phase=true) at main/main.cpp:2432
#12 0x0000000005ada225 in main (argc=2, argv=0x7fffffffd728) at platform/linuxbsd/godot_linuxbsd.cpp:74
RandomShaper commented 20 hours ago

That's been useful! The problem is that piece of script is trying to create a texture with not a single usage bit specified. See https://github.com/godotengine/godot/pull/92587.

akien-mga commented 18 hours ago

That did the trick! Should I keep the changes we made to allocator_info, or revert them for now?

RandomShaper commented 15 hours ago

That did the trick! Should I keep the changes we made to allocator_info, or revert them for now?

Good question! The one in the patch is needed as part of the update. However, the other line of code is not strictly part of the scope of this PR. It may be better to keep it as a separate commit. Besides, it's good to have the ability to frame that other change in a bisect.