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

Should Silk.NET.Shaderc depend on Silk.NET.Shaderc.Native? #2007

Closed
alexrp opened this issue Apr 2, 2024 · 11 comments · Fixed by #2011
Closed

Should Silk.NET.Shaderc depend on Silk.NET.Shaderc.Native? #2007

alexrp opened this issue Apr 2, 2024 · 11 comments · Fixed by #2011

Comments

@alexrp
Copy link
Collaborator

alexrp commented Apr 2, 2024

I just happened to notice that Silk.NET.Shaderc does not pull in Silk.NET.Shaderc.Native as a dependency.

Obviously I can easily reference the latter myself, but it seems to usually be the case that the managed Silk.NET packages pull in their native counterparts as dependencies, so I wonder if this is intentional or just an oversight?

@alexrp
Copy link
Collaborator Author

alexrp commented Apr 2, 2024

I guess I have a similar question for Silk.NET.Vulkan and Silk.NET.Vulkan.Loader.Native. 🤔

@aquagoose
Copy link
Contributor

It makes sense not to depend on the native package. It's a similar reason as to why the OpenAL native package is separate too.

Many systems (especially on the Linux side) already contain the vulkan, spirv, openal etc libraries, so there's no real point in including the native packages.

And also, some people like to provide their own versions of the libraries instead.

Doing it this way gives the developer a choice on what to do, rather than it being forced by the library itself.

@Perksey
Copy link
Member

Perksey commented Apr 2, 2024

This is a very good question. On one hand, we prefer the route of least surprise, which would be an argument for referencing the native package (everywhere except Linux that is, where it's expected that it's on the system or otherwise provided by a package manager). On the other hand, we don't want users who aren't using the native binaries to have to pull them in - they're beefy packages.

It's worth noting for Vulkan specifically Khronos are very explicit in expecting this to be system-provided - we only introduced the loader native package for MoltenVK originally.

I don't know where I stand here, because things like Assimp reference their native packages. @Beyley what do you think? cc @Khitiara as you're good at turning tech feelings and decisions into comprehensible sentences.

@alexrp
Copy link
Collaborator Author

alexrp commented Apr 2, 2024

Yeah, I think there are good arguments both ways here.

For my part, I value consistency very highly. If that means not referencing native packages from any managed packages by default, I'm totally okay with that too. Perhaps it might be possible to have Silk throw an exception nudging users towards the relevant .Native package if the library couldn't be loaded. Or something. 🤷

@Beyley
Copy link
Contributor

Beyley commented Apr 3, 2024

This is a very good question. On one hand, we prefer the route of least surprise, which would be an argument for referencing the native package (everywhere except Linux that is, where it's expected that it's on the system or otherwise provided by a package manager). On the other hand, we don't want users who aren't using the native binaries to have to pull them in - they're beefy packages.

It's worth noting for Vulkan specifically Khronos are very explicit in expecting this to be system-provided - we only introduced the loader native package for MoltenVK originally.

I don't know where I stand here, because things like Assimp reference their native packages. @Beyley what do you think? cc @Khitiara as you're good at turning tech feelings and decisions into comprehensible sentences.

Most linux distros do not package shaderc, spirv-reflect or spirv-cross in shared library form (and obviously no other OS does at all either), so it makes sense we should reference the native package by default for these.

@alexrp
Copy link
Collaborator Author

alexrp commented Apr 5, 2024

Just noting that I'm happy to send a PR to fix up references once there's consensus and a decision from the maintainers on which managed packages should reference their native counterpart. Might be a good time to check all the published packages and make all the adjustments in one go?

