FacticiusVir/SharpVk

CommandCaches confusion

SinnersSumit opened this issue · 4 comments

After updating my version of sharpvk I found a whole bunch of new function overloads that include 'CommandCaches' as arguments, a quick look at the object browser and source would seem to indicate that it is an internal object used to store vulkan commands, and that it cannot be created by external code.

ex. public static ExtensionProperties[] EnumerateExtensionProperties(CommandCache commandCache, string layerName)

Is CommandCache meant to be visible from calling code? If so, it needs some work, as there doesn't currently seem to be any way to create or even acquire one. If not, the functions that include it should probably be set to internal, or otherwise hidden to avoid confusion.

So this change was part of the switch to .NET Standard - because there is no mechanism in Standard/Core to replace Mono's DllMaps, there has to be a point early in the execution path that checks the current environment and loads the relevant Vulkan DLLs. You'll see this triggered in https://github.com/FacticiusVir/SharpVk/blob/master/src/SharpVk/Instance.partial.cs#L44, when it creates SharpVk.Interop.NativeLibrary.

CommandCache was always used internally for loading and caching functions added by Vulkan extensions, as those can't map through PInvoke and required a call to one of the GetProcedureAddress commands; so it was the easiest place to extend now that we must load & cache the core functions as well. As it is used before any Vulkan handles are created - which is where the cache would originally have been held - it must be passed in from the outside, hence the new method overloads.

Those new overloads were made public, as they could potentially allow calling code to inject a Vulkan API stub - useful if I get time to extend the automated testing in future - but as you noticed, I'd forgotten to make CommandCache instantiable from outside SharpVk so those overloads are not usable from external code.

The current CommandCache constructor exposes a little too much of the internal logic, particular around the tiering of Device/Instance/External command caching, but I'll add new constructor that accepts an IProcLookup (https://github.com/FacticiusVir/SharpVk/blob/master/src/SharpVk/CommandCache.cs#L74) to enable that use case. Thanks for the spot!

Sounds good. Another thing to note, is I may have found an error in CommandCache or at least one particular use of it. For some reason GetCommandDelegate in CreateDebugReport is throwing an argument null exception;

System.ArgumentNullException
  HResult=0x80004003
  Message=Value cannot be null.
Parameter name: ptr
  Source=mscorlib
  StackTrace:
   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointer(IntPtr ptr, Type t)
   at System.Runtime.InteropServices.Marshal.GetDelegateForFunctionPointer[TDelegate](IntPtr ptr)
   at SharpVk.CommandCache.GetCommandDelegate[T](String name, String type)
   at SharpVk.Multivendor.InstanceExtensions.CreateDebugReportCallback(Instance extendedHandle, DebugReportCallbackDelegate callback, Nullable`1 flags, Nullable`1 userData, Nullable`1 allocator)
   at VulkanFormsTest.Form1.InitializeDebugger() in D:\User\Documents\Programming\TestCode\VulkanFormsTest\VulkanFormsTest\Form1.cs:line 85
   at VulkanFormsTest.Form1..ctor() in D:\User\Documents\Programming\TestCode\VulkanFormsTest\VulkanFormsTest\Form1.cs:line 173
   at VulkanFormsTest.Program.Main() in D:\User\Documents\Programming\TestCode\VulkanFormsTest\VulkanFormsTest\Program.cs:line 19

The relevant parts of the stacktrace appear to go through these lines;

[CreateDebugReport at line 89](https://github.com/FacticiusVir/SharpVk/blob/73ede8512f3f58c07524b054e09f3a6877995396/src/SharpVk/Multivendor/InstanceExtensions.gen.cs#L89)
[GetCommandDelegate at line 65](https://github.com/FacticiusVir/SharpVk/blob/ccddbfd0f426f793b42d687a7b80522b0c45d772/src/SharpVk/CommandCache.cs#L65)

url formatting isn't working for me for whatever reason

looks like the IProcLookup is returning null for some reason

You'll get null from a procedure lookup if the requested command isn't valid in the current instance/device - have you included "VK_EXT_debug_report" in the Instance enabled extensions? (You can also use the ExtExtensions.DebugReport constant)

The null reference exception isn't the prettiest way to handle it, so I'll look at adding a more descriptive error. Either way, you'd always get an exception if the underlying API doesn't find the command.

Could have sworn I did... but It looks like I did indeed forget to include it. Thanks for catching that.