7

I'm currently implementing few abstractions to represent level-set operations for 3D objects. Basically what is described in this amazing page for GLSL shaders.

To give a brief overview, a 3D object can be described by a function that maps the R^3 domain to a scalar called level-set (or signed-distance-function). For example, for a sphere, the level-set function is defined by phi(X) = X.Norm2() - R*R where Norm2 represents the squared Euclidean norm of a vector in R^3.

So I came out with a LevelSetObject class that represents such a concept:

 class LevelSetObject
    {

    using SDFFunction = std::function<double(double, double, double)>;

    protected:
        SDFFunction m_SDF;

    public:

        double SDF(double x, double y, double z) const {
            return m_SDF(x, y, z);
        }

Now I wanted to define few operators between LevelSetObjects. For example the union operator:

LevelSetObject LevelSetObject::operator+(const LevelSetObject& other) const {
        LevelSetObject outObj;
        outObj.m_SDF = [this, other]
            (double x, double y, double z) {
                return std::min(m_SDF(x, y, z), other.m_SDF(x, y, z));
            };
        return outObj;
    }

But I'm experiencing bad memory access when I create a temporary due to, for example, a triple sum (while if I sum the two objects separately as in the commented case, no memory leak is spotted using Valgrind and not SIGSEV). LevelSetSphere is a derived class of LevelSetObject where I define specifically the SDF of a sphere (given its center and its radius)

int main(int argc, char* argv[]) {

    // Generate the central sphere
    double radius = 1.0;
    SimpleVector center(2, 2, 2);
    LevelSetSphere sphere(radius, center);

    // Generate the ears spheres
    LevelSetSphere ear1(radius/2, SimpleVector(1, 1, 2));
    LevelSetSphere ear2(radius/2, SimpleVector(3, 1, 2));

    // Combine objects
    auto mickeyMouse = sphere + ear1 + ear2;
    //auto first = sphere + ear1;
    //auto mickeyMouse = first + ear2;

    // Materialize in the domain

    mickeyMouse.SDF(0.0, 0.0, 0.0);



}

What I suppose is that in the operator+ definition, the std::function keeps a reference to other that becomes a dangling reference when I actually call m_SDF because a temporary is created during the triple sum. I also tried to change the signature of operator+ to operator+(const LevelSetObject other), so passing by copy, but the outcome is the same.

Where am I failing? :)

rdbisme
  • 850
  • 1
  • 13
  • 39
  • Please [edit] your code to reduce it to a [mcve] of your problem. Your current code includes much that is peripheral to your problem - a minimal sample normally looks similar to a good unit test: only performing one task, with input values specified for reproducibility. – Toby Speight Jan 24 '19 at 13:25

2 Answers2

10

The lambda in the derived class captures this to the derived class, and shoves it into the base class's std::function.

This is a recipe for trouble.

What this means that, at the very least, the derived class must be fully Rule of 3 compliant, and implement, at the very least, a copy constructor and an assignment operator that meticulously reinstalls a new lambda, with a freshly-captured this that actually refers to the right instance of the derived class.

If you have a std::function member of a class, that captures its own this, and the class gets copied, the captured this does not get automatically updated to refer to the new instance of the class. C++ does not work this way. The new class's std::function's this still references the original instance of the class. And if an instance of a class is assigned from another instance of the class, guess what? The copied std::function's captured this still points to the copied-from instance of the class.

But I don't really see anything that the std::function does here that cannot be implemented by a garden-variety virtual function. Simply replace m_SDF by a virtual function, and this entire headache goes away.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
  • How would I implement `operator+` (that needs to redefine the `m_SDF` "on the fly") when I have `m_SDF` as virtual function instead of `std::function`? – rdbisme Jan 29 '19 at 00:38
  • 1
    Why exactly do you believe anything needs to be redefined "on the fly"? A composite LogLevelSet just needs to store pointers to the objects it was composed from, the virtual op calls them and returns the result. Just need to correctly handle object lifetimes. – Sam Varshavchik Jan 29 '19 at 01:45
  • Probably due to me being used to dynamically typed languages more than C++. I see the point by the way now. Thanks for your help! – rdbisme Jan 29 '19 at 09:14
  • 1
    Not sure that's any better to be honest. Any possible usage of std::function can be replaced by a virtual function, and in this case I'd say either is OK. The whole question was an object lifetime problem, and it'd be easy enough to make the same mistake using virtual functions. It'd definitely teach you more C++ though if that's the goal. – Joseph Ireland Jan 29 '19 at 09:55
  • @JosephIreland learning more C++ is indeed one of the aims. :) – rdbisme Feb 01 '19 at 13:48
7

Your bad memory access isn't due to the other variable, it's the this pointer of the temp object going out of scope.

You can fix it by capturing the SDF variables explicitly, like this:

LevelSetObject LevelSetObject::operator+(const LevelSetObject& other) const {
        LevelSetObject outObj;
        auto& SDF=this->m_SDF;
        auto& other_SDF=other.m_SDF
        outObj.m_SDF = [SDF, other_SDF]
            (double x, double y, double z) {
                return std::min(SDF(x, y, z), other_SDF(x, y, z));
            };
        return outObj;
    }
Joseph Ireland
  • 2,465
  • 13
  • 21
  • I choose this as the correct answer since it gives the least effort right solution even if the [other answer](https://stackoverflow.com/a/54347674/1334711) goes more into details also providing an alternative solution with virtual functions. – rdbisme Feb 05 '19 at 23:22