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:

It's quite complicated; adding a new collidee entails adding code in multiple places;
There is one static_cast instead of true dispatching (although, I suppose it is safe);
It's possible to call void visit(Collider& c) yourself, which is meaningless;
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.

How can you test for collision between objects of two different types, if they don't know about each other? If the Collider class has the actual work, and thus knows about all the different types involved, why do to collidable classes need a virtual visit function at all? — JDługosz
– JDługosz, Commented Jul 20, 2021 at 23:03
That is, why would it not work like std::visit? I understand that this doesn't have a two-argument form, but I mean work in the same manner. The Collider has functions with signatures for all the classes it can handle, so having a master list as part of Collider is not out of the question. — JDługosz
– JDługosz, Commented Jul 20, 2021 at 23:05

user673679 · Accepted Answer · 2021-07-21 09:38:31Z

`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]);
}

JDługosz · Accepted Answer · 2021-07-20 23:08:51Z

1

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.

answered Jul 20, 2021 at 23:08

JDługosz

11.7k19 silver badges40 bronze badges

Add a comment |

Stack Exchange Network

Collider class using some kind of visitor pattern

2 Answers 2

`const` correctness:

unnecessary allocation:

unnecessary template specialization:

example:

use std::variant instead?

You must log in to answer this question.

Hot Network Questions

Collider class using some kind of visitor pattern

2 Answers 2

const correctness:

unnecessary allocation:

unnecessary template specialization:

example:

use std::variant instead?

You must log in to answer this question.

Related

Hot Network Questions

`const` correctness: