ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
59.41k stars 10.12k forks source link

Vulkan backend helper function question #7830

Open mihaly-sisak opened 1 month ago

mihaly-sisak commented 1 month ago

Version/Branch of Dear ImGui:

v1.91.0 WIP, master branch

Back-ends:

imgui_impl_sdl2.cpp + imgui_impl_vulkan.cpp

Compiler, OS:

any

Full config/build information:

No response

Details:

My Issue/Question:

I am currently learning Vulkan, and I found the ImGui example/backed helper functions incredibly helpful. However I came trough this line of code and I would like to ask for clarification.

In the Vulkan implementation helper function ImGui_ImplVulkanH_SelectSurfaceFormat there is a check for VK_FORMAT_UNDEFINED returned from vkGetPhysicalDeviceSurfaceFormatsKHR. imgui_impl_vulkan code in question

However the standard states that here the Vulkan API can not give back VK_FORMAT_UNDEFINED: Vulkan 1.0 vkGetPhysicalDeviceSurfaceFormatsKHR

The number of format pairs supported must be greater than or equal to 1. pSurfaceFormats must not contain an entry whose value for format is VK_FORMAT_UNDEFINED.

What is the reasoning behind checking for it? Is this code a workaround of some buggy Vulkan drivers?

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

    // First check if only one format, VK_FORMAT_UNDEFINED, is available, which would imply that any format is available
    if (avail_count == 1)
    {
        if (avail_format[0].format == VK_FORMAT_UNDEFINED)
        {
            VkSurfaceFormatKHR ret;
            ret.format = request_formats[0];
            ret.colorSpace = request_color_space;
            return ret;
        }
        else
        {
            // No point in searching another format
            return avail_format[0];
        }
    }
ocornut commented 1 month ago

Hello,

That's a good question, I don't know the answer. I did a git blame and found the first occurrence for this to be : 33874073dc3275a7d99e20c9986d39998328b62f by @ParticlePeter as part of #1042.

I am currently learning Vulkan, and I found the ImGui example/backed helper functions incredibly helpful.

I would like to stress that I'm personally terrible at Vulkan, that the backend has been the sum of variety of heterogeneous contributions over the years, and that it is likely not good learning material. So take it with a grain of salt :)

NostraMagister commented 1 month ago

The format enum (VKFORMAT...) is not exclusively for use with the last argument of vkGetPhysicalDeviceSurfaceFormatsKHR(). The specifications (below) that you point out, simply says that VK_FORMAT_UNDEFINED is NOT a valid value to return as part of the list. Since the specs make at least one format mandatory and VK_FORMAT_UNDEFINED may not be in the list, Vulkan guarantees that there will ALWAYS at least by one usable format. I just reiterate this for readers not familiar with this because you came to that conclusion yourself I think.

The number of format pairs supported must be greater than or equal to 1. pSurfaceFormats must not contain an entry whose value for format is VK_FORMAT_UNDEFINED.

You are correct about the fact that checking for it is not needed.

Below is the code (and some comments) that is part of my SDL3/Vulkan backend. Possibly it can help you although from your OP I am under the impression that you may already have something like that. I leave the comments because they clarify why the loop below ends in an error if none of the wanted pairs is found, rather than defaulting to one. It is a choice. For Dear ImGui use VK_FORMAT_B8G8R8A8_UNORM because examples (such as displaying images with ImGui::Image()) use that format.

// Users accept the defaults or can set the this->vul_SurfaceFormat.format and/or this->vul_SurfaceFormat.colorSpace to force a selection. Remember that if the selection (pair) doesn't exist we stop because the application expects an exact channel order, we don't use the top of list as default. Possible user choices.
// SDR formats: VK_FORMAT_B8G8R8A8_UNORM (default SDR BGR) and VK_FORMAT_R8G8B8A8_UNORM (SDR RGB)
// HDR formats: VK_FORMAT_A2B10G10R10_UNORM_PACK32 (HDR BGR) and VK_FORMAT_A2R10G10B10_UNORM_PACK32 (HDR BGR)
// Color Space: VK_COLOR_SPACE_SRGB_NONLINEAR_KHR (default). TODO: block all means for the user to change this.

// NOTE: In Vulkan the Surface Format can be different from the View Format if the swapchain image(s) has/have been created with the VK_IMAGE_CREATE_MUTABLE_BIT.
// We don't use the MUTABLE bit because we have no use case for it (yet) such as Multi-Planar Images/YUV, Texture Compression/BC1 or image as Render Target in a different format as sampled by a shader.

// Find the Formats that are available on the used physical device (GPU). There MUST, per Vulkan specs, always at least be one.
// https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetPhysicalDeviceSurfaceFormatsKHR.html
std::uint32_t availableSurfaceFormatsCount=0;
vkGetPhysicalDeviceSurfaceFormatsKHR(this->vul_PhysicalDevice, this->vul_Surface, &availableSurfaceFormatsCount, nullptr);
std::vector<VkSurfaceFormatKHR> availableSurfaceFormats(availableSurfaceFormatsCount);
vkGetPhysicalDeviceSurfaceFormatsKHR(this->vul_PhysicalDevice, this->vul_Surface, &availableSurfaceFormatsCount, availableSurfaceFormats.data());

// Check if we have the wanted format. If not we stop because the application EXPECTS that precise format when it will manipulate color values.
for (uint32_t i = 0; i < availableSurfaceFormatsCount; i++)
{
    // Remember, we look for a PAIR (format+color space)
    if (availableSurfaceFormats[i].format != this->vul_SurfaceFormat.format) continue;
    if (availableSurfaceFormats[i].colorSpace != this->vul_SurfaceFormat.colorSpace) continue;
    return VK_SUCCESS;
}

return VK_ERROR_FORMAT_NOT_SUPPORTED;