3
\$\begingroup\$

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 );
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
  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.

\$\endgroup\$
6
  • \$\begingroup\$ #3 is neccessary. One renderer instance can handle multiple types, and I couldn't find a way to do it with static_cast. #6 The owner of the Shader is who allocated it. One shader may be in use across multiple renderers, this convention makes it simple, but thanks for the tip. #5 I intend to keep logging even in release code, because there will be some heavy modding and this cheap debugging is all I got time for. \$\endgroup\$
    – akaltar
    Mar 20, 2015 at 21:43
  • \$\begingroup\$ @akaltar Humm, I see. Than from what you describe, the render function should actually be a member of the object itself, if you are willing to add polymorphism to your objects (perhaps having a Renderable base class. Otherwise, I don't see another option besides the type cast indeed. \$\endgroup\$
    – glampert
    Mar 20, 2015 at 21:47
  • \$\begingroup\$ @akaltar If one shader might be used across renderers, then I think a shared_ptr would be a good option. You certainly don't want a shader instance to be deallocated while a renderer is using it. \$\endgroup\$
    – glampert
    Mar 20, 2015 at 21:48
  • \$\begingroup\$ I will edit my question to add how this #3 thing is exactly used. The Renderers are owned by the same class as Shaders, and are long-lived(loaded on application start until exit), but will convert to shared_ptr when I upgrade to VS2013 so I can (almost) use c++11 properly. \$\endgroup\$
    – akaltar
    Mar 20, 2015 at 21:58
  • \$\begingroup\$ @akaltar, I've made some changes to account for #3. Take a look and let me know if you'd like further clarification. \$\endgroup\$
    – glampert
    Mar 20, 2015 at 22:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.