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

Tracking Issue for Bindings Generation & Improvements #887

Open
Perksey opened this issue Apr 17, 2022 · 53 comments
Open

Tracking Issue for Bindings Generation & Improvements #887

Perksey opened this issue Apr 17, 2022 · 53 comments
Assignees
Labels
Milestone

Comments

@Perksey
Copy link
Member

Perksey commented Apr 17, 2022

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.

@Perksey Perksey added help wanted Extra attention is needed feature New feature. area-SilkTouch tenet-Usability labels Apr 17, 2022
@Perksey Perksey added this to the 3.0 milestone Apr 17, 2022
@SupinePandora43
Copy link

VkInstance and VkDevice handling behind the scenes in Vk.

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

@HurricanKai
Copy link
Member

HurricanKai commented Apr 17, 2022

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

@roeyskoe
Copy link
Contributor

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.

@desiwko
Copy link

desiwko commented Apr 21, 2022

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)

@HurricanKai
Copy link
Member

HurricanKai commented Apr 21, 2022

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
Copy link
Member

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!

@Perksey
Copy link
Member Author

Perksey commented Apr 21, 2022

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.

@desiwko
Copy link

desiwko commented Apr 21, 2022

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.

@Perksey Perksey pinned this issue Apr 22, 2022
@SupinePandora43
Copy link

Replace void* with nuint in Gl.VertexAttribPointer

@Beyley
Copy link
Contributor

Beyley commented Apr 27, 2022

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

@SupinePandora43
Copy link

separation of dsa and non dsa?
Eg

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

@Perksey
Copy link
Member Author

Perksey commented Apr 27, 2022

the ENTIRETY of dx11

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

@Beyley
Copy link
Contributor

Beyley commented Apr 27, 2022

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

@Eeveelution
Copy link
Contributor

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

@HurricanKai
Copy link
Member

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>() 🤔

@Eeveelution
Copy link
Contributor

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

@HurricanKai
Copy link
Member

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

@Eeveelution
Copy link
Contributor

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

@desiwko
Copy link

desiwko commented Apr 28, 2022 via email

@HurricanKai
Copy link
Member

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.

@Beyley
Copy link
Contributor

Beyley commented May 4, 2022

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

@Perksey
Copy link
Member Author

Perksey commented May 4, 2022

We’ll solve this in 3.0 by removing SDL

@Beyley
Copy link
Contributor

Beyley commented May 6, 2022

glDebugMessageInsert has no string overload

@Khitiara
Copy link
Contributor

NamedBufferSubData and NamedBufferData are missing ReadOnlySpan<> overloads

@Vazde
Copy link

Vazde commented May 22, 2022

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.

@SupinePandora43
Copy link

@nike4613
Copy link

nike4613 commented Jun 3, 2022

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

@Beyley
Copy link
Contributor

Beyley commented Jun 17, 2022

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

@Beyley
Copy link
Contributor

Beyley commented Jun 17, 2022

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

@Perksey
Copy link
Member Author

Perksey commented Jun 17, 2022

@Beyley this is fixed in #948 and by extension 2.16

@Beyley
Copy link
Contributor

Beyley commented Jun 17, 2022

@Beyley this is fixed in #948 and by extension 2.16

o

@HurricanKai HurricanKai mentioned this issue Jun 20, 2022
43 tasks
@Khitiara
Copy link
Contributor

OpenAL is missing span overloads in many, many places. BufferData, SourceQueue/UnqueueBuffers in particular are sore spots

@Perksey
Copy link
Member Author

Perksey commented Jun 30, 2022

