Skip to content
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

[vendor:vulkan]: Missing bind for VK_NULL_HANDLE #4450

Open
flysand7 opened this issue Nov 4, 2024 · 6 comments
Open

[vendor:vulkan]: Missing bind for VK_NULL_HANDLE #4450

flysand7 opened this issue Nov 4, 2024 · 6 comments

Comments

@flysand7
Copy link
Contributor

flysand7 commented Nov 4, 2024

All handles in vulkan are distinct unsigned 64-bit integers. As a new person to this API, I haven't looked at the underlying types, so whenever a tutorial uses VK_NULL_HANDLE, this leaves me guessing that with Odin bindings the corresponding constant is one of the following:

  • vk.NULL_HANDLE
  • nil

The nil doesnt work, because the handles aren't pointers, but NULL_HANDLE doesn't exist, so all I can do is pass a 0 into the function.

vk.AcquireNextImageKHR(dev, swapchain, max(u64), image_semaphore, 0, &image_index)

This has slight effect on readability as I can't tell that the 0 in this case is not some kind of count, but a fence handle.

@nobodyspecial1553
Copy link

nobodyspecial1553 commented Nov 4, 2024

The way I've alleviated this issue is to add my own context in.
vk.AcquireNextImageKHR(dev, swapchain, max(u64), pImageIndex = &image_index, semaphore = image_semaphore, fence = 0)
Even with NULL_HANDLE, I'd probably still do this just for the extra context.
At the end of the day though I won't be messing with a number if I can't remember what it's for, so I'll look at the docs or source for clarification.

Totally agree that it should be there though.

@gingerBill
Copy link
Member

All handles in vulkan are distinct unsigned 64-bit integers.

Did you bother to check the source code?

Handle                :: distinct rawptr
NonDispatchableHandle :: distinct u64

https://github.com/odin-lang/Odin/blob/master/vendor/vulkan/core.odin#L21-L22

The basic handles are pointers, whilst the non-dispatchable ones are u64.

The problem here is that in C/C++ 0 can implicitly convert to both pointers and integers, and depending on how you configure the Vulkan headers, depends on what happens. There is also the other issue where the headers are not consistent on how they define the handles:

https://github.com/KhronosGroup/Vulkan-Headers/blob/main/include/vulkan/vulkan_core.h#L29-L60

So on the platforms that have VK_USE_64_BIT_PTR_DEFINES defined, those non-dispatchable handles are actually pointers, but on the other platforms they are uint64_t objects. This literally means that they are not consistent about the their own types and constants.

This was part of the compromise I made when making the Vulkan headers. It might be okay to have something like a NULL_HANDLE :: 0 but that's all it would be and not really that useful in practice.

@nobodyspecial1553
Copy link

Whilst you're more replying to flysand, I should have answered something similar myself considering I now remember having wondered why there was no NULL_HANDLE and then realizing why soon thereafter. My brain's memory banks hardly work.

So I retract my agreement that there should be a NULL_HANDLE, but stand everything else I said.

@flysand7
Copy link
Contributor Author

flysand7 commented Nov 4, 2024

Tried to read the documentation on this (emphasis mine):

The reserved values VK_NULL_HANDLE and NULL can be used in place of valid non-dispatchable handles and dispatchable handles, respectively, when explicitly called out in the Specification. Any command that creates an object successfully must not return these values. It is valid to pass these values to vkDestroy or vkFree commands, which will silently ignore these values

It seems that VK_NULL_HANDLE is meant to be used with non-dispatchable handles specifically. For dispatchable handles we have nil.

So on the platforms that have VK_USE_64_BIT_PTR_DEFINES defined, those non-dispatchable handles are actually pointers, but on the other platforms they are uint64_t objects. This literally means that they are not consistent about the their own types and constants.

They actually are consistent, according to the docs:

#ifndef VK_DEFINE_NON_DISPATCHABLE_HANDLE
    #if (VK_USE_64_BIT_PTR_DEFINES==1)
        #if (defined(__cplusplus) && (__cplusplus >= 201103L)) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201103L))
            #define VK_NULL_HANDLE nullptr
        #else
            #define VK_NULL_HANDLE ((void*)0)
        #endif
    #else
        #define VK_NULL_HANDLE 0ULL
    #endif
#endif
#ifndef VK_NULL_HANDLE
    #define VK_NULL_HANDLE 0
#endif

We don't handle these defines in Odin in any way. Also these defines arent meant to be made by the system, at least according to the docs (emphasis again mine):

The vulkan_core.h header allows the VK_USE_64_BIT_PTR_DEFINES definition to be overridden by the application.

Which seems to suggest whether non-dispatchable handles are objects or pointers at the end is our choice.

So it seems like there would be no problems or consistency issues regarding adding a NULL_HANDLE binding.

@gingerBill
Copy link
Member

Well it's not really a choice if they are pointers or integers if you want the types to be consistent on a per platform basis. Which is why I chose u64.

So the only logical choice would be to do:

NULL_HANDLE :: 0

which is still not great because it's just 0

@flysand7
Copy link
Contributor Author

flysand7 commented Nov 5, 2024

Why is it not great? It defines a constant that is present in the vulkan headers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants