-
-
Notifications
You must be signed in to change notification settings - Fork 23.9k
Vulkan: Update all components to Vulkan SDK 1.4.335.0 #114075
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
base: master
Are you sure you want to change the base?
Conversation
4f5375d to
e9e4199
Compare
| # GNU patch | ||
| *.orig | ||
| *.rej |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tired of having to manually delete .orig and .rej files generated by GNU patch (one had managed to slip through in this PR at first), so I'm gitignoring them. I don't expect it to conflict with legit file extensions we'd want to version.
VulkanMemoryAllocator not updated as it's not versioned together with the SDK, and it often requires more work. SPIRV-Reflect: Fix reflection code and remove Godot's SC parsing patch. Co-authored-by: Dario <[email protected]>
95c0785 to
dc5f635
Compare
|
SPIRV-Reflect now also updated to 1.4.335.0 with the upstream specialization constant support thanks to @DarioSamo! |
|
I added the VulkanMemoryAllocator update to 3.3.0 in the end, so we can test everything at once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally on several demo projects (Linux + NVIDIA), this causes reflection probe rendering to break on the 3D platformer demo. It appears as pure black and this error is printed every time a reflection probe is rendered:
ERROR: This render pipeline requires (4) bytes of push constant data, supplied: (16)
at: draw_list_set_push_constant (./servers/rendering/rendering_device.cpp:4951)
ERROR: The shader in this pipeline requires a push constant to be set before drawing, but it's not present.
at: draw_list_draw (./servers/rendering/rendering_device.cpp:4984)
This occurs on both Forward+ and Mobile. The issue happens with the first commit only (no VMA update), or when both commits are applied.
|
Added a fix by @DarioSamo that should solve the issues @Calinou documented. There might be more similar cases that will warrant extensive testing, so we decided to postpone this to 4.7. It should still be tested well (can be done now) and merged early in the dev phase, so we have time to be confident that it doesn't create regressions before 4.7 beta. |
Calinou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great now 🙂
I've also tested it on several other demos like the TPS demo, as well as baking lightmaps.
Needs to be tested on all platforms where we support Vulkan, but also D3D12 and Metal for SPIR-V.
Our last sync was almost 2 years ago so this is long overdue, and thus would be good to include in 4.6.
Updated:
TODO:
@DarioSamo Would you be able to take a look at doing the SPIRV-Reflect update as a PR/direct push to my branch?
Edit: Done, thanks Darío!
Could be done in a separate PR if that sounds reasonable, or in this PR if we risk having issues by sticking to an older version.
Edit: Done, we can remove it if it causes issues.
Edit: Done with macOS/iOS: Update MoltenVK to 1.4.1 for Vulkan SDK 1.4.335.0 godot-build-scripts#143.
Supersedes #107773.