GH-674 Check if Vulkan is supported before attempting to Initialize#1205
Open
JoltedJon wants to merge 3 commits intoRedot-Engine:masterfrom
Open
GH-674 Check if Vulkan is supported before attempting to Initialize#1205JoltedJon wants to merge 3 commits intoRedot-Engine:masterfrom
JoltedJon wants to merge 3 commits intoRedot-Engine:masterfrom
Conversation
WalkthroughAdds a Vulkan support probe via Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/vulkan/rendering_context_driver_vulkan.cpp`:
- Around line 918-953: The support probe in _vulkan_is_supported() currently
calls raw Vulkan entry points (volkInitialize, vkCreateInstance,
vkEnumeratePhysicalDevices) which bypasses the hook abstraction and can return
false when hook-backed setups are available; change the probe to consult and use
the VulkanHooks abstraction used by _create_vulkan_instance() and
_initialize_devices() (e.g., check VulkanHooks::is_available() or the equivalent
hook initializer and use its proc getters instead of raw vk* symbols) so the
probe respects hooked entry points, and if hooks report availability follow the
same creation/enumeration path the rest of the code uses (and only fall back to
raw entry points if VulkanHooks explicitly reports unavailable).
- Around line 933-957: The probe in _vulkan_is_supported creates an instance
(vkCreateInstance using create_info and app_info) but doesn't enable portability
enumeration, causing vkEnumeratePhysicalDevices to fail on MoltenVK; update the
probe's VkInstanceCreateInfo to add the
VK_INSTANCE_CREATE_ENUMERATE_PORTABILITY_BIT_KHR flag and add the
"VK_KHR_portability_enumeration" extension to
create_info.ppEnabledExtensionNames (ensure create_info.enabledExtensionCount is
set accordingly) before calling vkCreateInstance so vkEnumeratePhysicalDevices
can return devices on portability implementations.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
drivers/vulkan/rendering_context_driver_vulkan.cppdrivers/vulkan/rendering_context_driver_vulkan.h
… linux uses function pointers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drivers/vulkan/rendering_context_driver_vulkan.cpp`:
- Around line 33-34: The `#include` "core/error/error_list.h" is outside the
VULKAN_ENABLED guard and should be moved so it's only included for Vulkan
builds; relocate that include into the existing `#ifdef` VULKAN_ENABLED block with
the other includes in rendering_context_driver_vulkan.cpp so ERR_UNAVAILABLE
(used around the ERR_UNAVAILABLE reference in the Vulkan-specific code) is only
available in Vulkan builds and non-Vulkan builds don't pull the header.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
drivers/vulkan/rendering_context_driver_vulkan.cpp (1)
924-972: Add lightweight probe failure diagnostics.The probe currently returns
falsesilently on multiple branches, which makes support/debug triage harder wheninitialize()returnsERR_UNAVAILABLE. A small verbose log per failure branch would help without changing behavior.🛠️ Suggested minimal logging additions
`#ifdef` USE_VOLK if (volkInitialize() != VK_SUCCESS) { + print_verbose("VULKAN: Support probe failed at volkInitialize."); return false; } if (!vkCreateInstance) { + print_verbose("VULKAN: Support probe missing vkCreateInstance."); return false; } `#endif` @@ VkInstance _instance; if (vkCreateInstance(&create_info, nullptr, &_instance) != VK_SUCCESS) { + print_verbose("VULKAN: Support probe failed at vkCreateInstance."); return false; } @@ `#ifdef` USE_VOLK if (!vkEnumeratePhysicalDevices) { + print_verbose("VULKAN: Support probe missing vkEnumeratePhysicalDevices."); vkDestroyInstance(_instance, nullptr); return false; } `#endif` @@ vkDestroyInstance(_instance, nullptr); if (result != VK_SUCCESS || device_count == 0) { + print_verbose("VULKAN: Support probe found no usable Vulkan physical devices."); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/vulkan/rendering_context_driver_vulkan.cpp` around lines 924 - 972, The probe currently returns false silently; add lightweight diagnostic logging at each failure branch without changing behavior: log a clear message when volkInitialize() fails, when vkCreateInstance function pointer is null, when vkCreateInstance(&create_info, nullptr, &_instance) returns non-VK_SUCCESS (include the VkResult), when vkLoadInstance/vkEnumeratePhysicalDevices function pointer is null, and when vkEnumeratePhysicalDevices(_instance, &device_count, nullptr) returns non-VK_SUCCESS or when device_count == 0 (include result and device_count); reference the symbols volkInitialize, vkCreateInstance, vkCreateInstance(&create_info,...,&_instance), volkLoadInstance, vkEnumeratePhysicalDevices, _instance, result, and device_count, and ensure any existing vkDestroyInstance(_instance, nullptr) calls remain in place and logs are emitted before each return false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@drivers/vulkan/rendering_context_driver_vulkan.cpp`:
- Around line 924-972: The probe currently returns false silently; add
lightweight diagnostic logging at each failure branch without changing behavior:
log a clear message when volkInitialize() fails, when vkCreateInstance function
pointer is null, when vkCreateInstance(&create_info, nullptr, &_instance)
returns non-VK_SUCCESS (include the VkResult), when
vkLoadInstance/vkEnumeratePhysicalDevices function pointer is null, and when
vkEnumeratePhysicalDevices(_instance, &device_count, nullptr) returns
non-VK_SUCCESS or when device_count == 0 (include result and device_count);
reference the symbols volkInitialize, vkCreateInstance,
vkCreateInstance(&create_info,...,&_instance), volkLoadInstance,
vkEnumeratePhysicalDevices, _instance, result, and device_count, and ensure any
existing vkDestroyInstance(_instance, nullptr) calls remain in place and logs
are emitted before each return false.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #674
Runs a check to see if Vulkan is avaliable before trying to initialize the Rendering Driver for it.
This fixes the errors that popup if Vulkan drivers are not present on a users system despite being in Compatibility mode.
I tested it on mine and it seems to work on my system with and without vulkan drivers installed.
I suggest some additional testing from other people to make sure that this works
Summary by CodeRabbit