floooh / sokol-zig

Zig bindings for the sokol headers (https://github.com/floooh/sokol)
zlib License
375 stars 48 forks source link

More reflection! #80

Open Interrupt opened 2 months ago

Interrupt commented 2 months ago

So far when building shaders with --reflection for C and Zig it does not make enough functions to be able to build a complete list of stuff like the names and offsets of all of the uniforms in a Uniform Block.

The easy path here would be to add more reflection functions, like one where you could ask for the names of all of the uniforms in a given uniform block. Then you could loop through and use the other reflection functions to get the offsets and such.

Another path here would be to just include a YAML version of the shader inside the built shader file, and let people parse that themselves. That would let people do similar shader reflection parsing between both bare_yaml files and other built in shaders.

Do you have a preference on which way you would prefer for new PRs?

floooh commented 2 months ago

I'll just throw a couple of thoughts in here for now, because I've been thinking about similar stuff while working on the resource-bindings-cleanup branch (https://github.com/floooh/sokol/tree/issue1037_bindings_cleanup)

The good news is that all of this are breaking changes anyway, so we don't need to hold back with new ideas (like replacing the current set of runtime reflection functions with a struct).

I would suggest that you could create an experimental PR where we can check the 'look and feel' of such a new way to return reflection information, but to hold back that PR until the resource bindings cleanup is nearing completion (and then maybe integrate that PR into the bindings cleanup branch - there will also be one for sokol-shdc, currently there are only branches in the sokol and sokol-samples repo, and I'm very much at the beginning of that work still).

PS: I would try to avoid adding a YAML text blob into the generated code, I think it would be better to use a struct instead.

Interrupt commented 2 months ago

Yeah, having a fully populated reflection info struct for shaders would be the most ideal and really straightforward to work with - the reflection functions currently are hard to work with when you also don't have the full list of names to pass into them.

It's the uniform blocks currently that are tripping me up. Ideally I could grab the size of a uniform block, allocate that memory, and then lookup the uniform offsets by name to write into that data, ignoring uniforms that don't exist.

Interrupt commented 2 months ago

Looking into this some, it does seem like making a whole new set of structs just for the Shader Reflection info seems like a lot of duplication when so far it looks like all of these structs have the required info:

ShaderAttrDesc
ShaderUniformBlockDesc
ShaderStorageBufferDesc
ShaderImageDesc
ShaderSamplerDesc
ShaderImageSamplerPairDesc
ShaderStageDesc
ShaderDesc

And this one is missing just the offset: ShaderUniformDesc

Does it make sense to add the offset field to ShaderUniformDesc and re-use that same set of structs between both shader creation as it is now, and for the fully populated reflection info? That way you don't need to keep changes in sync between both when / if things change.

floooh commented 2 months ago

That was exactly my thinking when I thought about integrating the reflection info into the shader-desc struct, but after the bindings cleanup those structs will drop some information (like any string names), and other structs would need to be extended with information that's only useful for user code as reflection info, but not needed in sokol-gfx for resource binding.

That's why I think it's better to build a separate reflection info struct, even if it partly overlaps with the shader-desc structs.