Silk.Net.Vulkan API proposals

[abenedik]

First, congratulate on the great work that you are doing!

I have recently ported my Vulkan rendering engine to your library and it is working very well. But I have a few recommendations for the API:

Flag enum value names are too long and contain the name of the enum.
For example:
DebugUtilsMessageTypeFlagsEXT.DebugUtilsMessageTypeGeneralBitExt should be DebugUtilsMessageTypeFlagsEXT.General (or GenetalBit)

The "General" text is what is important for the user. But this is very hard to see because of the long prefix and suffix.
I see that the current name is converted from vulkan.h where it is defined as "VK_DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT". But still the code generator could remove the prefix and suffix.

The current name is also breaking code style rule CA1712

To prevent creating a breaking change by renaming the existing enum values, the generator could preserve the existing names and make them as obsolete. This would allow compiling the existing code with the new version of Silk.Net.Vulkan and allow developers to easily adapt to the new names.

The structs in Silk.Net.Vulkan assembly provide an additional constructor with optional parameters. This allows the user to generate the struct by providing only some of the parameters. But the problem with this implementation is that all the parameters are nullable. For value types this requires boxing.

For example the constructor for Extent3D is defined as:

public Extent3D(uint? width = null, uint? height = null, uint? depth = null)
    : this()
{
    if (width.HasValue)
       this.Width = width.Value;
    if (height.HasValue)
        this.Height = height.Value;
    if (!depth.HasValue)
        return;
    this.Depth = depth.Value;
}

When Extent3D is created by "new Extent3D(10, 10, 10)" the 3 int values are converted into 3 new objects that are passed to the constructor.

Instead of using nullable types the same optional parameter functionality can be achieved by using default keyword. For example:

public Extent3D(uint width = default(uint), uint height = default(uint), uint depth = default(uint))
: this()
{
    this.Width = width;
    this.Height = height;
    this.Depth = depth;
}

For number the default can be also replaced by 0. But the default keyword would be used for other more complex types like "void*", "Buffer", etc. (using "default(void*)", "default(Buffer)", etc.)

What is more, improved constructors can be used to set the default value for the sType field. Many Vulkan structs that are used to create vulkan objects define the sType filed. The value of the sType field must match the type of the struct. In .Net it is not possible to fix the value of this field.

But with using a special constructor it would be possible to set this field to the required value. For example:

public BindBufferMemoryInfo
(
    Buffer buffer = default(Buffer),
    DeviceMemory memory = default(DeviceMemory),
    ulong memoryOffset = default(ulong)
) : this()
{
    SType        = StructureType.BindBufferMemoryInfo; // Should not be set to any other value so we can skip sType parameter
    PNext        = null;                               // PNext is almost always set to null so we can also skip this parameter (user can still init it or set it later)
    Buffer       = buffer;
    Memory       = memory;
    MemoryOffset = memoryOffset;
}

For comparison, the current constructor has the following signature:

public BindBufferMemoryInfo
(
    StructureType? sType = StructureType.BindBufferMemoryInfo,
    void* pNext = null,
    Buffer? buffer = null,
    DeviceMemory? memory = null,
    ulong? memoryOffset = null)

As written in the code comment above, the struct constructor could set the SType value and also set PNext to null.

Most of the Vulkan objects are represented in .Net as structs with a ulong handle. This means we cannot check if the object is null (for example " buffer == null") but we need to check if the handle is zero ("buffer.Handle == 0"). Also, it is not possible to set the object to null, but we need to set handle to 0 ("buffer.Handle = 0;") or create a new object ("buffer = new Buffer();").

It would be nicer that the objects would provide an IsNull getter and a static Null getter. The implementation for Buffer object would look like:

public bool IsNull => Handle == 0;
public static Buffer Null => new Buffer();

This would allow some nicer code patterns, for example, when disposing an object that uses a buffer:

if (!buffer.IsNull)
{
    _vk.DestroyBuffer(_device, buffer, null);
    buffer = Buffer.Null;
}

Provide a ToString override or add DebuggerDisplay attribute to Vulkan structs that define only a Vulkan handle.

The ToString / DebuggerDisplay should display the object type and the value of the handle written as a hex value.

When handle values are written as decimal numbers, they are just very bug values that do not provide any data to the user and cannot be compared with other handle values. But if handle values are written as hex values, then it is seen that the value has some structure and also the values can be much easier compared. What is more, the Vulkan validation errors always display the handle values as hex values and this way it would be possible to check which object (struct) in your application this refers to.

Add None enum value (with value 0) to Flags enums.

