More progress on Vulkan initialization #12
Conversation
WalkthroughThe pull request introduces significant enhancements to Vulkan descriptor management in the graphics system. The changes focus on adding a robust descriptor set allocation and management mechanism in the Changes
Possibly related PRs
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
🧹 Nitpick comments (1)
src/Engine/Graphics.cpp (1)
939-939: Remove unused variabledevice_sizeThe variable
device_sizedeclared at line 939 is unused in the subsequent code. Removing it will clean up the code.Apply this diff to remove the unused variable:
- constexpr VkDeviceSize device_size{ 0 };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Engine/Graphics.cpp(8 hunks)src/Engine/Types.h(1 hunks)src/Engine/VMA.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Engine/VMA.cpp
🔇 Additional comments (5)
src/Engine/Graphics.cpp (4)
11-12: Includes for<queue>and<stack>are necessaryThe inclusion of
<queue>and<stack>headers is appropriate as they are used in thedescriptor_set_allocatorimplementation.
961-968: Verify the use ofvkGetDeviceQueue2The function
vkGetDeviceQueue2is used to retrieve the presentation queue. Ensure that the Vulkan API version supports this function (it requires Vulkan 1.1 or higher).If your application needs to support Vulkan versions prior to 1.1, consider using
vkGetDeviceQueueinstead.
47-48: Proper handling of overflow condition inallocatemethodThe
allocatemethod correctly checks for the maximum index limit and returnsrosy::result::overflowwhen exceeded. Good job handling potential overflow.
1187-1191: Ensure consistent null checks for initialization parametersThe function checks for
new_windowandnew_logbeingnullptrbefore proceeding. This is good practice to prevent null dereferences.src/Engine/Types.h (1)
13-13: Ensure all code handles the newoverflowresultThe addition of
overflowto theresultenum expands the possible return values. Make sure that all switch statements and error handling code that useresultare updated to handle this new value appropriately.Run the following script to find all usages of
rosy::resultand check for missing cases:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Engine/Graphics.cpp (1)
78-84: Improve error handling in descriptor_set_manager::initThe method always returns
result::okwithout validating inputs. Consider adding checks for:
new_max_indexes > 0new_bindingvalidityresult init(const uint32_t new_max_indexes, const uint32_t new_binding) { + if (new_max_indexes == 0) { + return result::invalid_argument; + } binding = new_binding; allocator = descriptor_set_allocator{}; allocator.max_indexes = new_max_indexes; return result::ok; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Engine/Graphics.cpp(10 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Engine/Graphics.cpp (1)
Learnt from: btipling
PR: btipling/Rosy#12
File: src/Engine/Graphics.cpp:999-1024
Timestamp: 2025-01-13T00:35:51.032Z
Learning: In the Rosy engine, cleanup of graphics resources (including descriptor managers) is handled through the deinit chain: Engine.cpp calls graphics::deinit() which calls graphics_device::deinit(). This ensures proper cleanup even if initialization fails midway.
🔇 Additional comments (8)
src/Engine/Graphics.cpp (8)
5-6: LGTM! Required includes for descriptor management.The added STL includes are necessary for the descriptor set allocator implementation.
36-69: Consider thread safety for descriptor_set_allocatorThe allocator uses shared resources (
std::stack, member variables) without synchronization, which could lead to data races if accessed concurrently.
167-182: LGTM! Well-organized member variables.The new member variables are properly initialized and logically grouped.
939-954: LGTM! Proper VMA initialization setup.The VMA allocator is correctly configured with Vulkan 1.3 support and buffer device address capabilities.
962-978: LGTM! Well-structured presentation queue initialization.Good implementation with proper error handling and debug utils integration.
1055-1057: LGTM! Good validation of binding counts.Proper validation ensuring consistency between bindings, flags, and pool sizes.
1188-1192: LGTM! Proper input validation.Good defensive programming with null checks for critical parameters.
999-1024: 🛠️ Refactor suggestionHandle descriptor manager cleanup on initialization failure
While the deinit chain handles cleanup on normal shutdown, the error paths need proper cleanup to prevent memory leaks during initialization failures.
if (desc_sampled_images == nullptr) { + delete desc_storage_images; + desc_storage_images = nullptr; return VK_ERROR_OUT_OF_HOST_MEMORY; } if (desc_samples == nullptr) { + delete desc_storage_images; + delete desc_sampled_images; + desc_storage_images = nullptr; + desc_sampled_images = nullptr; return VK_ERROR_OUT_OF_HOST_MEMORY; }Likely invalid or redundant comment.
Summary by CodeRabbit
New Features
overflowenumerator.Bug Fixes
Chores