Our OpenAL bindings are completely handwritten in 2.X (and haven't really been updated since the first or second preview of 1.0!) so for 2.X - contributions welcome :), but imo in 3.0 we'd be stupid not to have OpenAL autogenerated as well given our C/C++ toolchain will be a lot more mature.

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

D3D9 PresentParameters.Windowed should be a bool, not an int
https://i.beyleyisnot.moe/sUPW4dw.png

@Perksey
Copy link
Member Author

Perksey commented Aug 11, 2022

It should probably be a Bool32 given that bool is a managed type but I agree otherwise

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

D3D9 Swapeffect enum should be called SwapEffect

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

D3D9 Theres no enums generated for the ptexturecaps stuff
https://i.beyleyisnot.moe/WgFgihI.png

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

Theres no overloads on a lot of things for normal return types, requiring you to have to pass in a pointer to a struct instead
https://i.beyleyisnot.moe/ZhrilhQ.png

Also a lot of structs have a redundant 9 suffix that should be stripped out

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

SilkMarshal.PtrToString should have an overload for byte* (or is there a reason there isnt)

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

D3D9 missing devicecaps enum

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

Caps9.VertexShaderVersion doesnt seem to be as simple as just a uint (it seems to be similar to a Version32[?])

@Beyley
Copy link
Contributor

Beyley commented Aug 11, 2022

D3D9 Missing ClearFlags enum

@HurricanKai
Copy link
Member

Theres no overloads on a lot of things for normal return types, requiring you to have to pass in a pointer to a struct instead

Pretty sure this is a symptom of 2.x having no access to SAL stuff. Talked to @Perksey about this few days ago, and looks like we can make this happen for 3.0

@Beyley
Copy link
Contributor

Beyley commented Nov 23, 2022

D3D11 seems to be missing the DXGI_USAGE enum, well, its not an enum, but it should be in our bindings, maybe there should be a way to define a prefix as being treated as an enum?

@Beyley
Copy link
Contributor

Beyley commented Nov 23, 2022

D3Dcolorvalue is named wrong, should likely just be ColorValue

@Beyley
Copy link
Contributor

Beyley commented Nov 23, 2022

MapFlag.DONotWait is also named wrong, should be MapFlag.DoNotWait

@Beyley
Copy link
Contributor

Beyley commented Dec 11, 2022

CL.EnqueueNdrangeKernel should be named EnqueueNDRangeKernel

@nike4613
Copy link

nike4613 commented Feb 9, 2023

The GLFW bindings are missing GetWindowContentScale, despite having GetMonitorContentScale. (Unfortunately GetMonitorContentScale cannot be used instead, because some platforms don't seem to return a Monitor.)

GLFW bindings are also missing several members of the CursorShape enum, notably: RESIZE_ALL, RESIZE_NESW, and RESIZE_NWSE. (Interestingly, the GLFW online documentation doesn't define these, but ImGui's GLFW backend references them.) Looking more into that, those seem to me new in GLFW 3.4...

@RealityProgrammer
Copy link

RealityProgrammer commented May 27, 2023

  1. Silk.NET.Direct3D12.DescriptorRangeFlags.DataStaticWhileSetATExecute should not capitalize the T in AT
  2. Silk.NET.Direct3D12.RootParameterType's shorten name of the enum is not sufficient enough (enum members still have the Type prefix) A slight problem: 32-bit constants enum, if shorten then the enum name will start with a number, which is not valid.
  3. Rework on ref and out modifier based on SAL annotation (eg: it should be ID3D12ShaderReflection.GetResourceBindingDesc(uint, out ShaderInputBindDesc) instead of ref)
  4. Some API should NOT accept a Span overload (eg: QueryInterface)
  5. Update Assimp's new post-process flags.
  6. Utilize some enumerate instead of generating backing type if possible (eg: BufferDesc.CpuAccessFlags being uint instead of Silk.NET.Direct3D11.CpuAccessFlags)

@sgf
Copy link

sgf commented Nov 30, 2023

Silk.NET.Direct2D.Enum.LawnGreen to D3Dcolorvalue

I'm not good at using c++, or even this smart pointer thing. Even though a lot of people say smart pointers are great.
I don't understand when I should use ComPtr and when I shouldn't use it, whether it should be used in the parameters of the function, or whether it should not be used. Is it possible to define two ComPtr fields in two types to save the same T object? . Questions like this. In short, I feel that I seem to be stuck in the complexity of C++. The joy of writing C# is gone. And the C# code looks even worse than the C++ code.

For this I now have to switch to SharpDx.

@Perksey
Copy link
Member Author

Perksey commented Dec 13, 2023

Thanks for all the feedback, please do keep it coming! We are actively looking into 3.0 now.

@sgf
Copy link

sgf commented Dec 13, 2023

Regarding GLFW, I am not opposed to using it, but I think it should be port(port to pure csharp) instead of binding(wrapper).

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

No branches or pull requests