amerkoleci / Vortice.Vulkan

Cross platform .NET bindings for Vulkan, VMA, SPIRV-Cross and shaderc
MIT License
298 stars 26 forks source link

`CheckResult` and `ThrowIfFailed` seem to be incorrect #54

Closed malstraem closed 1 month ago

malstraem commented 1 month ago

VkResult codes have values ​​both greater than and less than zero. So it seems that both methods should throw only when the result code is less than zero.

For example, vkGetEventStatus returns VK_EVENT_SET and VK_EVENT_RESET on success.

amerkoleci commented 1 month ago

Hi, Not sure what you mean, it checks if Result is different than VK_SUCCESS, whats wrong?

malstraem commented 1 month ago

@amerkoleci, sorry for the understatement, I'll try to clarify.

If I understand correctly, you intended both methods to be error checking (I think me and many others have written something similar in their code). So the reason I think methods should only throw exceptions on negative values ​​is that the spec clearly says that success codes are non-negative values​​.

Here's imitate with mentioned vkGetEventStatus:

VkEventCreateInfo eventCreateInfo = new();

vkCreateEvent(device, in eventCreateInfo, null, out var @event);

// some logic

var result = vkGetEventStatus(device, @event);

ThrowIfFailed(result); // or result.CheckResult(); will be throw

if (result is VkResult.EventReset) // unreachable
{
    // some logic
}

Using your extensions to check for VK_ERROR_OUT_OF_HOST_MEMORY, VK_ERROR_OUT_OF_DEVICE_MEMORY, VK_ERROR_DEVICE_LOST that may be returned will reason in an exception being thrown, but there will be no actual failure.

Many methods can return different success codes, leaving the user to process the branches.

For example, vkWaitForFences can return VK_TIMEOUT to be processed or vkAcquireNextImageKHR can return VK_SUBOPTIMAL_KHR that is legal to continue using the swapchain, etc.

So, to be actually error checking, methods need to replace result != VkResult.Success with result < VkResult.Success.

Fun fact, khronos's validation layers also dove into ambiguity.

Your bindings excellent anyway 🤓

amerkoleci commented 1 month ago

Thanks!

Fixed with https://github.com/amerkoleci/Vortice.Vulkan/commit/7339287960c71ec3de1a483c285238a02bfb9885, grab the new 1.9.7 version

Thanks again