Monstrosity of addHighLevelShaderMaterial()

[greenya]

Hello!

Here i would like to talk about such IGPUProgrammingServices members as:

addHighLevelShaderMaterial(...),
addHighLevelShaderMaterialFromFiles(...)

As you may see its going crazy with number of arguments.
Now first one looks like:

Code: Select all

virtual s32 addHighLevelShaderMaterial(
                const c8* vertexShaderProgram,
                const c8* vertexShaderEntryPointName,
                E_VERTEX_SHADER_TYPE vsCompileTarget,
                const c8* pixelShaderProgram,
                const c8* pixelShaderEntryPointName,
                E_PIXEL_SHADER_TYPE psCompileTarget,
                const c8* geometryShaderProgram,
                const c8* geometryShaderEntryPointName = "main",
                E_GEOMETRY_SHADER_TYPE gsCompileTarget = EGST_GS_4_0,
                scene::E_PRIMITIVE_TYPE inType = scene::EPT_TRIANGLES,
                scene::E_PRIMITIVE_TYPE outType = scene::EPT_TRIANGLE_STRIP,
                u32 verticesOut = 0,
                IShaderConstantSetCallBack* callback = 0,
                E_MATERIAL_TYPE baseMaterial = video::EMT_SOLID,
                s32 userData = 0,
                E_GPU_SHADING_LANGUAGE shadingLang = EGSL_DEFAULT) = 0;

Yes, there are some convenient functions (which allows us to skip and don't specify entryPointName and compileTarges);
and yes, we have default values (all over the place), so our call can be very short indeed;

But i still think that this would be nice to change a bit; to change it in the way to make it easily extensible in future.
I would like to suggest to think about some kind optimization (not about execution speed, but overall look and feel... and future easy to add more arguments).

First) of all i suggest to make class that will encapsulate all ways how we can get the source of the shader program (it is a direct text string, a path to a file, a pointer to IReadFile; maybe others -- anyway, we can easy extend this single class for that).
Secondly), we need classes for each shader type (like vertex, pixel and geometry); we must eliminate code repetition, so we make kind of base class to store common data that has all shader types, and only then we inherit it to specify some single-shader-type related data (for example: arguments inType, outType and verticesOut related to geometry shader).
Thirdly), we need some top class which will hold the info about all three shader types and info related to the material (this class is not so critical, but just places all things to the same approach; so the aim to make some kind of addHighLevelShaderMaterial() which will take single argument which is highly and easily parameterized).

