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

CMake structure and CLI #27

Merged
merged 24 commits into from
Oct 27, 2024
Merged

CMake structure and CLI #27

merged 24 commits into from
Oct 27, 2024

Conversation

thatcosmonaut
Copy link
Collaborator

This project wasn't really structured like a single-header library anyway, so it can now build as a shared library with CMake. This should make it easier to use for interop.

It's also good for performance to offline compile shaders, so I added a CLI that will let you offline compile from HLSL and SPIRV to the formats accepted by the backends. The CLI is statically linked to SDL_gpu_shadercross and SDL, but it still dynamically loads SPIRV-Cross and the DirectX compiler DLLs.

TODO before merge: Add TranspileHLSLFromSPIRV.

Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay if I fixup the cmake project in this pr, or do you prefer after merging this pr?

include/SDL_gpu_shadercross.h Outdated Show resolved Hide resolved
include/SDL_gpu_shadercross.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@thatcosmonaut
Copy link
Collaborator Author

@madebr this is great. Do you want to just submit a patch for the CMake config? You know way more about build systems than me.

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

@madebr this is great. Do you want to just submit a patch for the CMake config? You know way more about build systems than me.

I can push to this repo (and to this PR). I just don't want to interfere with your work.

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

It looks like you created a cli branch on the "libsdl-org/SDL_gpu_shadercross" repo.
Please create temporary branches in your own repo, and create pull requests from there.

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

Issues/questions I have:

  • There are warnings on ci, so werror is disabled
  • Right now, it builds a SDL_gpu_shadercross library. Perhaps it should be SDL3_gpu_shadercross instead?

@thatcosmonaut
Copy link
Collaborator Author

Thanks for this. I'll push to a fork next time. Working on the warnings now.

@thatcosmonaut
Copy link
Collaborator Author

This is looking pretty good, just going to add the HLSL export now.

@thatcosmonaut
Copy link
Collaborator Author

@madebr Is there a way to specify that we want to link SDL to the CLI statically? I'd like to be able to distribute the executable standalone.

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

@madebr Is there a way to specify that we want to link SDL to the CLI statically? I'd like to be able to distribute the executable standalone.

Not currently. I'll add an option to make it opt-in.

@thatcosmonaut
Copy link
Collaborator Author

You're the best.

@thatcosmonaut thatcosmonaut marked this pull request as ready for review October 25, 2024 19:20
@flibitijibibo
Copy link
Collaborator

Will review this later tonight - @DanielGibson, how does the README look? If it's all good we should be able to close #18 with this PR too!

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

@slouken

* Right now, it builds a `SDL_gpu_shadercross` library. Perhaps it should be `SDL3_gpu_shadercross` instead?
Without 3 With 3
SDL_gpu_shadercross.dll SDL3_gpu_shadercross.dll
#include <SDL_gpu_shadercross/SDL_gpu_shadercross.h> #include <SDL3_gpu_shadercross/SDL_gpu_shadercross.h>

We should probably also start versioning with 3.0.0 instead of 1.0.0?

@madebr
Copy link
Contributor

madebr commented Oct 25, 2024

I went ahead with the rename. If unwanted, a revert is easy.

README.txt Outdated
This library also provides a command line interface for offline translation of shaders.

For SPIRV translation, this library depends on SPIRV-Cross: https://github.com/KhronosGroup/SPIRV-Cross
For compiling to DXIL, dxcompiler.dll and dxil.dll are required (or your platform's equivalent).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those also ship with Windows or else where do you get them?
Should the application using SDL_gpu_shadercross ship dxcompiler.dll and dxil.dll or should it expect the user to install some DirectX runtime?

Does it not need any additional libraries for macOS/Metal support?

(apart from this the README looks good to me :))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those also ship with Windows or else where do you get them? Should the application using SDL_gpu_shadercross ship dxcompiler.dll and dxil.dll or should it expect the user to install some DirectX runtime?

Added a note about this.

Does it not need any additional libraries for macOS/Metal support?

Nope, just SPIRV-Cross.

README.txt Outdated
@@ -8,6 +8,8 @@ This library also provides a command line interface for offline translation of s

For SPIRV translation, this library depends on SPIRV-Cross: https://github.com/KhronosGroup/SPIRV-Cross
For compiling to DXIL, dxcompiler.dll and dxil.dll are required (or your platform's equivalent).
These dependencies can be obtained in the Vulkan SDK: https://vulkan.lunarg.com/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, the Vulkan SDK for Windows doesn't include dxil.dll
Maybe one should use https://github.com/microsoft/DirectXShaderCompiler/releases instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're totally right, I must have put that dll in my own VulkanSDK dir. I'll add the link to the DXC release page.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we now have the symbol list we should probably change the header to always declare the whole API, and return errors when the compiled library doesn't enable the appropriate backend(s). That can be done in a later commit though, this is fine for now.

@madebr
Copy link
Contributor

madebr commented Oct 26, 2024

When configuring with -DENABLE_DEPS=ON (and also -DENABLE_INSTALL=ON), SDL3, spirv-cross and dxc is downloaded, eventually built and installed.

Check the artifacts: https://github.com/libsdl-org/SDL_gpu_shadercross/actions/runs/11535764041?pr=27

@madebr madebr mentioned this pull request Oct 26, 2024
4 tasks
@thatcosmonaut thatcosmonaut merged commit 62e7df2 into master Oct 27, 2024
8 checks passed
@thatcosmonaut thatcosmonaut deleted the cli branch October 27, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants