2
\$\begingroup\$

I'm working on a multi render library. The point of it is to have a swapable backend at runtime to render the same item.

The renderer is given to the window (which is a wrapper over SDL2). Then the window is in charge of drawing a IDrawable, which is a virtual class implemented in a namespace for each backend. The implementation of IDrawable in each namespace got the same tag as the backend, for preventing rendering a drawable with the wrong backend.

Here's the code. I would appreciate some reviews on the architecture; what's good and what's wrong.

Thanks!

#include <string>
#include <iostream>

enum Tag
{
  VK,
  GL
};

class IDrawable
{
    public:
        virtual void draw() = 0;
        virtual Tag getTag() const = 0;        
};

namespace gl
{

class D : public IDrawable
{
    public:
        void draw()
        {
            std::cout << "Drawable gl::D\n";
        }

        Tag getTag() const { return tag; };
    private:
        Tag tag = Tag::GL;
};

}

namespace vk
{

class D : public IDrawable
{
    public:
        void draw()
        {
            std::cout << "Drawable vk::D\n";
        }

        Tag getTag() const { return tag; };
    private:
        Tag tag = Tag::VK;
};

}

class Backend
{
    public:
        virtual void render(IDrawable * d) = 0;
};

class Opengl : public Backend
{
    public:
        void render(IDrawable * d)
        {
            std::cout << "Backend gl\n";
            d->draw();
        }
        Tag getTag() { return tag; };
    private:
        Tag tag = Tag::GL;
};

class Vulkan : public Backend
{
    public:
        void render(IDrawable * d)
        {
            std::cout << "Backend vk\n";
            d->draw();
        }
        Tag getTag() { return tag; };
    private:
        Tag tag = Tag::VK;
};

template <typename T>
class Renderer
{
    static_assert(std::is_base_of<Backend, T>::value, "Backend not valid");
    public:
        void draw(IDrawable * d)
        {
            if(backend.getTag() == d->getTag())
            {
                backend.render(d); 
            }
            else
            {
                std::cout << "Cannot render\n";  
            }
        }

        T backend;    
};

template <typename T>
class Window
{
    public:
        Window(Renderer<T> r) : rend(r) {}

        void draw(IDrawable * d)
        {
            rend.draw(d);   
        }

    private:
        Renderer<T> rend;
};

int main()
{ 
    Renderer<Opengl> a;
    Renderer<Vulkan> b;

    Window w(a);    
    Window k(b);    

    gl::D d1;
    vk::D d2;

    w.draw(&d1);
    k.draw(&d2);
    w.draw(&d2); // Should fail


    return 0;
}

Live example

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

First of all: OpenGL and Vulkan work very differently in some parts. In the following i assume you are already familiar with vulkan.

IDrawable

There would probably some problems arise with the current design. You cannot just render something like that in vulkan, a vulkan drawable cannot implement the draw() function without that you maintain any global state (which you hopefully don't want, one of the strengths of vulkan is that you don't need that anymore and therefore gain things like threadsafety and a more reliable api performance). You would at least need a command buffer to record into, but this would change the meaning of the draw interface on different backends (immediate rendering on opengl vs recording on vulkan), which is bad. Additionally, in this case every drawable would change the bound pipeline (another thing that can be optimized using vulkan). In the end it would maybe make sense to handle drawing entirely in the renderer and maybe only expose things like material and vertices from the drawable. Every drawable could e.g. own a Buffer object which is an interface that might be a vulkan or an opengl buffer and managed by the renderer (those are the parts where one could really nicely abstract opengl and vulkan). In the end this also drives away the need for this rather smelly tag-based polymorphism pattern, and allows to model things in a easier, more natural way (which is usually a good sign). When you render using e.g. vulkan there should not be any opengl drawables around (should there? not a sign if there could be...) so there is no need to use this tag checking.

Templates

I am not sure your use of templates is optimal. In this case (although you just write the type Window in both cases) the both Windows are actually totally different types and therefore could not be changed at runtime. This means if you want to change the renderer you have to change the code somewhere which is probably not what you want (The same for the Renderer class). It would make more sense to have something like virtual (or even abstract) Renderer and Window interfaces that are then implemented by {Vulkan, GL}Window/Renderer. But doing this for renderer already drives away the need to do this for window at all, which is (at least for me) code smell anyways, the Window class should not changed dependent on the renderer used. In the case described you would need the Backend class anymore. You could of course use the Backend and the Render class but then, at least don't use templates (and I am not sure if there is enough to abstract from the both rendering apis that would justify a separate Renderer class).

Modern C++

You use C++17 features (and compile using this flag) so i assume you want to write modern C++. If so use the (great) features modern C++ gives you:

  • Use enum class Tag instead of enum Tag for type safety and less name pollution
  • Use the override keyword for functions that implement virtual ones
  • Use references where a nullptr is not a valid argument. Unless Renderer::draw(nullptr) has a special meaning and is therefore handled (which would proably be bad interface design) you should use a reference. Same for Window and Backend.
  • Window(Renderer) copies the renderer which is probably a bad idea. The renderer will be a heavy class which lots of state (which should probably not be copyable in the first place), use move semantics in a case like this. Additionally it would be nice to separate Window and Renderer, prefer to build a Renderer on top of a Window and not the other way around (how should the Renderer know to which Window it should render?). A draw method makes much sense in a Renderer but not so much in a Window (although there are many Window abstractions like this, this will lead to design issues later).

Also: No matter in which stage code is, don't name your classes something like D. And IMO Vulkan isn't a really good name either, choose something like VulkanBackend instead (same of OpenGL).


Those were just some first things that came to my mind (there are of course also things done right in your code i did not mention), there is probably more to say about it. An abstraction like this is really hard to build and (as a small - maybe opinionated - side note) it probably makes more sense to focus on one of the both api's if you do not absolutely need both since they are really different (at least in some, critical places like the rendering - plan global calls vs command buffer recording).

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your review. I'm not familiar with vulkan at all, so that's why I wanted to start this project. As I said, this is a premininary architecture, nothing is written yet (that explains the really bad naming). I'll start to work on a standalone project with vulkan this week end to have a taste of the beast and come up with a better design. \$\endgroup\$
    – bl4ckb0ne
    Commented Jul 6, 2017 at 19:28

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.