Node-based rendering

Question

Recently, I started wrapping Meshes and Transform classes into new ones, but by doing this I came upon a problem: I needed to render all primitives, or meshes with a renderer. After studying a some game engines (especially the JME3 engine), I came up with this node-based rendering. It's really not complete yet, but functional. I would like to ask you to point out any relevant changes I could make.

Entity.h

#pragma once

#include <glm/glm.hpp>
#include <vector>
#include "../render/Transform.h"

class Entity {
public:
    Entity  (
                const glm::vec3& position = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& rotation = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& scale = glm::vec3(1.0, 1.0, 1.0)
            )
        :
            m_transform(position, rotation, scale)
    {}
    Entity  (
                const Transform& transform
            )
        :
            m_transform(transform)
    {}

    virtual ~Entity(){}

    virtual void Render(const Shader& shader) = 0;
    virtual void Update(){}

    void SetTransform(const Transform& transform) { m_transform = transform; }
    Transform GetTransform() const { return m_transform; }

    void SetPosition(const glm::vec3& position) { m_transform.SetPos(position); }
    void SetRotation(const glm::vec3& rotation) { m_transform.SetRot(rotation); }
    void SetScale(const glm::vec3& scale) { m_transform.SetScale(scale); }
    glm::vec3 GetPosition() const { return m_transform.GetPos(); }
    glm::vec3 GetRotation() const { return m_transform.GetRot(); }
    glm::vec3 GetScale() const { return m_transform.GetScale(); }
protected:
    Transform m_transform;
};

Spatial.h:

#pragma once

#include "Entity.h"
#include "../render/Material.h"

class Spatial : public Entity
{
public:
    Spatial (
                Mesh mesh,
                const glm::vec3& position = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& rotation = glm::vec3(0.0, 0.0, 0.0),
                const glm::vec3& scale = glm::vec3(1.0, 1.0, 1.0)

            )
        :
            Entity(position, rotation, scale),
            m_mesh(mesh),
            m_material(CreateTexture("res/images/White.png"))
    {}
    virtual ~Spatial(){}

    virtual void Render(const Shader& shader){
        m_material.Bind(shader);
        m_mesh->Render();
    }
    virtual void Update(){}

    Mesh GetMesh() const { return m_mesh; }
    void SetMesh(const Mesh& mesh) { m_mesh = mesh; }
    Material GetMaterial() const { return m_material; }
    void SetMaterial(const Material& material) { m_material = material; }
protected:
    Mesh m_mesh;
    Material m_material;
};

Node.h:

#pragma once

#include <vector>
#include <iostream>
#include <cassert>
#include "Entity.h"
#include "Spatial.h"

class Node : public Entity{
public:
    Node(){}
    ~Node(){
        m_components.clear();
    }

    void AttachChild(Entity* component){
        assert((Entity*)this != component);
        m_components.push_back(component);
    }
    void DetachAllChildren(){
        m_components.clear();
    }
    void DetachChild(unsigned index){
        m_components.erase(m_components.begin() + index);
    }

    void Render(const Shader& shader){
        for(auto it = std::begin(m_components); it != std::end(m_components); ++it){
            (*it)->Render(shader);
        }
    }

protected:
    std::vector<Entity*> m_components;
};

main.cpp:

int main(int argc, char **args){

    Node rootNode;

    Node node;

    Spatial spatial = Spatial(CreateMesh("res/models/Suzy.obj"));
    Spatial spatial2 = Spatial(CreateMesh("res/models/Suzy.obj"));

    rootNode.AttachChild(&spatial2);
    node.AttachChild(&spatial1);
    rootNode.AttachChild(node);

    ///Some kind of renderer will render here; haven't implemented that yet...    
}

Answer 1

My first question is what does your graph represent? It sounds like you want it to represent the things which need to be rendered. Does the graph encode the order of the rendering? Or is it just a container that holds all the objects to be rendered? It's not clear from what you've shown here why you want things in a graph. (Which isn't to say it's a bad idea. It's just not clear what your reasons are.)

The code you've written is pretty straightforward and seems mostly reasonable. It's very readable, which is great.

What is the purpose of a Node? You've made it an Entity, but it's not clear to me that it should be. It looks like you want it to be an entity so that it can have a Render() method which calls its components Render() methods. From what you're showing here, it's not clear that a Node ever uses its transforms. (If I'm misunderstanding, then let me know.) If that's the case, I'd make the Node class be a free class that doesn't inherit from Entity. It would look mostly the same. To render, you would either give it a Render() method like it has now, or create a class or function that walks the graph, gets each node's list of children and calls each child's Render() method. I'd probably also rename it to EntityNode since it only ever keeps track of entities.

I notice that Entity's Render() method is pure virtual, but its Update() method is not, though it's empty. Will most subclasses of Entity implement it? If not, then it's fine to leave it as an empty method. But if each subclass will implement its own version, then it should probably also be pure virtual, too.

Speaking of virtual methods, if you're using C++11, you should add override after each virtual method in the subclasses, like this:

class Spatial : public Entity {
    //... constructors, etc.
    virtual void Render (const Shader& shader) override {
        //... method contents ...
    }
    // ... other methods...
};

This can help you catch some hard-to-understand errors as your class hierarchy grows. For example, if you ever change the function prototype for the Render() method, it will remind you to fix each subclass's implementation.

In your main method, you're reading the same object twice, though it's not clear why. That might make sense if you're creating 2 instances of the same object. But if not, it probably indicates a need to structure things differently. Since I don't know your goals there, I can't say which it is.

Answer 2

The previous review already made some pretty good points, so I'd just like to point out a few other things.

First a couple design considerations:

What you're trying to implement is collectively known as a Scene Graph, you probably already know that. This is a big subject in itself, and an important part of many 3D rendering applications. The three main tasks of a scene graph are:

• Fast queries of objects in the scene. E.g.: Scene object A might need to ask the Scene Graph for the world positions of objects B and C at every update.

• Space partitioning for culling. I.e.: Define which parts of the scene and which geometries are visible and send only those to the renderer, avoiding wasted processing drawing invisible stuff.

• Represent some hierarchy or ordering of objects. This is very useful for object transforms and animations / attaching stuff together so that the transforms are relative.

As you can see, the job of a scene graph is not trivial, but very important nonetheless. It is also not uncommon for a 3D rendering app to have separate graphs for different concerns. For instance, games usually have a separate graph just for world partitioning and culling, which is part of the "rendering system", while another graph just stores the hierarchies and relationships of game objects. This is a smart approach, since it better separates concerns. An interesting piece discussing a similar subject: GameArchitect.

---

Now the code, it looks good, but there's one tiny bit that is a real eye sore to me:

assert((Entity*)this != component);

I don't think you need that type cast at all, since Node is a child of Entity. Nevertheless, you should never, ever use a C-style cast to cast between related class types. A C-style pointer cast is just a bitwise reinterpretation of the data, so you can end up doing something like this (very contrived example, I know):

int * xyz = new int[...];

// Many lines and function calls later...

Entity * ent = (Entity *)xyz;

And the compiler wouldn't mutter a word about it. Always use dynamic_cast or static_cast to type cast pointers to objects.

• dynamic_cast is a "smart cast", it fails with a nullptr if there's no viable conversion between the types, so you should use this one when you're not sure if the types are compatible.

• static_cast is more slack, it only performs some compile-time type inference, but no runtime error checking. Performance-wise, it is faster, but you can only use it in instances when you are 100% sure the types are a match. So be extra careful.