FWIW, here are some of my thoughts, incorporating comments in this issue so far:

  • Ultz.Native.Assimp: Currently referenced by Silk.NET.Assimp. Seems reasonable? I don't think you'd expect people to have this library installed generally. Side note: Seems to be missing win-arm64 binaries (but I don't know if anyone cares).
  • Ultz.Native.GLFW: Currently referenced by Silk.NET.GLFW. Ditto.
  • Ultz.Native.SDL. Currently referenced by Silk.NET.SDL: Ditto.
  • Silk.NET.DirectStorage.Native: Currently referenced by Silk.NET.DirectStorage. I'm not familiar enough with this API to know if this makes sense. I don't seem to have the DLLs in my Windows 11 drive's system directory, though, so... probably?
  • Silk.NET.DXVK.Native: Not referenced anywhere. Seems reasonable; it's for pretty niche scenarios. Side note: What's the purpose of the Linux binaries in this one?
  • Silk.NET.MoltenVK.Native: Not referenced anywhere. Seems reasonable; like DXVK, I imagine it's pretty niche. Side note: Is it intentional that this doesn't include binaries for osx-x64 and/or osx-arm64?
  • Silk.NET.OpenAL.Soft: Not referenced by Silk.NET.OpenAL. In an abstract sense, I can sort of understand why. But to my knowledge, OpenAL Soft is the only implementation currently used by...everyone(?) on Windows/Linux. Windows doesn't ship with an implementation. Allegedly, Apple has an OpenAL implementation, but I don't have a Mac, so I can't confirm if that's there out of the box and actually useful. 🤷 My gut feeling is that referencing the native package would be the most helpful default, but I can see that being controversial here. (We should probably all just switch to Steam Audio.) Side note: The OpenAL Soft native package is missing osx-arm64 binaries, but does have osx-x64 binaries. An alternative here -- assuming the Apple implementation is good -- might be to just not ship macOS binaries in this package and then reference it from Silk.NET.OpenAL?
  • Silk.NET.OpenGLES.ANGLE.Native: I'm not particularly familiar with OpenGL ES, but some cursory searching seems to indicate that Windows doesn't have native support, so you want to use something like ANGLE. So on the surface, it seems like it would be reasonable for Silk.NET.OpenGLES to pull this in.
  • Silk.NET.Shaderc.Native: Per comments above, seems like Silk.NET.Shaderc should reference this.
  • Silk.NET.SPIRV.Cross.Native: Ditto for Silk.NET.SPIRV.Cross.
  • Silk.NET.SPIRV.Reflect.Native: Ditto for Silk.NET.SPIRV.Reflect.
  • Silk.NET.Vkd3d.Native: Not referenced anywhere. Seems reasonable for the same reason as DXVK.
  • Silk.NET.Vulkan.Loader.Native: Not referenced anywhere. Seems reasonable. Side note: Not clear to me why it has binaries for Windows/Linux if it was meant for MoltenVK specifically. 🤔 Are they actually useful for anything on Windows/Linux?
  • Silk.NET.Vulkan.SwiftShader.Native: Not referenced anywhere. Seems reasonable as this is once again a very niche thing.
  • Silk.NET.WebGPU.Native.WGPU: Not referenced anywhere. Seems like it should be pulled in by Silk.NET.WebGPU.

@alexrp
Copy link
Collaborator Author

alexrp commented Apr 5, 2024

@aquagoose

And also, some people like to provide their own versions of the libraries instead.

I feel like this is probably the minority case, but I agree it's a use case that should be accommodated. The good news is that people who care about this can just do something like:

<PackageReference Include="Silk.NET.GLFW" Version="2.20.0" ExcludeAssets="native" />

I just tested this with and without the ExcludeAssets. It has the effect of not pulling in the native GLFW binaries as you'd expect.

@Beyley
Copy link
Contributor

Beyley commented Apr 5, 2024

What's the purpose of the Linux binaries in this one?

These allow you to use D3D9/11 natively on linux (no wine) in Silk.NET, you just have to install the package and configure your GetApi call. Agree that it should be optional

Silk.NET.WebGPU.Native.WGPU: Not referenced anywhere. Seems like it should be pulled in by Silk.NET.WebGPU.

Given Dawn's recent advancements in spec compliance, i'd feel happier if we provided Dawn native libraries and made those the default, with WGPU as an option for those who want it. But that will be something a contributor can tackle, since thats quite a big task that i personally dont have the time for right now.

Silk.NET.OpenAL.Soft: Not referenced by Silk.NET.OpenAL.

it's of my opinion we should reference ALSoft for OpenAL, no one uses other implementations

Silk.NET.Shaderc.Native: Per comments above, seems like Silk.NET.Shaderc should reference this.
Silk.NET.SPIRV.Cross.Native: Ditto for Silk.NET.SPIRV.Cross.
Silk.NET.SPIRV.Reflect.Native: Ditto for Silk.NET.SPIRV.Reflect.

100% in agreement here.

Silk.NET.OpenGLES.ANGLE.Native: I'm not particularly familiar with OpenGL ES, but some cursory searching seems to indicate that Windows doesn't have native support, so you want to use something like ANGLE. So on the surface, it seems like it would be reasonable for Silk.NET.OpenGLES to pull this in.

It depends on the driver for OpenGLES support. All major drivers do support it though (anything which can do D3D12 can do GL/GLES in Windows 11 through a translation layer provided by the OS).

@Perksey
Copy link
Member

Perksey commented Apr 5, 2024

Side note: What's the purpose of the Linux binaries in this one?

DXVK is primarily used on Linux given that Linux does not readily have a DirectX implementation. The non-Linux binaries are provided for posterity.

Side note: Is it intentional that this doesn't include binaries for osx-x64 and/or osx-arm64?

Noone's added it yet. The particular community contributor was only interested in iOS at that time, so only added it for iOS and iOS-like platforms; expecting to be used with AOT.

Side note: Seems to be missing win-arm64 binaries (but I don't know if anyone cares).

OpenAL Soft is the only implementation currently used by...everyone(?) on Windows/Linux

The Creative implementation is still quite actively used and is most notably made available as an auto-installable prerequisite in Valve's Steamworks. In addition, there are other bona fide implementations (such as MojoAL which is OpenAL-on-SDL).

assuming the Apple implementation is good -- might be to just not ship macOS binaries in this package and then reference it from Silk.NET.OpenAL?

Apple's implementation is good in that it forwards to the happy path/native APIs for macOS, and can be assumed to be of high quality despite it being relatively unchanged since icculus originally wrote it. However, it does not include all of the other functionality that OpenAL Soft provides in their own extensions. It depends what the developer is after, and because of this choice I don't think we should pull this in by default.

(We should probably all just switch to Steam Audio.)

We continued the discussion wrt Steam Audio in our Discord. Notably Steam Audio doesn't actually play audio, it just encodes it, you still need something to receive the audio stream. We are undecided on how we want to proceed.

I'm not particularly familiar with OpenGL ES, but some cursory searching seems to indicate that Windows doesn't have native support

Windows can have native support. Most of the time one attempts to use OpenGLES on desktop explicitly because of ANGLE, which provides a relatively high quality trimmed down version of OpenGL implemented on top of other native APIs such as Direct3D, Vulkan, or Metal. More often than not it is used to create contexts in Direct3D-exclusive environments, or in environments where you can't trust the native OpenGL driver to be high quality. Notably all major browsers ship with ANGLE. ANGLE however is a minority implementation of OpenGLES, so it wouldn't make sense to pull this in by default.

Per comments above, seems like Silk.NET.Shaderc should reference this.

I agree, Shaderc will always be the only implementation of the Shaderc API in all use cases, so it makes sense to pull this in by default. Ditto for the others.

Side note: Not clear to me why it has binaries for Windows/Linux if it was meant for MoltenVK specifically

While we introduced it for MoltenVK, the loader functions on all platforms. The vulkan-1.dll that is in your Windows system folder is the Vulkan loader. It's useful if you are using non-standard deployments, e.g. if you want to deploy using SwiftShader or some other emulator within the Portability Initiative, where you don't want to assume anything about the underlying system.

Silk.NET.WebGPU.Native.WGPU: Not referenced anywhere. Seems like it should be pulled in by Silk.NET.WebGPU.

WGPU is not the only implementation of WebGPU, Dawn is also a very common implementation. In addition, in some cases the implementation included in Emscripten is used. Given that it is not the only implementation, and different implementations have differing appropriateness depending on needs (similar to OpenAL), I don't believe it should be pulled in by default.

@Perksey
Copy link
Member

Perksey commented Apr 5, 2024

Note that these are just my personal opinions as a non-maintainer. @Beyley is the maintainer for 2.X and, along with the other maintainers, makes these decisions.

@alexrp
Copy link
Collaborator Author

alexrp commented Apr 5, 2024

Thanks for the feedback and clarifications! I agree with most points raised.

It seems there's agreement on Shaderc, SPIRV.Cross, and SPIRV.Reflect, so I'll send a PR for those.

These allow you to use D3D9/11 natively on linux (no wine)

Didn't know DXVK supported that; that's pretty cool.

We continued the discussion wrt Steam Audio in our Discord. Notably Steam Audio doesn't actually play audio, it just encodes it, you still need something to receive the audio stream.

Ah, that's unfortunate. 😞 It seems like it still has some features (e.g. the physics-based propagation) that makes it worthwhile, though. (But I guess this is getting a bit off-topic.)

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

Successfully merging a pull request may close this issue.

4 participants