search

buildaworldnet / IrrlichtBAW

Build A World fork of Irrlicht
http://www.buildaworld.net
Apache License 2.0
122 stars 28 forks source link

Add SPIR-V introspection and GLSL downcompile for OpenGL #130

Closed devshgraphicsprogramming closed 5 years ago

devshgraphicsprogramming commented 6 years ago

Use SPIRV-Cross https://www.khronos.org/assets/uploads/developers/library/2016-vulkan-devday-uk/4-Using-spir-v-with-spirv-cross.pdf https://github.com/KhronosGroup/SPIRV-Cross

to enable some introspection for asset::ISPIR_VShader

SPIRV-Cross does have some overhead, so introspection data should be gathered only on demand through bool asset::ISPIR_VShader::initIntrospection()

OpenGL 4.5 backend and below will need this to down-compile SPIR-V back to GLSL and will have to define their own workarounds to push_constants and descriptor sets as per https://github.com/buildaworldnet/IrrlichtBAW/wiki/Vulkan-Limitations-due-to-OpenGL-Support

Depends on #129

devshgraphicsprogramming commented 6 years ago

Superseded by https://docs.google.com/document/d/1WA8ctunP2IWM-sdi2D26c7q2FvixvEKLQ7sVo1czXqs/edit?usp=sharing

devshgraphicsprogramming commented 5 years ago

Which is superseded by https://repository.genmymodel.com/devshgraphicsprogramming/VulkanShaders

Crisspl commented 5 years ago

I'll put these questions here

So ICPUShader might contain higher lang source (glsl) if it was loaded from glsl file and not spirv directly?

devshgraphicsprogramming commented 5 years ago

what's the difference between ICPUShader and ICPUSpecializedShader? isn't specialized shader an ISPIR_VProgram + specialization data? Why isn't ICPUSpecializedShader derivative of ICPUShader then?

Because we can make multiple specialized shaders from one non-specialized shader, remember that we want to have near 1:1 mapping with Vulkan. And in Vulkan the specialization constants are actually given when creating a Graphics/Compute Pipeline (a particular material). So only IGPUShader would have a native API object attached to it on Vulkan.

However OpenGL will not tolerate these shennanigans, and we will have to use introspection to emulate specialization constants ourselves (by transforming the GLSL with SPIRV-CROSS). So in this case there will be nothing except GLSL source code and some introspection data in an IGPUShader object, but the native OpenGL shader handle will actually be kept in IGPUSpecializedShader.

Also remember that one ISPIR_VProgram may contain bytecode for multiple ACTUAL shaders (finally we can use other entry point names than "main"), and have introspection data for all of them. So it would be unwise to roll it into one object together with ICPUShader.

Because we want 1:1 parity of asset and video objects because of our loaders, we should have both ICPU variants of shaders.

Or is ICPUShader a collection of ICPUSpecializedShader-s? (i.e. whole shader program for example vs+fs) Is it complete program or just one shader/stage?

ICPUShader is basically shader code without constant-folding, ICPUSpecializedShader is shader code with the necessary definitions of the constant values but not constant-folded yet.

So we want to support spir_v only as input for the engine (asset pipeline)? And optionally(?) VK GLSL (which gets compiled to spir_v) - that's what we'll use shaderc for? What about GL GLSL shader sources?

We want to support SPIR-V and VK GLSL[1,2] only, GL GLSL will not be accepted as input (only handled by us to transpile VK GLSL into GL GLSL). [1] We need to add (to the shader source) definitions of SPIR-V/GLSL extensions which are enabled on the instance (so that #ifdef actually works for extension-specific codepaths) [2] Also need to add an #include keyword support For OpenGL backend, we need to use introspection to specialize constants, translate push_constants into a uniform struct, change *Index built-in variable names to *ID, and reorder shader bindings to emulate descriptor sets.

Have a look at https://github.com/buildaworldnet/IrrlichtBAW/wiki/Vulkan-Limitations-due-to-OpenGL-Support

GL GLSL (under COpenGLDriver) is compiled into machine code by downcompiling spir_v code (ISPIR_VProgram) into GLSL code (with spirv cross) and then put into OpenGL to compile, right?

No, VK GLSL is augmented (see notes 1,2), compiled to spir_v, and that is the ICPUShader.

Then we have a choice of either: A) SPIR_V is manipulated and downcompiled to become almost-valid (still spec constants) GL GLSL which becomes IGPUShader, but is essentially just a GLSL char string plus spec. const. introspection. Then IGPUShader+SpecializationConstants make a single IGPUSpecializedShader. B) IGPUShader is basically ICPUShader (SPIR-V). Then IGPUShader+SpecializationConstants are manipulated (spec. const. emulation, etc.) and downcompiled to become valid GLSL which is fed into OpenGL to produce IGPUSpecializedShader.

In all cases IGPUSpecializedShader is basically a handle to a separable shader program.

