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

Extension points in builtin shaders #569

Open
5 of 8 tasks
mosra opened this issue Jun 12, 2022 · 3 comments
Open
5 of 8 tasks

Extension points in builtin shaders #569

mosra opened this issue Jun 12, 2022 · 3 comments

Comments

@mosra
Copy link
Owner

mosra commented Jun 12, 2022

Discussed on Gitter a while ago, putting it here to avoid forgetting the details.

The main idea is that vertex (fragment, geometry...) shaders would contain various extension placeholders using macros. At the very least one for defining extra uniforms / inputs / outputs, and one inside the main function, with actual code.

#ifdef VERTEX_SHADER_INTERFACE_EXTENSION
VERTEX_SHADER_INTERFACE_EXTENSION
#endif

void main() {
    ...
    #ifdef VERTEX_SHADER_MAIN_EXTENSION
    VERTEX_SHADER_MAIN_EXTENSION
    #endif
}

Then, the user would have an option to subclass the shader and supply an additonal file to the base class, which defines a subset of these macros, such as:

#define VERTEX_SHADER_INTERFACE_EXTENSION \
    uniform(location = 69) in float fancyFactor;
#define VERTEX_SHADER_MAIN_EXTENSION \
    gl_Position.z *= fancyFactor;

This file would be inserted as the very first (after the #version directive), to allow also enabling various extension in it:

#extension GL_ARB_bindless_texture: require

#define VERTEX_SHADER_INTERFACE_EXTENSION \
    ...

Similar approach is taken in Babylon.js: https://github.com/BabylonJS/Babylon.js/blob/35eb617f377d67383e70e263e8a19596d80b5069/packages/dev/core/src/Shaders/default.fragment.fx

What's left to figure out:

  • Figure out a nice way to supply the extra file. Adding yet another positional argument to each shader constructor doesn't scale and every such addition causes a breaking change for users, so the constructors should probably have some ShaderConfiguration class supplied, like apps or Vk APIs do -- c5abcf5
    • Could this configuration be shared between GL and Vulkan to avoid code duplication? Probably not, at the very least the flags are different (Flag::UniformBuffers are implicit on Vulkan).
  • What to do if something has to be done before constructing the parent class? NoCreate and then *this = BaseShader{actualConstructorParameters}; in the constructor could work, but is kinda nasty. Can be done by overrriding compile() from Async GL shader compilation #534 instead.
  • On nasty old GL platforms, new attributes have to be bound after compiling and before linking. Make extension points impossible there? Could be doable by overriding compile() from Async GL shader compilation #534 and calling submitLink() again after setting up the attributes.
  • Code that has to be run after the shader is compiled & linked is easy, just put that into the derived class constructor.
  • Can something like this be done for SPIR-V? At the very least there could be a possibility to override the supplied SPIR-V binary, and the overriden binary could be generated pretty much the same as with runtime GLSL compilation, only through an offline tool.
    • Might need to extend GlslangShaderConverter with an ability to do "implicit includes" similarly to GCC's -include option, doing all this via a -DVERTEX_SHADER_INTERFACE_EXTENSION=... passed to magnum-shaderconverter would be PAINFUL.
    • Or #ifdef MACRO #include MACRO? Does that work in glslang?
@mosra mosra added this to TODO in Debugging and prototyping via automation Jun 12, 2022
@hsdk123
Copy link
Contributor

hsdk123 commented Sep 8, 2022

I'm not sure how do-able this would be, but would also be great if there was an easy way to just completely replace ex. the main() function in the shaders.

@mosra
Copy link
Owner Author

mosra commented Sep 8, 2022

I merged #576 yesterday, which resolves the major blockers here (see the updated list above). So this is getting closer to being doable ;)

@hsdk123
Copy link
Contributor

hsdk123 commented Sep 8, 2022

Cool! Hoping #506 can be picked up as well once you free up.

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

No branches or pull requests

2 participants