search

libsdl-org / SDL_shadercross

Shader translation library for SDL's GPU API.
zlib License
57 stars 18 forks source link

Support for static SPIRV-Cross, MSVC cleanup <> to "", void* warning #3

Closed RandyGaul closed 3 months ago

RandyGaul commented 3 months ago

Spent some time today trying this out with MSVC2022. I maintain a small 2D game creation framework and wanted to try out SDL_Gpu. There were a couple difficulties here since I statically linked SPIRV-Cross.

My understanding is the current implementation assumes SPIRV-Cross is installed by the user before trying out this header. Though, I don't want users of my framework to have to bother installing anything external, besides CMake/python.

#define SPIRV_CROSS_DLL "spirv-cross-c-shared.dll"

This line doesn't actually seem super helpful for anyone compiling from source, shared or not. I noticed the default behavior from SPIRV-Cross's cmake file was to created spriv-cross-c.dll, not spirv-cross-c-shared.dll, so I assume the -shared name came perhaps from a vcpkg install? Maybe you could help explain this bit. Let me know if you have thoughts here, I'm totally new to all this SPIR-V stuff and might be just a total dumb idiot here.

I decided to try adding SDL_GPU_SHADERCROSS_STATIC to eliminate all the function pointer loading, and this worked pretty well. It's an optional define similar to SDL_GPU_SHADERCROSS_IMPLEMENTATION.

I did swap #include<spirv_cross_c.h> to #include "spirv_cross_c.h", this makes it a bit easier to integrate all the headers together with a relative path here. I don't actually have #include<spirv_cross_c.h> on the path, so this include gives an error under MSVC2022.

The last bit were some minor warnings on assigning from void*, since C++ is a little pickier on type conversions here

flibitijibibo commented 3 months ago

That all makes sense to me - the dll names were picked based on what was provided in the Vulkan SDK. The idea was that the library from the SDK would be bundled, so no building of spirv-cross would be needed.

Will take a closer look on Monday, thanks for checking this out! I think the only thing I saw was // over /**/ comments but that's a nitpick at most.

thatcosmonaut commented 3 months ago

My understanding is the current implementation assumes SPIRV-Cross is installed by the user before trying out this header. Though, I don't want users of my framework to have to bother installing anything external, besides CMake/python.

No install is necessary, you just ship the SPIRV-Cross dll with your game/engine/whatever. Providing a path for static linkage does seem reasonable. The only potential downside is having to recompile if there are upstream improvements to SPIRV-Cross but we don't have to get in a whole debate about the tradeoffs of static vs dynamic linkage here.

RandyGaul commented 3 months ago

Makes sense, glad you're both in favor of supporting static linking. It can be a great simplification to not have to deal with disparate shared libs and tracking them. I'd also recommend to add in another option to specify the shared lib path. It's nice to have the default strings in there, but making it overrideable would be really nice. I'll just add this to the PR here.

I like how you guys architected SDLG_GpuCompileFromSPIRV to allow everyone to get rid of dependencies once icculus gets his shader tools going. Really solid technical decisions and planning there.

And thanks @thatcosmonaut for your blog post describing SPIR-V Reflect. I'm not used to the whole SPIRV stuff so that little bit of guidance really helped a lot for getting in a working online compilation flow.