Collider class using some kind of visitor pattern

Question

I'm trying to create a Collider class which will process collisions between different classes of the same base. One restriction is that I want these classes know nothing about each other, now they depend on Base and Collider classes only. Here is my code:

collidees.h

#pragma once

#include "collider.h"

#define ALLOW_COLLIDER_VISIT                                        \
    virtual void visit(Collider& c) override { c.collide(*this); }; \
    virtual void visit(Collider& c, Base& other) override { c.collide(*this, other); };

struct Base {
    virtual ~Base() = default;

    virtual void visit(Collider& c)              = 0;
    virtual void visit(Collider& c, Base& other) = 0;
};

struct A : Base {
    ALLOW_COLLIDER_VISIT;
};

struct B : Base {
    ALLOW_COLLIDER_VISIT;
};

struct C : Base {
    ALLOW_COLLIDER_VISIT;
};

collider.h

#pragma once

#include <memory>
#include <iostream>

#define ADD_REVERSE_COLLISION(A, B)                    \
    template<>                                         \
    inline void Collider::performCollision(B& b, A& a) \
    {                                                  \
        performCollision(a, b);                        \
    }

#define CALL_COLLIDER_FOR(CLASS) \
    void collide(CLASS& a) { collider.performCollision<T, CLASS>(static_cast<T&>(base), a); }

struct Base;
struct A;
struct B;
struct C;
struct Collider;

struct HelperBase {
    HelperBase(Base& base_, Collider& collider_) : base { base_ }, collider { collider_ } {}
    virtual ~HelperBase() = default;

    virtual void collide(A& base) = 0;
    virtual void collide(B& base) = 0;
    virtual void collide(C& base) = 0;

    Base&     base;
    Collider& collider;
};

template<typename T>
struct Helper;

struct Collider {
    template<typename T>
    void collide(T& t) {
        if (helper) {
            helper->collide(t);
        }
    }

    template<typename T>
    void collide(T& t, Base& other) {
        helper = std::make_unique<Helper<T>>(t, *this);
        callOtherToVisit(other);
    }

    void callOtherToVisit(Base& other);

    template<typename T1, typename T2>
    void performCollision(T1& first, T2& second) {
        std::cout << "No collision handler\n";
    }

    std::unique_ptr<HelperBase> helper = nullptr;
};

template<typename T>
struct Helper : HelperBase {
    Helper(T& base, Collider& c) : HelperBase { base, c } {}

    CALL_COLLIDER_FOR(A);
    CALL_COLLIDER_FOR(B);
    CALL_COLLIDER_FOR(C);
};

template<>
inline void Collider::performCollision(A& a, B& b) {
    std::cout << "Colliding a and b\n";
}
ADD_REVERSE_COLLISION(A, B);

template<>
inline void Collider::performCollision(B& b1, B& b2) {
    std::cout << "Colliding b1 and b2\n";
}

template<>
inline void Collider::performCollision(A& a, C& c) {
    std::cout << "Colliding a and c\n";
}
ADD_REVERSE_COLLISION(A, C);

template<>
inline void Collider::performCollision(B& b, C& c) {
    std::cout << "Colliding b and c\n";
}
ADD_REVERSE_COLLISION(B, C);

collider.cpp

#include "collider.h"
#include "collidees.h"

void Collider::callOtherToVisit(Base& other)
{
    other.visit(*this);
}

main.cpp - driver code

#include "collidees.h"
#include "collider.h"

int main()
{
    Collider collider;

    auto a = std::make_unique<A>();
    auto b = std::make_unique<B>();
    auto c = std::make_unique<C>();

    a->visit(collider, *b);
    a->visit(collider, *c);
    c->visit(collider, *b);
    c->visit(collider, *a);
    b->visit(collider, *a);

    return 0;
}

I suppose there are some disadvantages of this code:

1. It's quite complicated; adding a new collidee entails adding code in multiple places;
2. There is one static_cast instead of true dispatching (although, I suppose it is safe);
3. It's possible to call void visit(Collider& c) yourself, which is meaningless;
4. Class Helper stores reference to the object that can be easily deleted, which causes storing of dangling reference.

(Feel free to add other drawbacks in comments)

I would be glad to see any suggestions and improvements (or even your versions of this code).

UPDATE: Answers to comments:

• I assume collision detection as a separate step. At least in my case it is enough to add shape (or even just a rect) data to base class and find intersections of two objects, so derivatives still know nothing about each other.
• Virtual and override keywords in method declaration - it's just an inattention.
• Const correctness - again, I was focused on main problem, and missed it, although, yes, the performCollision method may modify arguments, so they cann't be const.

Anyway, thanks for replies. I think I will try to correct my code according to user673679's suggestions and then try to incorporate std::variant and std::visit as both commenters mentioned. I'll wait a little bit more for others and then aacept an answer.

Answer 1

const correctness:

There are a lot of consts missing.

• None of the member functions modify member data (on the shape classes, or the helpers), so they should all be marked const. e.g. virtual void visit(Collider& c) const = 0;

• None of the function arguments passed by reference (to the shape classes or helpers) are modified, so they should be passed by const& instead. e.g. virtual void collide(A const& base) = 0;

(Note: I guess if you intended to implement collision response (i.e. modify the objects) inside the performCollision calls, then you'd have to leave things non-const. Instead, I'd suggest returning enough information (e.g. as a std::optional<CollisionData>) to resolve the collision as a separate step later on. This is more flexible (e.g. it allows you to process collisions in groups for optimization or accuracy)).

---

unnecessary allocation:

We don't need to store the Helper in a unique_ptr. Rather than storing a reference to the Base class in HelperBase, we can store a reference to the template class in the template Helper class, e.g.:

