Silk.NET icon indicating copy to clipboard operation
Silk.NET copied to clipboard

Tracking Issue for Bindings Generation & Improvements

Open Perksey opened this issue 3 years ago • 57 comments

Comments

We truly invite everyone to send us reports of any place where our bindings are not ideal. This issue will serve as a list of things we should fix in 3.0.

The following summary comments have been formulated from your feedback, and these comments can be consulted for further information on the specific tasks:

  • https://github.com/dotnet/Silk.NET/issues/887#issuecomment-2127867602

This is a living document! Please do not hesitate to point out any other potential improvements - we may add them to this list. If you would like to work on a task, please contact @Perksey in Discord for a brain dump if it's unclear what is required of a task.

Priority 0 (required for the first 3.0 preview)

  • [x] OpenGL generates both GLEnum and the stronger group enums. Functions taking enums as parameters are declared as a "duck type" Constant<uint, GLEnum> or Constant<uint, GLEnum, BufferUsage>, meaning that you can pass in a uint, GLEnum, or BufferUsage using implicit casts thereby not requiring an explosion in overloads.
  • [x] All functions are now stored in the GL class directly. GL can be used in a static way or as an instance of IGL. The static mechanism essentially just forwards to a thread-local IGL instance, but this won't be true for every binding.
  • [x] Automatically detect pointers to opaque structures and encapsulate these as handle structures e.g. VkInstanceT* or Ptr<VkInstanceT>/Ref<VkInstanceT> becomes VkInstance which contains a void* - make sure this is generic and can work with any binding.
    • Vulkan, WebGPU, and SDL depend on this.
    • [x] Handle suffix has been added to generated structs
  • [x] Fixed buffer structs should be extracted and have a sensible name
  • [x] Anonymous structs should be extracted and have a sensible name

Priority 1 (required for the first 3.0 preview that contains a binding dependent on the task)

  • [ ] Automatically detect pointers to structs containing a vtable pointer void** and encapsulate these as handle structures for them e.g. ID3D11Device* becomes D3D11Device containing a void***.
    • Required for any binding containing COM APIs.
    • [ ] Generic methods will take a type implementing IComVtbl as they do today (a TSelf generic parameter may be required for codegen in terms of these interfaces) i.e. CreateFactory<T>() -> T where T : IComVtbl/IComVtbl<T> and get a DXGIFactory2 out. These should be generated as they are in 2.X i.e. where IID_PPV_ARGS would be used in C/C++.
    • [ ] Handle types can implicitly case to their base types i.e. DXGIFactory2 will implicitly cast to DXGIFactory1 for example, to address a comment raised in this thread about polymorphism.
  • [ ] Per https://github.com/dotnet/Silk.NET/pull/2020: Note that we need to make changes with/in ClangSharp to allow us to get the canonical type details e.g. HWND is actually HWND__* but the attribute only shows us HWND. This is to ensure that CreateBasicSymbolConstraints and co works correctly.
    • Required for any binding containing COM APIs.
  • [x] Implement Vulkan-style vtables i.e. using vkLoad*ProcAddr functions correctly using handles captured from vkCreateInstance/vkCreateDevice a la 2.X.
    • Required for Vulkan or OpenXR.
    • [x] Ideally make it less complicated than 2.X and just throw (with a helpful message!) if the user tries to use multiple instance/device combos with one object, rather than maintaining a dictionary of vtables.

Priority 2 (required in any 3.0 preview)

  • [ ] ClangSharp currently erroneously renames "param" to "param". Once fixed upstream, remove this hack. https://discord.com/channels/521092042781229087/587346162802229298/1437503858849878037
  • [ ] Don't drop the last word from a name trimming prefix if an enum name would start with a number once trimmed - instead, just prepend an X to the trimmed name.
  • [ ] Support OpenGL handle generation as if they were opaque pointer types.
  • [ ] Document the Silk.NET DSL i.e. the duck types, nullptr, AsRef, etc... Have a comprehensive documentation page describing all of our esoteric types and how to read function signatures defined in terms of them.
  • [ ] Enforce extensions and versions using the SupportedApiProfile by using an analyser similar to SupportedOSPlatform. The analyser would encourage you to either annotate your function with its own attribute (such that the requirement bubbles up throughout the program) or to check that requirement a la GL.IsExtensionPresent("GL_KHR_debug") or GL.IsVersionAtLeast(4, 3).
  • [x] Introduce Pfn structures for function pointers that marshal invocations of the delegate being marshalled to DSL structures i.e. so users can use Ptr<sbyte> whereas we lower it to sbyte* implicitly. This is like 2.X but better.
  • [ ] SAL attribute parsing into SymbolConstraints. Might be better done in ClangSharp itself and copying code from win32metadata code where possible.
  • [ ] Per https://github.com/dotnet/Silk.NET/pull/2020: Note for a future PR: add [Flags] back
  • [ ] We're not using in/out/ref as it currently stands for simplicity and keeping the overload count down, but we need to provide ample documentation and utilities for equivalent functionality

Priority 3 (preview 5/initial release)

  • [ ] Introduce an analyser that directs the user to the most efficient usage model for each binding i.e. instance IGL for OpenGL, static Sdl for SDL, etc...
    • [ ] Add an attribute to indicate this based on the "static default" vtable
  • [ ] Generate actual interfaces for COM interface structs i.e. ID3D11Device becomes D3D11Device representing the actual void*** handle (this shall implement IComVtbl), and ID3D11Device is generated as an actual interface to allow C# types to implement COM types.
    • [ ] Ideally not using ComWrappers, consult @Perksey for a brain dump
    • [ ] Ideally the user experience is just "implicit cast to the handle type, SilkMarshal will handle the rest".
    • [ ] Implement a demo for the #1951 use case
  • [ ] Caps9.VertexShaderVersion doesn't seem to be as simple as just a uint (it seems to be similar to a Version32[?]) Should we provide utilities for this?
  • [ ] Provide Throw/Assert/Expect extension methods on result-like types.
  • [ ] Investigate the other miscellaneous improvements outlined in https://github.com/dotnet/Silk.NET/issues/887#issuecomment-2127867602

Priority 4 (initial release or following initial release)

  • [ ] Add a Roslyn code completion provider that recurses into each T of a duck type such as Constant.
  • [ ] Add a ReSharper/Rider plugin that implements the same code completion behaviour.

Priority 5 (stretch goal, likely future release)

  • [ ] Introducing Object-suffixed classes to enable C# classes to mirror OOP-style C functions. e.g. instead of NamedBufferData, you could do BufferObject.BufferData.
    • The consensus on the maintainers team is that this doesn't add much to the bindings, but it does help make the library feel more at home with the rest of your C# code. Vulkan is probably going to be the primary use case for this.
  • [ ] Overload functions like vkEnumerate, DXGI's EnumAdapter etc to return enumerables.

Perksey avatar Apr 17 '22 18:04 Perksey

VkInstance and VkDevice handling behind the scenes in Vk.

Extensions. It's kinda... complicated? There's some generic stuff, KhrSurface vs SurfaceKHR etc

SupinePandora43 avatar Apr 17 '22 18:04 SupinePandora43

VkInstance and VkDevice handling behind the scenes in Vk.

This will be removed in favor of handle groups, but is a pain point in 2.x

There's some generic stuff, KhrSurface vs SurfaceKHR etc

This is reasonable feedback, but it's unclear what we can do here, this is effectively upstream khronos naming

HurricanKai avatar Apr 17 '22 18:04 HurricanKai

Currently on OpenGL there are several inconsistencies with what enum you can use with what function. For example gl.TexImage2D does not accept GLEnum for all of its enum parameters.

I do agree that it may not be a great idea if you generate overloads for every single relevant enum there is with all of the combinations, but I think that being able to use GLenum everywhere would be nice, since it contains every native value (or atleast it should) and the other OpenGL relevant enums are just a subsets of it.

roeyskoe avatar Apr 18 '22 11:04 roeyskoe

There are several instances of methods that have ridiculous amounts of overloads due to the sheer number of parameters and combinations of ptr/ref/span permutations also in combination with for example the GLEnum vs. enum types. TexImage2D for example has 48 different combinations. I've seen certain methods with upwards of 100 overload options. It may be beneficial to limit overloads by a common type - e.g. either all by ptr, all by ref or all by span if possible.

This would also solve something I wouldn't mind seeing - which would be allowing the entire api to be usable inside a safe context if so desired. Currently certain methods are completely unavailable outside of unsafe - and arguably there may be certain ones that would be extremely difficult to craft into a safe context. (I'm looking at you, Vulkan!) (e.g all ref/out or span params)

desiwko avatar Apr 21 '22 21:04 desiwko

I do agree that it may not be a great idea if you generate overloads for every single relevant enum there is with all of the combinations, but I think that being able to use GLenum everywhere would be nice, since it contains every native value (or atleast it should) and the other OpenGL relevant enums are just a subsets of it.

Would it not be preferable to just eliminate GLEnum and strongly type the correct enum everywhere?

HurricanKai avatar Apr 21 '22 21:04 HurricanKai

This would also solve something I wouldn't mind seeing - which would be allowing the entire api to be usable inside a safe context if so desired. Currently certain methods are completely unavailable outside of unsafe - and arguably there may be certain ones that would be extremely difficult to craft into a safe context. (I'm looking at you, Vulkan!) (e.g all ref/out or span params)

There has been some discussion about the number of overloads in regard to 3.0 (-> we want to further improve overloads with better overloads) I actually kind of like your idea, we could look to just generate one or a handful of safe methods + the pure native signature. If we do a really good job with safety I think most users would not miss the partially safe overloads. Seems like a thing to think about for sure!

HurricanKai avatar Apr 21 '22 21:04 HurricanKai

Would it not be preferable to just eliminate GLEnum and strongly type the correct enum everywhere?

For OpenGL specifically, I would not be happy doing this unless someone from our team audited the entire OpenGL specification and ensured that the groups are correct and sane, and committed to reviewing all changes thereafter to ensure that remains the case. The strong typing situation has been a mess ever since the demise of Silicon Graphics, as they stopped being properly maintained at that point. Everything thereafter is best effort.

Perksey avatar Apr 21 '22 22:04 Perksey

Would it not be preferable to just eliminate GLEnum and strongly type the correct enum everywhere?

For OpenGL specifically, I would not be happy doing this unless someone from our team audited the entire OpenGL specification and ensured that the groups are correct and sane, and committed to reviewing all changes thereafter to ensure that remains the case. The strong typing situation has been a mess ever since the demise of Silicon Graphics, as they stopped being properly maintained at that point. Everything thereafter is best effort.

I haven't looked at the spec in a really, really long time, but aren't new enums occasionally added specifically for EXT and ARB extensions? I think in cases like that you may want to still use the raw enums instead of encapsulating potentially constantly changing enum definitions. I know I'd personally like the option to use enums for those just-in-case moments.

desiwko avatar Apr 21 '22 23:04 desiwko

Replace void* with nuint in Gl.VertexAttribPointer

SupinePandora43 avatar Apr 27 '22 04:04 SupinePandora43

TODO

We truly invite everyone to send us reports of any place where our bindings are not ideal. This issue will serve as a list of things we should fix in 3.0.

the ENTIRETY of dx11

Beyley avatar Apr 27 '22 04:04 Beyley

separation of dsa and non dsa? Eg

interface BaseGL {
    void UseProgram(uint id);
}
interface DSA {
    void NamedBufferData(uint id, ...);
}
interface DSALess {
    void BufferData(...);
}

SupinePandora43 avatar Apr 27 '22 05:04 SupinePandora43

the ENTIRETY of dx11

That’s not helpful feedback :P what’s not ideal about them?

Perksey avatar Apr 27 '22 06:04 Perksey

the ENTIRETY of dx11

That’s not helpful feedback :P what’s not ideal about them?

mostly the UUID shenanigans, see Vortice

ID3D11Texture2D backBuffer = this._swapChain.GetBuffer<ID3D11Texture2D>(0);

vs Silk.NET

ComPtr<ID3D11Texture2D> backBuffer;
_swapChain.Get().GetBuffer(0, SilkMarshal.GuidPtrOf<ID3D11Texture2D>(), ref backBuffer.Handle); //i dont know if this actually compiles, but i yoinked it from a line you posted in discord perskey so im going to assume it does

while its usable, its quite less sharpie and a bit harder to read

Beyley avatar Apr 27 '22 06:04 Beyley

i think this little code example of me once trying to use silk.net d3d11 illustrates how to me, unusable they are:

void* factoryOut = null;
//you need to get GUIDs for alot of stuff
Guid dxgiGuid = typeof(IDXGIFactory2).GUID;

dxgi.CreateDXGIFactory2(0, ref dxgiGuid, ref factoryOut);
//you need to cast this to be correct aswell..
this._iDxgiFactory = (IDXGIFactory2*) factoryOut;

//then this, this is just horrifying to me, and is the only way i knew how to get a swapchain going
//Gotta get the pointer to a IUnknown pointer for the device because thats what CreateSwapChainForHwnd wants
fixed (IUnknown* device = &this._device) {
    this._iDxgiFactory->CreateSwapChainForHwnd(device, window.Native.Win32.Value.Hwnd, ref swapChainDesc, ref fullscreenDesc, null, ref this._swapChain);
}

not a huge fan of having to get alot of GUIDs just to be able to create something like a texture or factory, and the pointer shenanigans just make it really unpleasant to use, yes ComPtr exists but it doesn't eliminate almost any of that, its just as if i were writing c++ but with c#, it just doesnt work

Eeveelution avatar Apr 27 '22 11:04 Eeveelution

Yeah the DX stuff certainly needs fixing - I think that work is inline with function groups. Additionally we should look at overloads for methods that take a single GUID* like GetBuffer above, maybe creating a generic parameter, and replacing the GUID* with a call to SilkMarshal.GuidPtrOf<T>() 🤔

HurricanKai avatar Apr 28 '22 10:04 HurricanKai

While yes, you can do that, it still feels weird, especially the

fixed (IUnknown* device = &this._device)

I can live with the UUIDs but thats just ridiculous, and I think alot of the API has that, which is why currently I'm using vortice instead of silk for my d3d11 needs

Eeveelution avatar Apr 28 '22 10:04 Eeveelution

Not an DX expert, but I don't see how we can do anything about that? That's just what it looks like when you store IUnknown in a field and need a pointer to it later. We can of course create ref overloads, which makes this much nicer, but retrieving the IUnknown from a GC managed location will always require some form of pinning

HurricanKai avatar Apr 28 '22 10:04 HurricanKai

I mean, I don't know how Vortice does it, but it has to be possible somehow if they've done it right? although I don't know if they regenerate their bindings often, which Silk does, which might hinder smth like this, the thing I'm looking for is making the API more natural, something like vortice, and taking a pointer to a pointer and casting that to a IUnknown pointer then using that is far from natural

Eeveelution avatar Apr 28 '22 10:04 Eeveelution

Technically if we're talking native C code, it IS natural to do that with COM. Nothing wrong with it from the purist standpoint, but you are right in that it's not exactly intuitive to use even in C++ anymore.

I haven't played with it at all, but how did XNA wrap Direct3D? There may be some hints there that could be leveraged to making a more sane managed interface to D3D.

On Thu, Apr 28, 2022, 5:52 AM Furball @.***> wrote:

I mean, I don't know how Vortice does it, but it has to be possible somehow if they've done it right? although I don't know if they regenerate their bindings often, which Silk does, which might hinder smth like this, the thing I'm looking for is making the API more natural, something like vortice, and taking a pointer to a pointer and casting that to a IUnknown pointer then using that is far from natural

— Reply to this email directly, view it on GitHub https://github.com/dotnet/Silk.NET/issues/887#issuecomment-1112061637, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFMMZU2TJUVDFBWDESJRK4DVHJUWPANCNFSM5TUFZQ2A . You are receiving this because you commented.Message ID: <dotnet/Silk. @.***>

desiwko avatar Apr 28 '22 11:04 desiwko

I entirely agree that we can improve things here, using ref for cases like this seems like a good idea. I just mean that the fixed statement specifically is just an artifact of using pointers with managed fields.

HurricanKai avatar Apr 28 '22 17:04 HurricanKai

SDL bidings tend to not match up the Enums with their respective functions very well, only doing it sometimes spirattically

Beyley avatar May 04 '22 02:05 Beyley

We’ll solve this in 3.0 by removing SDL

Perksey avatar May 04 '22 06:05 Perksey

glDebugMessageInsert has no string overload

Beyley avatar May 06 '22 05:05 Beyley

glDebugMessageCallback doesn't have delegate* overload

SupinePandora43 avatar May 09 '22 07:05 SupinePandora43

NamedBufferSubData and NamedBufferData are missing ReadOnlySpan<> overloads

Khitiara avatar May 17 '22 20:05 Khitiara

I'm not entirely sure how the bindings work currently in this regard, but how about automatic error checking for all the Vulkan functions which return a Result? For example Vulkan-Hpp does that, and only returns Results for functions where it makes sense to check for anything else other than Succcess, like with AcquireNextImage.

Vazde avatar May 22 '22 08:05 Vazde

Use safe fixed size buffers (https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md)

SupinePandora43 avatar May 31 '22 06:05 SupinePandora43

The GLFW bindings seem to be missing the {Get,Set}WindowUserPointer functions, despite having the equivalents for Monitor and Joystick.

nike4613 avatar Jun 03 '22 21:06 nike4613

The function definition for PfnDebugUtilsMessengerCallbackEXT in vulkan is incorrect, it should return a Boolean32, not a uint

Beyley avatar Jun 17 '22 17:06 Beyley

the DebugUtilsMessageSeverityFlagsEXT enum is named very redundantly, with fields like DebugUtilsMessageSeverityFlagsEXT.DebugUtilsMessageSeverityVerboseBitExt and DebugUtilsMessageSeverityFlagsEXT.DebugUtilsMessageSeverityInfoBitExt same with other EXT ones like DebugUtilsMessageTypeFlagsEXT

Beyley avatar Jun 17 '22 17:06 Beyley