In C and C++ it is common to set enum field to 0. Therefore the enum values in vulkan.h do not provide names for zero values.
But in .Net it is very unusual to set enum field to a value that is not defined in the enum. Also when the enum field is initialized it is set to a zero value - to a value that is not defined by an enum. Also, Vulkan expects that some enum values are usually set to 0, for example: ImageCreateInfo.Flags is usually 0; the SampleCountFlags can be 0 when no MSAA is used.

Mostly out of curiosity, I am wondering what is the purpose of NativeName attributes that are set to all structs and all fields in the structs, for example:

[NativeName("Name", "VkBindBufferMemoryInfo")]
public unsafe partial struct BindBufferMemoryInfo
{
    [NativeName("Type", "VkStructureType")]
    [NativeName("Type.Name", "VkStructureType")]
    [NativeName("Name", "sType")]
    public StructureType SType;

    [NativeName("Type", "void*")]
    [NativeName("Type.Name", "void")]
    [NativeName("Name", "pNext")]
    public void* PNext;

     //...

Because there are many fields and structs in the Vulkan assemblies defining all those attributes actually takes a significant amount of bytes in the assembly. If this is used just for annotations, then it would be probably better to generate an XML documentation file instead. For example, the XML documentation file can be generated from:

/// <summary>
/// VkBindBufferMemoryInfo
/// </summary>
public unsafe partial struct BindBufferMemoryInfo
{
    /// <summary>
    /// sType
    /// </summary>
    public StructureType SType;

    /// <summary>
    /// pNext
    /// </summary>
    public StructureType PNext;
}

Or you could even use "", for example:

/// <unmanaged>VkBindBufferMemoryInfo</unmanaged>
public unsafe partial struct BindBufferMemoryInfo
{
    /// <unmanaged>sType</unmanaged>
    public StructureType SType;

    /// <unmanaged>pNext</unmanaged>
    public StructureType PNext;
}

This would reduce the size of assemblies and also show the native name in the IntelliSense.

What do you think about those proposals?

If you find them useful and if you can help me to get started with changing the code generator I can also try to provide a PR for those items.

[HurricanKai]

1. I agree we can have aliases that are nicer to use.
2. I've long wanted to change this, but it's unfortunately a breaking change, so we can't until 3.0
3. I wanted to do this for some time, but I don't think IsNull is ideal, I hoped that the language would eventually have some feature for us here (I'd really like to have a way to support is null, but it doesn't. I'm not opposed to having an IsZero or IsNull property, though I'm not too big of a fan either.
4. We can do this for sure, though I want to say that for debugging purposes comparing handles isn't great. Just name your handles via vkSetDebugUtilsObjectNameEXT
5. I suppose this is fine, though I don't see the issue with using 0
6. I think this is just legacy, @Perksey ? - We want to keep some other form of documentation of the original member around though. I've used it a few times to lookup something (ie for enum names having the original name is very valuable)

[abenedik]

Great! Thanks for the answers.

I am glad to hear that you agree with most of the proposals. From my side, it is totally fine if this goes to 3.0.

I understand that (3) is very subjective. Though in my openion using "== default" is in not a nice solution. For example, "if (_renderPass == default)" look very unusual to me. It would make we wonder WTF is that (what is the default render pass) and I would not think that this means that the object is actually not defined (at least not on a first glimpse). In this case, I think that "if (_renderPass.Handle == 0)" is better for code readability. And from that perspective "if (_renderPass.IsNull)" or "if (_renderPass.IsZero") or something similar is even better in my opinion. But I understand that this is also not ideal.

Hm, "WIP Silk.NET Rider plugin" sounds like something that I would also like to install 🙂 Does that mean that NativeName annotations can be also useful for us during our work? But if this is used for one time check of the compiled version of the Silk.Net, then maybe the final published version may not have those attributes (I assume that this significantly increases the size of assembly, but I may be wrong there). I also understand that changing the validation and build process is a tricky task and that it may not be worth your time if it is only for a few bytes of assembly size.