Your choice of variant A or B should be guided by how easy it is to mess around with SPIR-V vs. GLSL, i.e. whether IGPUShader on OpenGL should be slightly altered SPIR-V awaiting a downcompile or GLSL with extra info about constants to specialize.

How should specialization be done? e.g.: Do introspection, change things in introspection data (bindings, wg size etc) and generate new shader based on modified introspection data?

In OpenGL yes, but where and how you introspect and modify is up to you (SPIR_V level, or GLSL).

What is VkSpecializedMapEntry (ICPUSpecializedShader::mSpecializations); [Google doesn't know about this]? Actually what is this mSpecializations member?

What range of the specialization backing ICPUBuffer you use as the specialization constant byte-value. https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/VkSpecializationMapEntry.html

Why ISPIR_VProgram needs to be an interface?

Because generating introspection data is slow, therefore optional. But it needs to kept together with the source-code (caching).

What is CVulkanShaderModule?

A wrapper around a VkShaderModule created via https://www.khronos.org/registry/vulkan/specs/1.1-extensions/man/html/vkCreateShaderModule.html

Are diagrams in the doc up to date? If so, further questions are needed. Especially the 2nd image.

We could contemplate introducing a new object, irr::asset::IShaderSpecializationInfo which would be used both to construct an ICPUSpecializedShader and an IGPUSpecializedShader from itself+I[CPU/GPU]Shader. It could make the code a lot clearer.

Also what about the 2nd image?

So ICPUShader might contain higher lang source (glsl) if it was loaded from glsl file and not spirv directly?

Exactly

Crisspl commented 5 years ago

Introspection (SPIRV-Cross calls it reflection) depends on entry point, so I suggest moving introspection interface to ICPUSpecializedShader since this is where entry point is defined. https://github.com/KhronosGroup/SPIRV-Cross/wiki/Reflection-API-user-guide#query-entry-points https://github.com/KhronosGroup/SPIRV-Cross/blob/master/spirv_cross.hpp#L316

Interface:

ISPIR_VProgram As for now just a wrapper over SPIR-V source contained in ICPUBuffer. Derivative of core::IReference Counted. Maybe should also be an asset?

ICPUShader Might be created from ISPIR_VProgram* or Vulkan GLSL source passed as std::string. In case of the latter, ISPIR_VProgram is created internally but its internal buffer really contains GLSL string. Then if ICPUShader::getSPIR_VProgram gets called and GLSL code, instead of SPIR-V, is present - GLSL is compiled to SPIR-V, old ISPIR_VProgram gets dropped and new one actually containing SPIR-V code is created.

GLSL->SPIR-V compilation overview:

IGPUShader Basically same thing as CPU equivalent, but may be created from SPIR-V source only.

ICPUSpecializedShader Consists of:

Allows to specialize specialization constants using SSpecializationConstants struct actually being pair of container (vector/unordered_set) of specialzations + backing buffer. Each specialization is defined by structure

struct SSpecializationMapEntry
{
    uint32_t specConstID;
    uint32_t offset;
    uint32_t size;
};

User, while specializing should know spec-constant's ID, then insert an entry into SSpecializationContants::container and write the value in SSpecializationConstants::buffer on offset established in the entry. Then SSpecializationConstants object goes to ICPUSpecializedShader constructor along with SPIR-V program, entry point, pipeline creation bit-flags and shader type/stage. ICPUSpecializedShader is also capable of introspection since introspection is dependent on entry point.

IGPUSpecializedShader Nothing really happens here in interface class because of huge differences between backends.

COpenGLSpecializedShader Constructor has same params as ICPUSpecializedShader. Possibly there should be another ctor from CPU equivalent. Constructor copies SPIR-V code, manipulates it, down-compiles to GLSL, second manipulation pass, feed it to OpenGL API and get GL object ID. Then both SPIR-V-copy and GLSL code can be freed. Code manipulation is divided into two passes (before and after compilation to GLSL):

See https://github.com/Crisspl/IrrlichtBAW/commit/a78f55c6c655335479ddf460ac61cd211e879bfe for initial code base.

Problem: Entry point + shader stage/type must be known while compiling GLSL->SPIR-V.

Possible approaches: Common for 1 and 2: Introduce two new classes: IGLSLProgram and IShaderProgram (base for ISPIR_VProgram and IGLSLProgram). Possibly rename all three of them by adding "Source" at end. Also an enum E_SHADER_SOURCE_LANG could be of use to avoid dynamic_casting.

  1. In this approach IGLSLProgram stores not only source but also shader stage and EP name. ICPUShader's constructor (the one from GLSL source) takes IGLSLProgram (instead of std::string source) and keeps it till first call to ICPUShader::getSPIR_VProgram. When the getter get called for the first time and ICPUShader has IGLSLProgram (and not SPIR-V), GLSL is compiled to SPIR-V as it happens now.
  2. ICPUShader doesn't have a member function getSPIR_VProgram at all or it gets replaced with getShaderProgram (returns either ISPIR_VProgram or IGLSLProgram) and keeps whatever it was constructed from (GLSL or SPIR-V). Then when ICPUSpecializedShader is created from ICPUShader, GLSL is compiled since now both shader stage/type and EP name are known.
  3. ICPUShader's constructor (the one from GLSL source) along with string source takes E_SHADER_STARGE and entry point name, then immidiately compiles to SPIR-V, but doesn't store shader type nor EP name anywhere.
devshgraphicsprogramming commented 5 years ago

Introspection (SPIRV-Cross calls it reflection) depends on entry point, so I suggest moving introspection interface to ICPUSpecializedShader since this is where entry point is defined.

The reflection operation will be very expensive, so I'd keep all the reflection data together with ISPIR_VProgram.

All ICPUShaders (GLSL, VK GLSL, raw VK SPIR-V or even HLSL) "compile down to" (more accurately, generate) one common object, VK SPIR-V.

I would like it to be that way also for one more, far important reason. In Vulkan your Pipeline has to match the shader 100%, i.e. if you're using some fixed input or descriptor set then it must be in the layout and if you're NOT using it then it cannot be in the layout.

Having introspection data in ICPUShader will potentially allow us to dynamically build pipelines with correct descriptor layouts via the reflection interface. I.e. I would like for the code that uses the engine to be able to query the entry points, etc. without having a-priori knowledge of their names and types.

Although helper methods in ICPUSpecializedShader that return/select a narrowed down set of reflection data would be useful.

There is no hard requirement for IrrBAW introspection data generated for an ISPIR_VProgram to be one API object, there can be a deeper hierarchy there if appropriate

As for now just a wrapper over SPIR-V source contained in ICPUBuffer. Derivative of core::IReference Counted. Maybe should also be an asset?

It is not only a wrapper over a ICPUBuffer, it should cache the generated reflection data about the SPIR-V it wraps.

We need to think about how to handle ICPUBuffer immutability (ICPUBuffer changes after reflection is generated) or whether to abandon the use of ICPUBuffer as backing data completely?

Yes it should like in the asset namespace but I don't think it should actually be serializable to a Blob.

Might be created from ISPIR_VProgram* or Vulkan GLSL source passed as std::string. In case of the latter, ISPIR_VProgram is created internally but its internal buffer really contains GLSL string. Then if ICPUShader::getSPIR_VProgram gets called and GLSL code, instead of SPIR-V, is present - GLSL is compiled to SPIR-V, old ISPIR_VProgram gets dropped and new one actually containing SPIR-V code is created.

I would keep logical hygiene and would advocate there is only a ICPUShader -- _produces_ --> ISPIR_VProgram relationship.

My initial idea was to have multiple derivations of ICPUShader, except for the SPIR-V base. For VK GLSL, OpenGL GLSL, HLSL, etc. and have the override of getSPIR_VProgram be actually responsible for the final transpilation to SPIR-V. So there would have been no internal buffer of ISPIR_VProgram until getSPIR_VProgram is called.

However I've also planned on having IGLSLCompiler : public asset::IShaderGenerator (and in the future similar IShaderGenerators) that returns ISPIR_VPrograms or ICPUShader.

So now, in the light of the fact that we will want to serialize ICPUShaders to .baw, we should really ask ourselves whether the IShaderGenerator should be fully in charge of compiling and ICPUShader be a final class that is always a wrapper for VK SPIR-V. Because otherwise I can imagine issues maintaining the serialization of GLSL, HLSL, etc.

At the stage when the interfaces are written down and finalized, we should ask ourselves whether it makes sense to merge ISPIR_VProgram and ICPUShader into a conceptually single object (see my reply to the previous quote).

devshgraphicsprogramming commented 5 years ago

GLSL->SPIR-V compilation overview:

This is entirely correct, but we need a good model of handling #includes so that they can come both from filesystem files (incl from .zip archives) as well as be substituted by various engine built-ins.

I imagine having a series of "virtual files" called irr/builtin/xyz.glsl for PBR, warp-scans, reductions etc.

devshgraphicsprogramming commented 5 years ago

User, while specializing should know spec-constant's ID

While this is generally desired, the engine should allow for explicit querying or implicit obtaining of active spec-constants (name+ID). This is why I proposed reflection/introspection via ICPUShader.

Crisspl commented 5 years ago

About ISPIR_VProgram:

It is not only a wrapper over a ICPUBuffer, it should cache the generated reflection data about the SPIR-V it wraps.

About ICPUShader:

Having introspection data in ICPUShader will potentially allow us to dynamically build pipelines with correct descriptor layouts via the reflection interface.

Ok, so where introspection data should be kept? Because I'm either missing something or we have two conflicting decisions here. Plus where all the queries (active spec constants, entry points, etc.) should be (ISPIR_VProgram or ICPUShader)?

Also the most important: introspection depends on entry point so you can't do it without knowing what is your entry point and so it cannot be done from ISPIR_VProgram or ICPUShader.

devshgraphicsprogramming commented 5 years ago

COpenGLSpecializedShader

Possibly there should be another ctor from CPU equivalent.

Possibly a good idea if suddenly we don't need to grab the driver, compiler, or any higher-level engine objects to achieve this construction.

translation of push_constants into regular uniform storage block/struct. Perhaps this involves just replacement of push_contants layout qualifier with binding = X. We have to know what's the X.

Actually I've gathered some intel that Nvidia does not pre-load the data at probably-most-used-offsets-in-bound-UBO into registers and there is always an implicit global VRAM load in next-gen DSA OpenGL.

I think we should re-qualify the push_constants block into a non-UBO sourced (no layout qualifier preserved except for row/column matrix layout) uniform struct.

i.e. (lightCount is a spec constant or a define)

layout(push_constant) uniform PushConsts {
    vec4 lightPos[lightCount];
} pushConsts;

would change to (with explicit location assignment)

layout(location = 0) struct PushConsts {
    vec4 lightPos[lightCount]; // consecutive array elements are assigned locations 0 to lightCount-1
};

uniform PushConsts pushConsts;

Unfortunately that means that you will have to first reflect/introspect the push_constants block and get the name of every member of every array index of every field to later set with glProgramUniform* (recursion) https://stackoverflow.com/questions/23591264/how-to-pass-uniform-array-of-struct-to-shader-via-c-code

You can either assign explicit locations or query them, however I would assign and compute explicitly. But I would then also query to make sure the offsets match (driver bugs) and that the uniform is actually active. https://www.khronos.org/opengl/wiki/Uniform_(GLSL)

However the active uniforms can be cached as glProgramUniformCallPtr, location, arraycount, push_block offset, active stage mask after compilation. https://www.khronos.org/opengl/wiki/GLAPI/glProgramUniform

The general wisdom of "compatibility profile outperforms core" suggests that, while removing glUniform* and glProgramUniform* from the front-end we should keep glProgramUniform* for the back-end.

We can delay the implementation of push_constants in OpenGL until #102

possibly bindings reordering

The global descriptor (set,binding) tuple needs to be converted into a per-resource-type binding.

The best idea I've come up with is to always report the OpenGL driver as "having support for 4 descriptor sets" and returning the max number of resource-type per-shader-per-descriptor-set as ActualResourceBindingMax/4.

Then binding = 4*set+X, where the X will most likely be the SPIR_V binding (or for better compaction, it could be the count of identical resource types in the descriptor set at original bindings less than current).

devshgraphicsprogramming commented 5 years ago

Ok, so where introspection data should be kept? Because I'm either missing something or we have two conflicting decisions here. Plus where all the queries (active spec constants, entry points, etc.) should be (ISPIR_VProgram or ICPUShader)?

My initial idea was to keep the back-end SPIR-V Cross objects and data inside private members of ISPIR_VProgram, but access via ICPUShader. I'm starting to increasingly think that ISPIR_VProgram and ICPUShader should be one class only.

Also the most important: introspection depends on entry point so you can't do it without knowing what is your entry point and so it cannot be done from ISPIR_VProgram or ICPUShader.

But we can query all entry-points and shader stages, and then do an introspection per-each returned (EP,SS) tuple... so it can be done from ICPUShader.

devshgraphicsprogramming commented 5 years ago

Problem: Entry point + shader stage/type must be known while compiling GLSL->SPIR-V.

There should be an IGLSLCompiler : public asset::IShaderGenerator that creates compiled SPIR-V out of GLSL+entry_point+shader_stage

Crisspl commented 5 years ago

Yeah, but my point was that you wanted to compile GLSL when ICPUShader is created from GLSL code. And ICPUShader doesn't know shader stage nor entry point.

devshgraphicsprogramming commented 5 years ago

No, VK GLSL is augmented (see notes 1,2), compiled to spir_v, and that is the ICPUShader.

I do not believe I said I wanted the ICPUShader to compile VK GLSL to SPIR-V by itself via a member method nor constructor.

The initial UML also includes the IGLSLCompiler that should be in charge of turning VK GLSL into SPIR-V.

Crisspl commented 5 years ago

May be I misunderstood something. What I remember is that there's ICPUShader::getSPIR_VProgram and if ICPUShader currently holds GLSL, than it should be compiled to SPIR-V on first call to aforementioned func. Is that right?

devshgraphicsprogramming commented 5 years ago

What I remember is that there ICPUShader::getSPIR_VProgram and if ICPUShader currently holds GLSL, than it should be compiled to SPIR-V. Is that right?

That's why we should talk about whether to make ICPUShader and ISPIR_VProgram one and the same. Essentially is there any point to having an ICPUShader that isn't basically a wrapper over SPIR-V code? Would there be some loss in generality for the case of HLSL, GLSL, etc.

Because it looks like we could get away with a Shader-Factory paradigm, in the form of an asset::IShaderGenerator

Crisspl commented 5 years ago

My initial idea was to keep the back-end SPIR-V Cross objects and data inside private members of ISPIR_VProgram, but access via ICPUShader.

For me this seems like they could be merged into one.

No, VK GLSL is augmented (see notes 1,2), compiled to spir_v, and that is the ICPUShader.

Ok, so GLSL is compiled to SPIR-V outside of ICPUShader (before creation) and then fed to its constructor? Ok, but entry point and shader stage still have to be known - this doesn't change anything

devshgraphicsprogramming commented 5 years ago

Ok, so GLSL is compiled to SPIR-V outside of ICPUShader (before creation) and then fed to its constructor? Ok, but entry point and shader stage still have to be known - this doesn't change anything

That's a limitation of the GLSL compiler, not SPIR-V itself. So in the case of cross-compilation the user will have to keep track of meta-data for shaders such as stage+entry point.

GLSL compiler will need to be told what the entry point is called and shader stage.

ICPUShader should not contain entry point and shader stage (except as SPIR-V opcodes).

Crisspl commented 5 years ago

Ok so then I think it can be done so that GLSL loader expects shader stage and entry point in a structure passed through const void* LoaderParams::userData which we already have if I remember correctly. Then loader internally compiles glsl to spirv with IGLSLCompiler (maybe we could also pass glsl compiler through userData if we want to have more than one in the future) and creates ICPUShader from it.

devshgraphicsprogramming commented 5 years ago

Ok so then I think it can be done so that GLSL loader expects shader stage and entry point in a structure passed through const void* LoaderParams::userData which we already have if I remember correctly. Then loader internally compiles glsl to spirv with IGLSLCompiler (maybe we could also pass glsl compiler through userData if we want to have more than one in the future) and creates ICPUShader from it.

I think GLSL compiler and GLSL loader should be two separate things (loader uses the compiler). Then we can make loader behave like glslangValidator, where it will deduce the stage from the file extension and always expects main to be the entry point.

Crisspl commented 5 years ago

I think GLSL compiler and GLSL loader should be two separate things (loader uses the compiler).

That's exactly what i'm saying.

Then we can make loader behave like glslangValidator, where it will deduce the stage from the file extension and always expects main to be the entry point.

Ok, nice. However I don't get why you mentioned glslangValidator

devshgraphicsprogramming commented 5 years ago

However I don't get why you mentioned glslangValidator

Because its the "official" compiler (in the form of a commandline tool) for GLSL into SPIR-V

devshgraphicsprogramming commented 5 years ago

Note: In the future, ICPUSpecializedShader should have a method to create skeleton ICPUDescriptorSetLayouts/ICPUPipelineLayouts from the reflection data.

Crisspl commented 5 years ago

What info should ICPUDescriptorLayout contain? also you want reflection/introspection to be in specialized (ICPUSpecializedShader) or non-specialized (ICPUShader) ?

devshgraphicsprogramming commented 5 years ago

That's for a new UML and issue, to do with graphics pipelines.

Recap

Moreover we should split this issue (#130) into two stages/issues:

I would like to skip the second stage for the time being until we get a nice SMaterial replacement in the form of a RenderPass independent pipeline .

Crisspl commented 5 years ago

ICPUShader should not back its SPIR-V storage using ICPUBuffer because of mutability concerns

ICPUShader::getSPIR_VProgram should return a const byte array and a size

isn't ICPUBuffer exactly array + size? As of "mutability concerns" - currently my code just returns const ICPUBuffer* (unpushed yet code)

Tackle #129 first to create ICPUShaders (without handling #includes)

include afaik can be done with shaderc, you just have to implement some interface shaderc delivers. However I'll have to look into its code/api more

devshgraphicsprogramming commented 5 years ago

isn't ICPUBuffer exactly array + size? As of "mutability concerns" - currently my code just returns const ICPUBuffer* (unpushed yet code)

I was just worried about someone getting ICPUBuffer from the ICPUShader and abusing it after the reflection data has been generated.

include afaik can be done with shaderc, you just have to implement some interface shaderc delivers. However I'll have to look into its code/api more

Great, but right now burning priority is getting this shader API semi-functional so we can move onto GraphicsPipeline and ComputePipeline objects.

Crisspl commented 5 years ago

I was just worried about someone getting ICPUBuffer from the ICPUShader and abusing it after the reflection data has been generated.

No, no, I wanted to return const buffer and if anyone wants to modify the spirv then he has to copy buffer contents

Great, but right now burning priority is getting this shader API semi-functional

oh, ok. Not later than tomorrow then, I think

devshgraphicsprogramming commented 5 years ago

No, no, I wanted to return const buffer and if anyone wants to modify the spirv then he has to copy buffer contents

Ok that could work.

oh, ok. Not later than tomorrow then, I think

Not that burning, I meant postpone handling of #include by a few weeks. Right now we should focus on merging

devshgraphicsprogramming commented 5 years ago

@Crisspl turns out my GPU supports VK_NV_raytracing, so keep the ICPUShader and ICPUSpecializedShader forward compatible with Mesh/Task Shader Pipeline and Raytracing Pipeline.

For now I believe it mostly just supporting more Shader Stage types (and recognising GLSL file extensions for shaderc compiler).

3rdparty libs such as SPIRV-Cross and shaderc might have to be updated to latest releases.

Crisspl commented 5 years ago

Take a look at shaderc's CompileOptions member functions and see what you'd like to support in IGLSLCompiler.

Crisspl commented 5 years ago

I have an idea for introspection data (SIntrospectionData) to be array of 4 descriptor sets plus specialization constants (ID + name string?). Then each descriptor set is a number of collections (possibly sorted by binding num, maybe a map) each for every resource type (storage imgs, samplers, UBOs, SSBOs, etc.). Every res type has its own struct since we might care about different things in each resource. What do you think?

devshgraphicsprogramming commented 5 years ago

Take a look at shaderc's CompileOptions member functions and see what you'd like to support in IGLSLCompiler.

Pretty much everything, except for optimization level... that should always be MAX. (we probably don't need anything below this line https://github.com/Crisspl/IrrlichtBAW/blob/master/source/Irrlicht/libshaderc/shaderc/shaderc.hpp#L255 )

First of all I'd make a glue-class that is both shaderc's IncluderInterface and IrrBaW's Asset Loader or AssetLoaderOverride. Basically we have a nice framework for resolving dependencies (file paths) so maybe we should use it.

Also I've realised that we need to retain the ability to serialize shaders that need custom compilation per-platform (extensions) and hence are unsuitable to be serialized in SPIR-V (one spir-v program per every permutation of compile-time options), shader source files could also be assets. Then the ICPUShader could come in two variants, as an already compiled SPIR-V program and one that needs to be created (compiled) from references to the shader source objects it needs for compilation.

I'd make 2 default custom shaderc includers (available to user to extend to their needs).

The basic one would simply take a runtime modifiable cache of <std::string,asset::IShaderSource>, it should resolve which entry to get based on requesting shader source and current work directory. It should also resolve any shader source requested that begins in irr/builtin/via dummy objects (with null allocator) created from static member const char* of the CGLSLFunctionGenerator. (I have a feeling that asset:IShaderSource should basically be an io::IReadFile)

For now we can only provide 2 builtin files: irr/builtin/anim/linearSkinning.glsl irr/builtin/math/complex.glsl // for complex number functions

The second default includer should build on top of the first, and it should augment it with the ability to attempt to load not-found includes via a io::IFileSystem from a set of predefined include paths (a-la C++ compiler).

devshgraphicsprogramming commented 5 years ago

I think GLSL compiler and GLSL loader should be two separate things (loader uses the compiler). Then we can make loader behave like glslangValidator, where it will deduce the stage from the file extension and always expects main to be the entry point.

I think we'll need to backtrack on that.

devshgraphicsprogramming commented 5 years ago

I have an idea for introspection data (SIntrospectionData) to be array of 4 descriptor sets plus specialization constants (ID + name string?). Then each descriptor set is a number of collections (possibly sorted by binding num, maybe a map) each for every resource type (storage imgs, samplers, UBOs, SSBOs, etc.). Every res type has its own struct since we might care about different things in each resource.

For the constants we'd really need only the ID + byte_size, but yeah name would be nice.

In Vulkan, there's a DescriptorSetLayout and a DescriptorSet, think of them as the type declaration and the particular variable instantiation. The descriptor set should be sorted by binding index number, use a union and a type enum to choose between the info structs. Vulkan doesn't group bindings per-resource-type and neither should we.

We should make the introspection data returned be useable to create an ICPUDescriptorSetLayout. Depending on how complete you can get the information from well formed GLSL SPIR-V, you can either:

You should also (if possible) return some data about the vertex/tessellation shader inputs and fragment shader outputs. I know that the data available won't be enough to even create a VkVertexInputAttributeDescription from introspection/reflection data, but vertex shader could at least provide us with the input ID, name and E_FORMAT class (obviously not the E_FORMAT itself, but at least we know if we need integer or float/normalized and how many components). Tessellation control/eval shader could provide us with VkPipelineTessellationStateCreateInfo::patchControlPoints if its there present in the layout qualifier and retrievable with reflection. While the fragment shader could let us know VkPipelineColorBlendStateCreateInfo::attachmentCount , and whether sample-shading is recommended.

Crisspl commented 5 years ago

Vulkan doesn't group bindings per-resource-type and neither should we.

Hm so unlike in OpenGL, in Vulkan binding points are shared between all resource types? Ok, container of unions then. I'll have to read the spec about descriptor sets at least.

Depending on how complete you can get the information from well formed GLSL SPIR-V, you can either: (...)

Initially I've been thinking about the 2nd option. I think it's more flexy and maybe more relevant since introspection data may include more than one descriptor set.

The includers stuff I'll take care of as one of the last things to do, so there's probably no need to discuss it more as for now?

devshgraphicsprogramming commented 5 years ago

Initially I've been thinking about the 2nd option. I think it's more flexy and maybe more relevant since introspection data may include more than one descriptor set.

Probably makes more sense unless you can actually create 1-4 full descriptorsetlayouts from the data available.

The includers stuff I'll take care of as one of the last things to do, so there's probably no need to discuss it more as for now?

Makes sense, but it is now apparent that the GLSL compiler should either rely on a tightly integrated source-code asset loader or it should be an asset loader itself.

Crisspl commented 5 years ago

I'll put this here https://github.com/Crisspl/IrrlichtBAW/commit/4de89aefdb670dc6e3bb1d90046a763e6d8512ef#r32293339

Crisspl commented 5 years ago

Looking on the UML: if we have both

createGPUObjectsFromAssets(ICPUSpecializedShader[1..*]) : IGPUSpecializedShader[1..*]
createGPUObjectsFromAssets(ICPUShader[1..*]) : IGPUShader[1..*]

how should step-by-step creation be looking from ICPUShader to IGPUSpecializedShader? I mean f.e. in Vulkan: we have ICPUShader, then we create IGPUShader from this (at this point we have API object) and now we could make IGPUSpecializedShader (which under Vulkan would be API object + spec data + EP-stage pair). But there's also this unwanted child ICPUSpecializedShader. Why is that actually? Under OpenGL this looks likewise: ICPUShader->IGPUShader (which can actually be same thing as ICPUShader) -> IGPUSpecializedShader (here we manipulate shader source code, downcompile spir-v to GLSL and finally get API object).

My proposal: have ICPUSpecializedShader only in purpose of storing it to file and loading from. It should be also possible to create IGPUSpecializedShader from ICPUSpecializedShader with createGPUObjectsFromAssets(ICPUSpecializedShader[1..*]) : IGPUSpecializedShader[1..*]. But driver should also have IDriver::specializeShader(IGPUShader*) which creates specialized shader which can actually be put into particular pipeline creation.

devshgraphicsprogramming commented 5 years ago

The situation is identical for IMeshDataFormatDesc and IMeshBuffer, some of these objects on the GPU side have actually no API object attached and they are 100% counterparts of the CPU object and provide no additional value.

When we create IGPUMeshBuffer from ICPUMeshDataFormatDesc the process is very similar.

devshgraphicsprogramming commented 5 years ago

@Crisspl it's imperative that you check that cross compiled SPIR-V for OpenGL redeclares gl_FragCoord

layout(origin_upper_left) in vec4 gl_FragCoord;

If this doesn't appear in the OpenGL down-compiled GLSL (check that by actually using gl_FragCoord in the code) then open an issue/bug-report with SPIR-V Cross project.

Basically for vulkan the default is origin_upper_left while for OpenGL it is origin_lower_left.

Obviously if the SPIR-V OpCode for gl_FragCoord convention declaration is already present in the SPIR-V you should honor whatever is there!

devshgraphicsprogramming commented 5 years ago

@Crisspl would you mind making an updated version of https://repository.genmymodel.com/devshgraphicsprogramming/VulkanShaders

To reflect what we've previously agreed and our shader-pipeline stub code that you've already written?

Crisspl commented 5 years ago

https://repository.genmymodel.com/crisserpl2/VulkanShaders

devshgraphicsprogramming commented 5 years ago

After discussion, the following things became apparent.

GLSL compilation can be split into stages

  1. #include pasting
  2. Preprocessing (with extension availability pasting)
  3. Compilation

This enables us to save ourselves a big headache

I think GLSL compiler and GLSL loader should be two separate things (loader uses the compiler). Then we can make loader behave like glslangValidator, where it will deduce the stage from the file extension and always expects main to be the entry point.

I think we'll need to backtrack on that.

Since GLSL compiler can provide 3 functions (expected to be called in order):

  1. gatherHeaders(IncludeHandler or IAssetLoaderOverload)
  2. std::string preprocess(const char* sourceBegin, const char* sourceEnd, const char* sourceToPrependBegin=nullptr, const char* sourceToPrependEnd=nullptr)
  3. ICPUShader* compile(const char* sourceBegin, const char* sourceEnd) And their overloads for r-value strings, and const ref strings

This enables the GLSL compiler to not be a loader, and possibly exist outside of the asset namespace (but obviously requiring it).

devshgraphicsprogramming commented 5 years ago

Serializing #included GLSL is enough

Since we want to avoid the well-known-shit-show of path-resolution inherent in texture loading, it makes sense to only allow serialization of GLSL with no #includes into .baw

This is really nice because the serialized GLSL will never need to be parsed to find out how many and what includes need to be fetched, while still allowing the IGLSLCompiler::preprocess to be called and enable Hardware-Vendor-Specific versions of the GLSL to be compiled via the extension #defines.

~Unless there's a specific need for an asset::IGLSLSource object, the thing that is being serialized as a GLSL_SOURCE blob-type, should just really be a std::string.~

Update: It seems an asset::IGLSLSource is needed to save the std::string source + entry point + shader stage

devshgraphicsprogramming commented 5 years ago

GLSL Source Loading is Orthogonal

The class/mechanism that provides us with the ability to do assetMgr->getAsset("./myMaterial.vert") can be separated from all the above, and provided at a later date.

This is because an IGLSLLoader can be what we would call a "non-native" loader (like OBJ, JPEG, etc.) that we don't expect to serialize 100% losslessly, or to provide any part of the Render-Graph without programmer's assistance (a bit how like you can't do anything with an asset that is just a texture).

Such a GLSL Loader could be implemented in two ways:

  1. Return an ICPUShader (compiled SPIR-V code with defines already attached for the current executing hardware)
  2. Return an IGLSLSource which is basically an std::string but with reference couting and smart pointer

Since asset is to be independent of video to enable offline processing, number 2 seems the only option.

This is really nice because the loader will only probably use IGLSLCompiler::gatherHeaders + IAssetLoaderOverload to resolve all its gathered headers.

Then we end-up with a nice chain of IGLSLSource -> ICPUShader -> ICPUSpecializedShader

devshgraphicsprogramming commented 5 years ago

Default instantiation of ICPUShader from IGLSLSource ?

Let's say that a .baw file serialized an IGLSLSource (or something similar), then:

But following remain unanswered:

In light of those questions it may make more sense to expect a IGLSLSource -> ICPUSpecializedShader direct path instead.

devshgraphicsprogramming commented 5 years ago

I've added some rules to keep in mind that will help us make the correct decisions: https://github.com/buildaworldnet/IrrlichtBAW/wiki/Asset-Pipeline-Philosophy

We know that without knowing the IDriver's enabled extensions, it makes no sense to compile an ICPUShader out of IGLSLSource (we cannot make valid VK GLSL for SPIR-V generation without doing irreversible preprocessing).

It would seem that without the video namespace, we cannot convert an IGLSLSource into an ICPUShader, so only two approaches remain:

  1. Make ICPUShader reference either an ICPUBuffer (spir-v) or IGLSLSource (glsl)
  2. Make ICPUSpecializedShader reference either an ICPUShader (spir-v) or IGLSLSource (glsl), kind of how ICPUMesh can reference different types of meshbuffers

For now I feel like option 1 would break a lot of our earlier conclusions regarding introspection/reflection etc. Before we had a nice assumption that ICPUShader would always be introspectable, now it could be introspectable only if it contains SPIR-V. This I don't like and it makes me favour option 2 instead.

Note: IGLSLSource can store GLSL UTF8 sourcecode in an ICPUBuffer instead of an std::string, makes more sense.

devshgraphicsprogramming commented 5 years ago

A tiny issue regarding preprocessing

Since we want to store/serialize GLSL source without any #includes but without resolved preprocessor macros such as #ifdef, etc. we come up against a wall.

The problem is that, if all #-directives except for #version and '#include` are disabled (for example commented out) we end up with out desired result, however this also disables include guards.

We need to prevent an endless loop or other error if a GLSL header includes itself directly or indirectly.

One approach would be to do it programmatically, which would require knowing of and keeping a stack of included files and refuse pasting the next include if it already appears n-times in the stack.

The other would be to abuse the preprocessor and insert our own lax-header guard that would allow for inclusion n-times like this:

#ifndef _GENERATED_INCLUDE_GUARD_XXX_1 
   #define _GENERATED_INCLUDE_GUARD_XXX_1 1
#elif !defined(_GENERATED_INCLUDE_GUARD_XXX_2)
   #define _GENERATED_INCLUDE_GUARD_XXX_2 2
...
#elif !defined(_GENERATED_INCLUDE_GUARD_XXX_N)
   #define _GENERATED_INCLUDE_GUARD_XXX_N N
#endif  

#ifndef _GENERATED_INCLUDE_GUARD_XXX_N
... source code ...
#else
/** IrrlichtBAW Preprocessor Message: Max Self-Include Count Reached !!! **/
#endif

#ifdef _GENERATED_INCLUDE_GUARD_XXX_N
   #undef _GENERATED_INCLUDE_GUARD_XXX_N
...
#elif defined(_GENERATED_INCLUDE_GUARD_XXX_1)
   #undef _GENERATED_INCLUDE_GUARD_XXX_1
#endif

or something more clever, but in similar spirit.

Crisspl commented 5 years ago

Hm, I think our include handler can have some container of contexts. Context is some kind of map [key=absolute_include_path, val=number_of_this_path_being_included]. Each context exists as long as preprocessing of its assigned source lasts (by "assigned source" i mean the shader source put into GLSL compiler). This way it can prevent from including more than once (i.e. working as include guards). This would most likely mean IGLSLCompiler creating such context for include handler, putting our include handler into shaderc (wrapped in this shaderc::IncludeInterface) and removing context after work done (and "work" means here preprocessing of source resolving #includes only - without #defines, #ifdefs etc.)