Templated renderer class

Question

A class that matches the rendered objects shader and render function(sets shader resources) based on its typeid hash.

#include <unordered_map>

#include <glm/glm.hpp>

#include "gl_func.h"
#include "gl_buffer.h"
#include "shader.h"
#include "log.h"

class Renderer
{
private:

    typedef void (*RenderFunctionPointer)();

    typedef std::unordered_map< size_t, Shader* >::iterator ShaderIterator;
    typedef std::unordered_map< size_t, RenderFunctionPointer >::iterator
        FunctionIterator;

    std::unordered_map< size_t, Shader* > Shaders;
    std::unordered_map< size_t, RenderFunctionPointer > RenderFunctions;

    glm::mat4 ProjectionMatrix;
    glm::mat4 ViewMatrix;

public:

    // Sets the given matrices for all shaders
    void SetProjectionMatrix( glm::mat4 p );
    void SetViewMatrix( glm::mat4 v );

    /*! \brief Tells the renderer how to render object of type T
     *  \param shd Shader to use for rendering.
     *  \param f Pointer to a function that takes a shader and a T& argument. */
    template< typename T >
    void AddObjectType( Shader* shd, void (*f)(Shader*, T& ) )
    {
        size_t type = typeid( T ).hash_code();
        Shaders[type] = shd;
        RenderFunctions[type] = (RenderFunctionPointer)f;
    }

    /*! \brief Renders the given object.
     *  The object type has to be registered before this call.
     *  \param obj Object to Render */
    template< typename T >
    void Render( T& obj )
    {
        size_t type = typeid( T ).hash_code();

        ShaderIterator i = Shaders.find( type );

        if( i == Shaders.end() )
        {
            LOG_WARNING() << "Couldn't find shader for object: " 
                << typeid( T ).name();
            return;
        }

        Shader* shd = i->second;
        shd->SetModelMatrix( obj.GetTransform() );
        shd->Bind();

        FunctionIterator f = RenderFunctions.find( type );
        if( f == RenderFunctions.end() )
        {
            LOG_WARNING() << "Couldn't find renderfunction for object: "
                << typeid( T ).name();
            return;
        }

        RenderFunctionPointer rp = f->second;
        reinterpret_cast< void(*)(Shader* shd, T& t) >(rp)( shd, obj );
    }
};

Here is some usage to clarify why is a reinterpret_cast necessary(this is not code to be reviewed, just rough example of usage):

// Init
DeferredRenderer->AddObjectType<Asset>(AssetShader.Get(),
   RenderFunctions::Deferred::RenderAsset );

// Somewhere passing a Asset to external C code( lua )
void* genericRenderable = MyAsset;
PassToLua( genericRenderable );

// Lua code calling entity system.
SetComponent( DeferredRenderable, genericRenderable );

// The render system processing entities
static_cast<Renderable*>(genericRenderable)->Render( DeferredRenderer );

// This calls 
void Asset::Render( Renderer* r ) // where r is DeferredRenderer
{
    r->Render( *this );
}

Answer 1

1. Another approach, the first one to come to my mind actually, would be using a std::type_index as key in your unordered_map. That's exactly what type_index was designed for, and it is more specific about its purpose than a raw size_t integer.

2. BTW, size_t is a member of namespace std in C++, so if you are going to use it, make sure to properly qualify the type as std::size_t. The C++ standard does not require it to also be available in the global namespace. You should also include <cstddef> somewhere (the header that defines std::size_t). At the moment you rely on some other standard header that imports this dependency.

3. I really don't like this:

   void (*f)(Shader*, T& )
   
   reinterpret_cast< void(*)(Shader* shd, T& t) >(rp)( shd, obj );
   

   First issue here is the duplicate typing of this function's signature. If you introduce a typo in any of the declarations, that could very well crash de program, because it would be using a different signature from the actual function. This can be improved with a local typedef. Second is the type cast from RenderFunctionPointer to the actual pointer type. It looks dangerous. Third, std::function would be a much better and type-safe option.

   According to feedback from a comment, this approach was taken to account for different functions for different types of objects. The template type T can be anything, however the map has to store a known type.

   Since you are already paying for the price of an indirect function call, then perhaps a cleaner approach would be to use polymorphism. Make the object type inherit from a Renderable interface that has a onRender(Shader & shd) virtual method. That would also remove the need to store function pointers into the renderer, cleaning up the interface as well. Instead of telling the renderer how to draw an object, let the object know how to draw itself.

4. You don't need these typedefs:

   typedef std::unordered_map< size_t, Shader* >::iterator ShaderIterator;
   typedef std::unordered_map< size_t, RenderFunctionPointer >::iterator FunctionIterator;
   

   If your sole objective is to reduce typing in places like this:

   ShaderIterator i = Shaders.find( type );
   

   That is what auto is for (C++11).

5. I see that you have a macro wrapping your log system (LOG_WARNING()). However, even if you mute your log by changing an internal verbosity parameter, the log calls would still be evaluated. I'm assuming this will be used for software like video games, where you probably don't need the overhead of logging on a released title. So I would suggest that you change you logging macros a little bit to avoid evaluating the log calls if you ever wish to mute the log system permanently (such as in a release build):

   #define LOG_WARNING(msg) do { MyLog::getInstance() << msg << "\n"; } while (0)
   

   That's one possibility. Then if you need to disable it, just make the macro a no-op:

   #define LOG_WARNING(msg)
   

   Now when using the log, a small change:

   LOG_WARNING("Couldn't find shader for object: " << typeid( T ).name());
   

   For the muted/disabled log, the logging calls would not be evaluated at all, incurring no runtime cost.

6. A final comment: The ownership of the Shader instances added to the renderer via AddObjectType() if quite unclear. Who is supposed to free those pointers? The renderer? The caller? No one? In such situations, the best solution is almost always to use a smart pointer. Perhaps a std::shared_ptr would be adequate in this case, if ownership is meant to be shared. Otherwise, a std::unique_ptr would be the best course of action.