Anyway, if you will decide to go for XML documentation path in the future, I would recommend using "unmanaged" attribute. Though this is not an official standard, it was used in SharpDX library (see: https://github.com/sharpdx/SharpDX/blob/master/Source/SharpDX.DXGI/DXGIObject.cs). Also, its value is shown to the user by the Visual Studio IntelliSense.

And thank you again for the great work that you do! I really appreciate it.

[Perksey]

Extra info given I'm the author of the bindings generator:

1. Agreed, want to fix this for 3.0. This is actually a bug in the generator which we can't fix due to a fix being a breaking change.
2. I don't actually remember why we did this, it was a conscious decision made in [2.0] The Wednesday round-up pull request, now on a Saturday. #340 but I don't remember what we were working around in doing this. But when/if we use record struct this will be solved, and in any case I 100% want to do this for 3.0.
3. Would be fixed by record structs in 3.0 again which generates equality operators so you can do == default. We could also add our own operators which accept nint on either side so you can do == 0. But unfortunately, unlike C++ there's no definitive "null" type so it's going to be impossible to do == null I'm afraid.
4. NativeName is for extra source annotation (for our reference as well as the reader's reference) as well as for use in static analysis tools, such as the WIP Silk.NET Rider plugin. It's very useful for validation, and I don't see it being removed. You're right we could use XML documentation instead, but it's not as easy to work with for static analysis and there aren't really any suitable standard XML doc tags for this.

[HurricanKai]

I don't think record structs are allocated on the heap, that's kind of the point behind them?
Could it be you're mixing up records and record structs?
Normal structs are like classes and heap allocated.
I may he misinformed here, I'll check that again

[Perksey]

Yeah a record struct is just a struct but with added pre-generated code from the compiler.

However, record structs use properties. This means that on debug builds there will indeed be a miniscule performance drop.

push ebp
mov ebp, esp
sub esp, 0x4c
vzeroupper
vxorps xmm4, xmm4, xmm4
vmovdqu [ebp-0x44], xmm4
vmovdqu [ebp-0x34], xmm4
vmovdqu [ebp-0x24], xmm4
vmovdqu [ebp-0x14], xmm4
mov [ebp-4], ecx
cmp dword ptr [0x1014c1a8], 0
je short L0032
call 0x6366fc10
nop
vxorps xmm0, xmm0, xmm0
vmovdqu [ebp-0x24], xmm0
vmovdqu [ebp-0x14], xmm0
lea ecx, [ebp-0x24]
mov edx, 1
call RPBI_B.set_SType(UInt32)
nop
mov dword ptr [ebp-0x48], 3
mov ecx, [ebp-0x48]
sar ecx, 0x1f
push ecx
push dword ptr [ebp-0x48]
lea ecx, [ebp-0x24]
call RPBI_B.set_RenderPass(UInt64)
nop
lea ecx, [ebp-0x24]
mov edx, 4
call RPBI_B.set_ClearValueCount(UInt32)
nop
lea ecx, [ebp-0x24]
mov edx, 5
call RPBI_B.set_PClearValues(IntPtr)
nop
mov dword ptr [ebp-0x4c], 6
mov ecx, [ebp-0x4c]
sar ecx, 0x1f
push ecx
push dword ptr [ebp-0x4c]
lea ecx, [ebp-0x24]
call RPBI_B.set_RenderArea(UInt64)
nop
vmovdqu xmm0, [ebp-0x24]
vmovdqu [ebp-0x44], xmm0
vmovdqu xmm0, [ebp-0x14]
vmovdqu [ebp-0x34], xmm0
nop
jmp short L00b6
mov eax, [ebp-4]
vmovdqu xmm0, [ebp-0x44]
vmovdqu [eax], xmm0
vmovdqu xmm0, [ebp-0x34]
vmovdqu [eax+0x10], xmm0
mov esp, ebp
pop ebp
ret

This isn't too much cause for concern, as on release builds these calls are inlined, resulting in identical assembly for both structs and record structs:

sub esp, 0x20
vzeroupper
vxorps xmm0, xmm0, xmm0
vmovdqu [esp], xmm0
vmovdqu [esp+0x10], xmm0
mov dword ptr [esp], 1
xor eax, eax
mov dword ptr [esp+8], 3
mov [esp+0xc], eax
mov dword ptr [esp+0x18], 4
mov dword ptr [esp+0x1c], 5
mov dword ptr [esp+0x10], 6
mov [esp+0x14], eax
vmovdqu xmm0, [esp]
vmovdqu [ecx], xmm0
vmovdqu xmm0, [esp+0x10]
vmovdqu [ecx+0x10], xmm0
add esp, 0x20
ret

---

However, this brought up another issue: record structs can't have pointers! So we won't be using them for the time being.

[abenedik]

Uh yes, you are right - I mixed record structs with records. I see now that record structs come with C# 10, while records were introduced in C# 9. This is all new stuff to me. And also Google returns many results for records when searching for record structs and in those cases I was not paying enough attention.

Ok, so record structs are the same as normal structs, except that the compiler automatically adds a few default implementations for Equals and GetHashCode and that you can easily define them in a single line (public readonly record struct RecordStruct(Type Type, int Value); - defining Type and Value properties).

Sorry for the confusion.

[abenedik]

Uh you are fast with your analysis of the generated assembly code... My respect!

[thargy]

Interestingly, record structs come with real performance gains around equality and hash codes (useful for dictionaries, etc.) over regular structs and are otherwise of equal performance. In general, I haven't yet seen a scenario where you shouldn't use them over regular structs - though the lack of pointer support would be one (except where we move to nint anyway perhaps?)