-
Notifications
You must be signed in to change notification settings - Fork 32
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
Make runtime shader compilation optional #251
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions:
- Is the shader compiler a standalone program? Which source is it exactly? I didn't want to spend time trawling the cmake file to find out. Is it just cute_shaderc.cpp and associated header?
- Could you outline here what the major individual build targets are exactly and outline their dependencies to each other?
- Can the shader compiler be linked in statically into cute.lib or cute.dll (static or dynamic libs, but not as a standalone exe), as a simple cmake option?
- Is this easy for users to toggle on/off in cmake? I want runtime shaders in CF on by default, and behind a single cmake option to turn them off and NOT compile or pull in any SPIRV nonsense. Is that the way things work here as of now?
- Do you have plans yet to add in a dedicated website page for documenting the compiler?
- Can the compiler output serialized SPIRV blobs directly, and not just C headers?
- Can the compiler output reflection info required for passing into SDL_Gpu (various input counts for specific resource sets)?
- I'd prefer only one or two samples to use bytecode. The rest should just use runtime shaders alone to reduce complexity.
CMakeLists.txt
Outdated
add_executable(timestep samples/timestep.cpp) | ||
add_executable(joypad samples/joypad.c) | ||
add_executable(outline_stencil samples/outline_stencil.cpp) | ||
add_executable(recolor samples/recolor.cpp) | ||
add_executable(recolor samples/recolor.cpp samples/recolor_data/recolor_shd.h) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the majority of sample code to use runtime string shaders. Precompiling is unneeded complexity when starting out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I made the changes just to check that all samples are fine with CF_GLSLANG=OFF
I can revert but those samples would crash when CF_GLSLANG=OFF
, is that acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's fine. As long as they all work with default settings when cloning, if the user turns it off I would expect them to then crash.
include/cute_draw.h
Outdated
* @remarks This can be created using the `cute-shaderc` compiler. | ||
* @related CF_Shader cf_draw_push_shader cf_draw_pop_shader cf_draw_peek_shader cf_make_draw_shader_from_bytecode | ||
*/ | ||
typedef struct CF_DrawShaderBytecode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline on brace to match the rest of CF. We will need a page on CF website documenting cute-shaderc
for sure. I can help get you going on that if you need -- it's really easy.
samples/hello_triangle_data/tri_fs.h
Outdated
@@ -0,0 +1,36 @@ | |||
#pragma once | |||
|
|||
static const uint8_t s_tri_fs_bytecode_content[448] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want, in a comment, the string representation of the shader just above the bytecode here. Perhaps the compiler should output this directly into the header. sokol-shdc does this and it's greatly appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Preprocessed shader or original code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably preprocessed
size_t size; | ||
}; | ||
|
||
class Includer: public glslang::TShader::Includer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, is this includer thing a part of the glslang API? I hate it, but looks like it's just a way to use glslang, which is accpetable and I'm glad you hid this all back here in the implementation in an isolated place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is glslang API to search for include file. It is passed to glslang:TShader::preprocess. glslang will call the includer to search for files.
I use that to implement lookup in the VFS, builtin shader include and also automatic include guard.
In the case of duplicated includes, the content is resolved into an empty string.
Would also be quite nice to document in the code as comment a rough plan on transitioning to SDL shader tools in the future, explaining how the current SPIRV handling is temporary until SDL evolves more thoroughly. |
Yes, it is just
First, all glslang and spriv targets are now unconditionally fetched.
When When
Yes. It is the
Not quite yet. SPIRV-tool is still needed regardless of options because I have only implemented bytecode compilation, not reflection.
Yes, I can do that.
It was possible earlier but I reverted it because draw shader, the most common case, is a couple of shaders.
I can add a
Not yet. Is the plan to distribute
Sure. I will only make changes to samples that already use shader in file and revert changes from samples with inline shaders. |
Tbh, I have no idea where SDL shader tool is heading. I'll add some comment on how it is a temporary solution. Shader syntax may change depending on what SDL_shader tool is doing. |
Thanks! Excellent. Edit: I'll get to the rest of your updates later. |
If we attach reflection info can we instead cut out SPIRV from CF runtime (other than SPIRV-Cross), unless online shader compilation is required? We can augment the bytecode blob struct with all necessary info. Thoughts? Just a few more structs to store extra reflection info for CF runtime, mainly uniform information. The shader compiler has all it needs to output prebuilt shaders and setup a little reflection table of information. It could be a static array of structs holding uniform information and counts. |
Yes, that's the plan. |
docs/topics/shader_compilation.md
Outdated
|
||
## Offline compilation | ||
|
||
If you have a lot of shaders and want to avoid runtime compilation overhead, CF provides its own offline compiller: `cute-shaderc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compiller typo
src/cute_shader/cute_shader.h
Outdated
#include <stddef.h> | ||
#include <cute_shader_bytecode.h> | ||
|
||
typedef struct cute_shader_define_t { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use CF_ style for structs, and brace on separate line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, for cute shader, it's not really part of CF's public API though. How should I name it:
CF_ShaderDefine
: Might clash.CF_CuteShaderDefine
: A bit long but possible.CSH_Define
: CuteShader define.CS_Define
: ButCS
is alreadycute_sound
How should I name the functions:
cute_shader_init
: The current.cf_shader_init
: May clash with the main CF.cf_cute_shader_init
: A bit long.csh_init
: Sounds like a shell (csh).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say just go with CF_ prefix
include/cute_shader_bytecode.h
Outdated
* @brief Information about an input of a vertex shader. | ||
* @related CF_ShaderBytecode | ||
*/ | ||
typedef struct CF_ShaderInputInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate brace line
src/cute_shader/cute_shaderc.cpp
Outdated
SHADER_TYPE_BUILTIN, | ||
} shader_type_t; | ||
|
||
static const char* parse_flag(const char* arg, const char* flag_name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brace on different line
Looking quite good, and nice job on the documentation. Let me know when you feel like it's ready for me to pull it down and try things out. |
Except for stylistic changes, which I'll get to later today, there should not be anymore implementation changes. |
Huge changes:
Builtin shaders have to be declared in src/cute_shader/builtin_shaders.h
As of this writing, windows build in CI is blocked by: actions/runner-images#10978.
Allso,(Implemented)#include
within thecf_shader_directory
is not yet resolved.I'm posting this early for feedbacks.