So next code shows how i think this should be done:
(please note, this code compiles but not works, because it is assumed to be put into IGPUProgrammingServices.h)
(watch outside on http://pastebin.com/DrVDnP76)

Code: Select all

namespace irr {
namespace video {
 
// Describes source of the shader (e.g. text string, name of the text file, pointer to IReadFile).
class GPUShaderSource
{
public:
 
        GPUShaderSource(const c8* sourceString)
                : source(0)
        {
                if (sourceString && sourceString[0])
                {
                        size_t s = strlen(sourceString);
                        source = new c8[s + 1];
                        strcpy(source, sourceString);
                }
        }
        
        GPUShaderSource(io::IReadFile* sourceFile)
                : source(0)
        {
                source = getFileContent(sourceFile);
        }
 
        GPUShaderSource(const io::path sourcePath)
                : source(0)
        {
                io::IReadFile* f = 0; // FIXME: cannot use here internal function call "io::createReadFile(sourcePath);"
                if (f)
                {
                        source = getFileContent(f);
                        f->drop();
                }
        }
 
        ~GPUShaderSource()
        {
                if (source)
                        delete [] source;
        }
 
        inline c8* getSource() const
        {
                return source;
        }
 
        inline bool isValid() const
        {
                return source != 0;
        }
 
private:
 
        c8* getFileContent(io::IReadFile* f)
        {
                if (f)
                {
                        const long s = f->getSize();
                        if (s > 0)
                        {
                                c8* c = new c8[s + 1];
                                f->read(c, s);
                                c[s] = 0;
 
                                return c;
                        }
                }
 
                return 0;
        }
 
        c8* source;
};
 
// Describes particular shader. This is base class to all shader types, and it contains all common data.
template <class T_CompileTarget, T_CompileTarget T_CompileTargetDefaultValue>
class GPUShaderInfo
{
public:
 
        GPUShaderInfo(const GPUShaderSource& source = GPUShaderSource((c8*)0), const c8* entryPoint = "main",
                T_CompileTarget compileTarget = T_CompileTargetDefaultValue)
                : sourceCode(0)
                , entryPoint(0)
                , compileTarget(compileTarget)
        {
                if (source.isValid())
                {
                        size_t s = strlen(source.getSource());
                        sourceCode = new c8[s + 1];
                        strcpy(sourceCode, source.getSource());
                }
 
                if (entryPoint && entryPoint[0])
                {
                        size_t s = strlen(entryPoint);
                        this->entryPoint = new c8[s + 1];
                        strcpy(this->entryPoint, entryPoint);
                }
        }
 
        ~GPUShaderInfo()
        {
                if (sourceCode)
                        delete [] sourceCode;
 
                if (entryPoint)
                        delete [] entryPoint;
        }
 
        inline bool isValid() const
        {
                return sourceCode && entryPoint;
        }
 
        inline c8* getSourceCode() const
        {
                return sourceCode;
        }
 
        inline c8* getEntryPoint() const
        {
                return entryPoint;
        }
 
        inline T_CompileTarget getCompileTarget() const
        {
                return compileTarget;
        }
 
private:
        
        c8* sourceCode;
        c8* entryPoint;
        T_CompileTarget compileTarget;
};
 
typedef GPUShaderInfo<E_VERTEX_SHADER_TYPE, EVST_VS_1_1> GPUVertexShaderInfo;
typedef GPUShaderInfo<E_PIXEL_SHADER_TYPE, EPST_PS_1_1> GPUPixelShaderInfo;
 
// Vertex shader and pixel shader have very similar structure, that is why we have used template, but if they would go different in future,
// we can do something similar to what we are doing bottom (with geometry shader) - and we will not break old code which uses it.
 
class GPUGeometryShaderInfo : public GPUShaderInfo<E_GEOMETRY_SHADER_TYPE, EGST_GS_4_0>
{
public:
 
        GPUGeometryShaderInfo(const GPUShaderSource& source = GPUShaderSource((c8*)0), const c8* entryPoint = "main",
                E_GEOMETRY_SHADER_TYPE compileTarget = EGST_GS_4_0, scene::E_PRIMITIVE_TYPE inType = scene::EPT_TRIANGLES,
                scene::E_PRIMITIVE_TYPE outType = scene::EPT_TRIANGLE_STRIP, u32 outVertexCount = 0)
                : GPUShaderInfo<E_GEOMETRY_SHADER_TYPE, EGST_GS_4_0>(source, entryPoint, compileTarget)
                , inputPrimitiveType(inType)
                , outputPrimitiveType(outType)
                , outputMaxVertexCount(outVertexCount)
        {
        }
 
        inline scene::E_PRIMITIVE_TYPE getInputPrimitiveType() const
        {
                return inputPrimitiveType;
        }
 
        inline scene::E_PRIMITIVE_TYPE getOutputPrimitiveType() const
        {
                return outputPrimitiveType;
        }
 
        inline u32 getOutputMaxVertexCount() const
        {
                return outputMaxVertexCount;
        }
 
private:
 
        scene::E_PRIMITIVE_TYPE inputPrimitiveType;
        scene::E_PRIMITIVE_TYPE outputPrimitiveType;
        u32 outputMaxVertexCount;
};
 
// Describes shader material. Contains all necessary data to call IGPUProgrammingServices::addHighLevelShaderMaterial().
class GPUShaderMaterialInfo
{
public:
 
        GPUShaderMaterialInfo(const GPUVertexShaderInfo& vertexShader, const GPUPixelShaderInfo& pixelShader = GPUPixelShaderInfo((c8*)0),
                const GPUGeometryShaderInfo& geometryShader = GPUGeometryShaderInfo((c8*)0), IShaderConstantSetCallBack* constantSetCallBack = 0,
                E_MATERIAL_TYPE baseMaterial = EMT_SOLID, s32 userData = 0, E_GPU_SHADING_LANGUAGE shadingLanguage = EGSL_DEFAULT)
                : vertexShader(vertexShader)
                , pixelShader(pixelShader)
                , geometryShader(geometryShader)
                , constantSetCallBack(constantSetCallBack)
                , baseMaterial(baseMaterial)
                , userData(userData)
                , shadingLanguage(shadingLanguage)
        {
        }
 
        inline bool isValid() const
        {
                // we assume that whole the material is valid, if any shader is valid
                return vertexShader.isValid() || pixelShader.isValid() || geometryShader.isValid();
        }
 
        inline const GPUVertexShaderInfo& getVertexShader() const
        {
                return vertexShader;
        }
 
        inline const GPUPixelShaderInfo& getPixelShader() const
        {
                return pixelShader;
        }
 
        inline const GPUGeometryShaderInfo& getGeometryShader() const
        {
                return geometryShader;
        }
 
        inline IShaderConstantSetCallBack* getConstantSetCallBack() const
        {
                return constantSetCallBack;
        }
 
        inline E_MATERIAL_TYPE getBaseMaterial() const
        {
                return baseMaterial;
        }
 
        inline u32 getUserData() const
        {
                return userData;
        }
 
        inline E_GPU_SHADING_LANGUAGE getShadingLanguage() const
        {
                return shadingLanguage;
        }
 
private:
 
        GPUVertexShaderInfo vertexShader;
        GPUPixelShaderInfo pixelShader;
        GPUGeometryShaderInfo geometryShader;
        IShaderConstantSetCallBack* constantSetCallBack;
        E_MATERIAL_TYPE baseMaterial;
        s32 userData;
        E_GPU_SHADING_LANGUAGE shadingLanguage;
};
 
// this is hypotetic implementation of addHighLevelShaderMaterial()
// please note: it crashes since thisGPU pointer is 0.
 
s32 addHighLevelShaderMaterial(const GPUShaderMaterialInfo& materialInfo)
{
        if (!materialInfo.isValid())
                return -1;
 
        const GPUVertexShaderInfo& vs = materialInfo.getVertexShader();
        const GPUPixelShaderInfo& ps = materialInfo.getPixelShader();
        const GPUGeometryShaderInfo& gs = materialInfo.getGeometryShader();
 
        IGPUProgrammingServices* thisGPU = 0; // FIXME: lets assume we are defined inside IGPUProgrammingServices
        // then addHighLevelShaderMaterial() from Irrlicht core would look like:
 
        // NEXT LINE LEADS TO CRASH
        return thisGPU->addHighLevelShaderMaterial(
                vs.isValid() ? vs.getSourceCode() : (const c8*)0,
                vs.isValid() ? vs.getEntryPoint() : (const c8*)0, // please note, we don't need to duplicate default value "main", we already defined it once in proper constructor (same for all other "(type)0" expressions)
                vs.isValid() ? vs.getCompileTarget() : (E_VERTEX_SHADER_TYPE)0,
                ps.isValid() ? ps.getSourceCode() : (const c8*)0,
                ps.isValid() ? ps.getEntryPoint() : (const c8*)0,
                ps.isValid() ? ps.getCompileTarget() : (E_PIXEL_SHADER_TYPE)0,
                gs.isValid() ? gs.getSourceCode() : (const c8*)0,
                gs.isValid() ? gs.getEntryPoint() : (const c8*)0,
                gs.isValid() ? gs.getCompileTarget() : (E_GEOMETRY_SHADER_TYPE)0,
                gs.isValid() ? gs.getInputPrimitiveType() : (scene::E_PRIMITIVE_TYPE)0,
                gs.isValid() ? gs.getOutputPrimitiveType() : (scene::E_PRIMITIVE_TYPE)0,
                gs.isValid() ? gs.getOutputMaxVertexCount() : (u32)0,
                materialInfo.getConstantSetCallBack(),
                materialInfo.getBaseMaterial(),
                materialInfo.getUserData(),
                materialInfo.getShadingLanguage()); // here "shadingLang" argument:
                        // i don't know actually the truth so it should be clarifyed:
                        // now i assume that each material defines language type for all shaders in it -- if it is not correct,
                        // that "shadingLanguage" should be simply moved from GPUShaderMaterialInfo to GPUShaderInfo class
}
 
} // end of video
} // end of irr

After this all, now we can use our "addHighLevelShaderMaterial()" next way:

Code: Select all

void main()
{
        // this way we add new shader material;
        // please note, we do not need overloads with "FromFiles" suffix, sources can be extended via extending GPUShaderSource (check its constructors to see how actually now it can be used)
 
        // just a test call, in real case here should be "device->getVideoDriver()->getGPUProgrammingServices()->addHighLevelShaderMaterial(...)"
        s32 m = video::addHighLevelShaderMaterial(video::GPUShaderMaterialInfo(
                video::GPUVertexShaderInfo("some shader program code goes here...", "main", video::EVST_VS_3_0),
                video::GPUPixelShaderInfo(io::path("shaders/frag-777.hlsl")),
                video::GPUGeometryShaderInfo(), // <-- this way we simply set "null" value (meaning don't use this particular shader)
                0,
                video::EMT_SOLID,
                0,
                video::EGSL_CG
                ));
 
        // ------------------------------------------------------------------

That is it!
What do you think about it?

[hendu]

I feel it's much more code for very little benefit in the addHigh call length. It also slows the function call down several times, by having to create three temporary objects.

[3DModelerMan]

I think the next step that Irrlicht needs to take is to redesign the material system. It would be cool to replace SMaterial with an IMaterial interface that we could inherit from and setup shaders that way. Or a shader class could just have conversion operators to E_MATERIAL_TYPE so that you can load a shader and call node->setMaterialType(myShader);

[greenya]

I feel it's much more code for very little benefit in the addHigh call length. It also slows the function call down several times, by having to create three temporary objects.

Yes, but often we do not need to addHighLevelShaderMaterial() every frame we render, in most cases we need to add new shader materials when game level is loading. So keeping it simple to manage and extend in future is good idea.

Besides, if you really need to call addHighLevelShaderMaterial() every frame (or very often), you can init GPUShaderMaterialInfo object before, and as many as you need. It will contain ready to be used source code of the shader (i mean, it will be loaded from file if needed); so all you will need is to call addHighLevelShaderMaterial() and pass single argument.

[Mel]

I think it is not that important really, due to the 80/20 rule, 80% of the time is running only the 20% of the code. And the shader compilation falls on the 80% rest of the code most of the time, i'm afraid of.

Not obstant, i think it wouldn't hurt to have a structure that contained all the data needed, like a SIrrlichtCreationParameter structure. The compilation routine then takes this structure, and the call is simplified quite a lot.

Code: Select all

addShader(ShaderCompilationStructure);

Besides, this could be useful for more than just shader compiling, using this structure, for instance, would allow us to keep trace of the shaders that have been compiled, and could help to simplify the task of deleting them. Now that there is going to be native support for CG shaders, a structure like this would help to unify even more the code.

[hybrid]

Yes, I also prefer the way that Mel suggest. BTW: I already started this discussion right before the geometry shader introduction. But since no one took this up, we introduced geometry shaders the old style. But we will definitely change this towards a more encapsulated way. This will also allow to change the shaders online more easily, re-use compiled shaders, etc.

[hendu]

One is certainly better than three.

I think it is not that important really, due to the 80/20 rule, 80% of the time is running only the 20% of the code. And the shader compilation falls on the 80% rest of the code most of the time, i'm afraid of.

I'm not implying it's a hot path; merely that the change would be bloat. That's obviously bad by itself, and also would likely start to spread to other parts.