template<class T>
struct Helper: HelperBase
{
    Helper(T const& t): t(t) { }

    T const& t;

    ...
};

Since Helper now preserves the type of the first argument we no longer need the static_cast.

Then we can create a Helper of the relevant type directly in the shape class visit functions, e.g:

struct A : Base
{
    void visit(Shape const& other) const override
    {
        auto helper = Helper<A>(*this);
        other.visit(helper);
    }

    void visit(HelperBase const& helper) const override
    {
        helper.collide(*this);
    }
};

This avoids the heap allocation, and means we don't actually need the Collider class. :)

We could pass the shape class type to the ALLOW_COLLIDER_VISIT macro, or add a make_helper<T>(*this) function so that we didn't have to.

---

unnecessary template specialization:

We don't need to use template specialization for the collision functions:

template<typename T1, typename T2>
void performCollision(T1& first, T2& second) ...

We can use simple overloading:

void performCollision(A const& , A const& ) { std::cout << "(A, A)\n"; }
void performCollision(B const& , B const& ) { std::cout << "(B, B)\n"; }
void performCollision(C const& , C const& ) { std::cout << "(C, C)\n"; }
void performCollision(A const& , B const& ) { std::cout << "(A, B)\n"; }
... etc.

---

example:

To illustrate the points above:

#include <iostream>

struct A;
struct B;
struct C;

void collide(A const& , A const& ) { std::cout << "(A, A)\n"; }
void collide(B const& , B const& ) { std::cout << "(B, B)\n"; }
void collide(C const& , C const& ) { std::cout << "(C, C)\n"; }
void collide(A const& , B const& ) { std::cout << "(A, B)\n"; }
void collide(B const& , A const& ) { std::cout << "(B, A)\n"; }
void collide(A const& , C const& ) { std::cout << "(A, C)\n"; }
void collide(C const& , A const& ) { std::cout << "(C, A)\n"; }
void collide(C const& , B const& ) { std::cout << "(C, B)\n"; }
void collide(B const& , C const& ) { std::cout << "(B, C)\n"; }

struct CollisionDispatcherBase
{
    virtual void collide(A const& other) const = 0;
    virtual void collide(B const& other) const = 0;
    virtual void collide(C const& other) const = 0;
};

template<class T>
struct CollisionDispatcher : CollisionDispatcherBase
{
    CollisionDispatcher(T const& t): t(t) { }

    T const& t;

    void collide(A const& other) const override { ::collide(t, other); };
    void collide(B const& other) const override { ::collide(t, other); };
    void collide(C const& other) const override { ::collide(t, other); };
};

struct Shape
{
    virtual ~Shape() {}

    virtual void visit(Shape const& other) const = 0;
    virtual void visit(CollisionDispatcherBase const& c) const = 0;
};

struct A : Shape
{
    void visit(Shape const& other) const override { auto c = CollisionDispatcher<A>{ *this }; other.visit(c); }
    void visit(CollisionDispatcherBase const& c) const override { c.collide(*this); }
};

struct B : Shape
{
    void visit(Shape const& other) const override { auto c = CollisionDispatcher<B>{ *this }; other.visit(c); }
    void visit(CollisionDispatcherBase const& c) const override { c.collide(*this); }
};

struct C : Shape
{
    void visit(Shape const& other) const override { auto c = CollisionDispatcher<C>{ *this }; other.visit(c); }
    void visit(CollisionDispatcherBase const& c) const override { c.collide(*this); }
};

#include <vector>
#include <memory>

int main()
{
    auto colliders = std::vector<std::unique_ptr<Shape>>();
    colliders.push_back(std::make_unique<A>());
    colliders.push_back(std::make_unique<B>());
    colliders.push_back(std::make_unique<C>());
    
    for (auto i = std::size_t{ 0 }; i != colliders.size() - 1; ++i)
        for (auto j = i + 1; j != colliders.size(); ++j)
            colliders[i]->visit(*colliders[j]);
}

---

use std::variant instead?

If the version of C++ you're using supports std::variant, we can ditch inheritance and use std::visit to do the double dispatch for us:

#include <iostream>
#include <variant>
#include <vector>

struct A { };
struct B { };
struct C { };

struct collide_visitor
{
    void operator()(A const& , A const& ) const { std::cout << "(A, A)\n"; }
    void operator()(B const& , B const& ) const { std::cout << "(B, B)\n"; }
    void operator()(C const& , C const& ) const { std::cout << "(C, C)\n"; }
    void operator()(A const& , B const& ) const { std::cout << "(A, B)\n"; }
    void operator()(B const& , A const& ) const { std::cout << "(B, A)\n"; }
    void operator()(A const& , C const& ) const { std::cout << "(A, C)\n"; }
    void operator()(C const& , A const& ) const { std::cout << "(C, A)\n"; }
    void operator()(C const& , B const& ) const { std::cout << "(C, B)\n"; }
    void operator()(B const& , C const& ) const { std::cout << "(B, C)\n"; }
};

int main()
{
    using collider = std::variant<A, B, C>;

    auto colliders = std::vector<collider>();
    colliders.push_back(A{});
    colliders.push_back(B{});
    colliders.push_back(C{});

    for (auto i = std::size_t{ 0 }; i != colliders.size() - 1; ++i)
        for (auto j = i + 1; j != colliders.size(); ++j)
            std::visit(collide_visitor{}, colliders[i], colliders[j]);
}

Answer 2

Just one coding tip:

virtual void visit(Collider& c) override

Use exactly one of virtual or override. That is, you don't repeat the virtual on the overrides.