LibGfx: Refactor Vulkan code and fix some bugs #6976
Conversation
cqundefine
force-pushed
the
vulkan_context_refactor
branch
from
25288ef to
b17623b
Compare
We had the same check in both places so let's just unify it. For bonus points it also removes the need to include VulkanContext.h in some places which will be cleaned up in the next commit.
This is reduces the amount of rebuilt files from a few hundred to around 20, which makes editing it much easier.
This includes changing it from a struct to class, making all members private, adding getters and moving the create function inside the class.
Similarly to VulkanImage, this is now more like the other Ladybird code. The member variables have been made private, getters added and the create function moved to the class.
This is much cleaner than using array_size or manually specifying the size.
cqundefine
force-pushed
the
vulkan_context_refactor
branch
from
b17623b to
9df75b3
Compare
|
When Also the message should be Also what about or Should we be mentioning any distribution packages needing to be installed or Vulkan SDK installation requirements somewhere in the ladybird documentation. |
|
Strictly speak, is it required to modify any code to get the validation layer messages? Can't that specific part of it be achieved with the Vulkan Configurator from the Vulkan SDK? Is there any additional benefits to putting it in the code. I think that would allow you to remove all the VULKAN_DEBUG items. |
This allows to easily toggle Vulkan validation layers which can be really helpful while debugging Vulkan code.
Otherwise the validation layers complain that it's required by `VK_EXT_image_drm_format_modifier`.
Otherwise the validation layers complain that this is required when using VK_EXTERNAL_MEMORY_HANDLE_TYPE_DMA_BUF_BIT_EXT as an image handle.
This does not seem to impact functionality and gets rid of a validation layer warning.
The validation layers complain if it's set to concurrent and we have only one queue.
This just filters any of the "VK_*" defines that were mistaken for our debug macros, for example VK_EXT_DEBUG_UTILS_EXTENSION_NAME.
cqundefine
force-pushed
the
vulkan_context_refactor
branch
from
9df75b3 to
8543d19
Compare
|
I changed the message and ended up making the lack of validation layer a non-critical failure. I wonder if it should remain obligatory as
I believe the main benefit is convenience and not requiring the use of external tools. I do frequent Vulkan development and I actually don't have the Vulkan SDK installed at all. I think looking at how it's a relatively small amount of code I would be more inclined to keep it. I think it would be great to get an opinion on a maintainer on this. |
|
I would go to discord to try to get some attention on this one. It is a PR covering multiple Vulkan issues and with many commits so requires more dedicated attention. Not always possible, but simpler PRs are usually easier to digest during a review. One thing I would have found useful is to only turn on the validation layers through code (without the other fixes) so as to check the messages myself. I know I could cherry-pick, and I quickly tried and got a merge conflict that I didn't try to solve at all (probably not that hard to resolve), but yeah that commit first in the stack would be ideal or maybe in its own PR separate from the actual Vulkan fixes. You don't have to do it, just kind of an observation, or you could have attached a log with the validation layer messages you got. I realized soon after I left my post that having the validation layers in code was very convenient. No SDK needed, like you said. I remember trying to use for the first time the Vulkan Configurator and it took some time (not that much though) to get it working right. Regarding the VULKAN_DEBUG, I had the same thought about what if someone wants to use it for something not validation layer related. Should there be a VULKAN_DEBUG macro (for general Vulkan debug messages) and a VULKAN_VALIDATION_LAYER_DEBUG macro to separate the functions, you might not want the two together? Some general comments about the commit messages:
Regarding the above commit, did you try to follow the instructions in Meta/check-debug-flags.sh to add the Vulkan defines in the false positives in
|
2 participants
This PR makes VulkanContext and VulkanImage closer to the Ladybird C++ code style which also prevents exposed member variables. I've tried enabling validation layers which actually turned out to be quite helpful and exposed a few bugs. I've left that code there locked behind a debug macro.