1

I am trying to use the C++ "Clipper Library" (http://www.angusj.com/delphi/clipper.php), but when I try to return one of the objects from the clipper library from a function, it seems to become null or is altered somehow

Here is the function I wrote. The only relevant lines should be the last 3.

ClipperLib::PolyTree MeshHandler::trianglesToPolyTreeUnion(std::vector<Triangle> triangles)
{
    // Make all of the triangles CW
    for (auto& triangle : triangles)
    {
        triangle.makeClockwise();
    }
    // Set up the Clipper
    ClipperLib::Clipper clipper;
    // To take a union, add all the paths as "subject" paths
    for (auto& triangle : triangles)
    {
        ClipperLib::Path triContour(3);
        triContour[0] = convertGLMToClipperPoint(triangle.getVertex(0));
        triContour[1] = convertGLMToClipperPoint(triangle.getVertex(1));
        triContour[2] = convertGLMToClipperPoint(triangle.getVertex(2));
        clipper.AddPath(triContour, ClipperLib::PolyType::ptSubject, true);
    }
    // Now get the PolyTree representing the contours
    ClipperLib::PolyTree tree;
    clipper.Execute(ClipperLib::ClipType::ctUnion, tree);
    return tree;
}

When I call clipper.execute, it writes into the tree structure some contour information. It writes the correct information, and I've tested that it's correct. However, when I return the tree, it doesn't seem to copy anything, and the PolyTree that results from this function is empty.

I'm sure that there's nothing wrong with the library and that I'm just making a beginner c++ mistake here. Hopefully someone has an idea of what it might be.

Thanks!

edit: For reference, here is a documentation page for the polytree (http://www.angusj.com/delphi/clipper/documentation/Docs/Units/ClipperLib/Classes/PolyTree/_Body.htm)

edit: I thought the clipper library wasn't open source, but it is. Here is the code

typedef std::vector< IntPoint > Path;
typedef std::vector< Path > Paths;
class PolyNode;
typedef std::vector< PolyNode* > PolyNodes;

class PolyNode 
{ 
public:
    PolyNode();
    Path Contour;
    PolyNodes Childs;
    PolyNode* Parent;
    PolyNode* GetNext() const;
    bool IsHole() const;
    bool IsOpen() const;
    int ChildCount() const;
private:
    unsigned Index; //node index in Parent.Childs
    bool m_IsOpen;
    JoinType m_jointype;
    EndType m_endtype;
    PolyNode* GetNextSiblingUp() const;
    void AddChild(PolyNode& child);
    friend class Clipper; //to access Index
    friend class ClipperOffset; 
};

class PolyTree: public PolyNode
{ 
public:
    ~PolyTree(){Clear();};
    PolyNode* GetFirst() const;
    void Clear();
    int Total() const;
private:
    PolyNodes AllNodes;
    friend class Clipper; //to access AllNodes
};
Leif Gruenwoldt
  • 13,561
  • 5
  • 60
  • 64
user3281410
  • 502
  • 3
  • 14
  • 1
    Without the definition of ClipperLib::PolyTree we can only guess. However note that you're returning by value, so there will be some copy constructor (implicit or explicit) involved. – jsantander Apr 03 '14 at 15:09
  • 2
    @jsantander Not if there is copy elision performed, which looks very probable for this example. RVO is one of few exceptions in `c++`, when observable behavior of a program can change with optimization. It might be the case though. I wonder if OP tried fiddling with optimization flags. – luk32 Apr 03 '14 at 15:10
  • 1
    Unrelated, but you should also pass `triangles` by const reference: `const std::vector& triangles` – lethal-guitar Apr 03 '14 at 15:12
  • @ jsantander Should I worry that someone would write a class that cannot be copied? If, for some reason, the poly tree class had no copy constructor, would I need to create 1 pointer to it and then pass around the pointer instead of copying? – user3281410 Apr 03 '14 at 15:14
  • 1
    @user3281410 - Please post the polytree definition. Once you do that, then you will get answers to any questions you will ask about it. Right now, we don't know if it's safely copyable without a user-defined copy constructor and assignment operator being implemented. – PaulMcKenzie Apr 03 '14 at 15:16
  • That's possible, hard to say without seeing the source. You could try allocating `tree` on the heap and return a `std::unique_ptr` so you don't have to manually delete the memory. – lethal-guitar Apr 03 '14 at 15:16
  • crap how do you indent by 4 spaces all at once? – user3281410 Apr 03 '14 at 15:21
  • 1
    You can paste unindented code, select it, and click on the “code” button (looks like curly brackets `{ }`) and it will indent the entire selection by 4 spaces. Watch out for tabs. They muck up the indentation. – Emmet Apr 03 '14 at 15:22
  • 1
    @user3281410 - You have raw pointers to other objects in your class, and the class does not have a user-defined copy constructor or assignment op. Therefore you can't return it just like that, as copying isn't safe. As a matter of fact, it violates the "rule of 3", where if you have a destructor, you more than likely need a user-defined copy ctor and assignment operator. – PaulMcKenzie Apr 03 '14 at 15:26
  • @PaulMcKenzie I'm not sure I agree. I'd say it is safe to copy them as long as neither PolyNode or PolyTree own that memory (e.g they're not deleted in a destructor) – jsantander Apr 03 '14 at 15:29
  • @PaulMcKenzie Doesn't the tree class just have a vector of pointers in it? I don't understand why the compiler doesn't know how to copy a vector of pointers, although I'm sure you're right. – user3281410 Apr 03 '14 at 15:31
  • Well, now you're looking at implementation details. If that Clear() function does anything different in another version of this class, then copying will either become safe or not safe. If it isn't clear that copying is safe, IMO it isn't safe, even if the current implementation of the destructor suggests it is. Regardless, the class still violates the rule of 3. – PaulMcKenzie Apr 03 '14 at 15:32
  • @PaulMcKenzie I agree it is not good design. user3281410, have you tried stepping in with a debugger? – jsantander Apr 03 '14 at 15:35
  • 1
    @user3281410 - If you have a destructor that does "stuff" with anything to do with the pointers, then you need to write a user defined assignment operator and copy constructor. That is the bottom line. Otherwise your object cannot be copied safely as your function is doing. A vector of pointers just magnifies the problem even more. If it were a vector of *smart pointers*, more likely a vector of shared_ptr's, then you should be OK. – PaulMcKenzie Apr 03 '14 at 15:39
  • @jsantander Since I know it works, I'd rather just adapt my code than try to understand the internals of the library. Is the safest thing to do to just use unique pointers like lethal-guitar suggested earlier? If that's a good solution and someone puts it as an answer, I'd accept it. – user3281410 Apr 03 '14 at 15:40
  • @PaulMcKenzie I understand now. I didn't see that there was a Clear() function being called in the destructor. I thought you were saying that just having the vector of pointers was the thing that made it difficult to copy. So probably what's happening is that the destructor from the first copy of the object is clearing the data in the copy that lives outside of the function. Thanks. – user3281410 Apr 03 '14 at 15:42
  • I'm the author of this library and have just noted the comments above. This library has been my first and only foray into C++ so I do appreciate the constructive criticism above. I'll endeavor to fix this bug shortly. – Angus Johnson Oct 14 '15 at 15:43

5 Answers5

3

Before doing anything, make sure the following program works correctly:

int main()
{
   PolyTree p1;
   // fill PolyTree with some values that make sense (please add code to do this)
   //...
   PolyTree p2 = p1; 
   PolyTree p3;
   p3 = p1;
}

That is basically what we want to test. If you can get this code to work (add the relevant headers and initializations necessary), then you can focus back on the function. If the code above doesn't work, then there is your answer.

You need to get the code above to produce the correct copy semantics, and even just important, when main() exits, no memory corruption occurs on the destruction of p1, p2, and p3.

So either you can fix the class to copy safely, or forget about it and live with a class that you have to handle very carefully and in limited situations (i.e. you can't reliably return copies of it as you're doing now).

PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45
  • Well, but keep in mind that `PolyTree` is a library class, and the OP would like to avoid modifying it if possible. – lethal-guitar Apr 03 '14 at 15:47
  • 3
    ok. But it gives the OP a chance to understand exactly what is required to have a safely copyable object. – PaulMcKenzie Apr 03 '14 at 15:49
  • Sure, I wasn't criticizing your answer, just wanted to mention it :) – lethal-guitar Apr 03 '14 at 15:50
  • Thanks Paul. I don't understand all of the complexities of C++ yet, but I think the idea here is to test if copy assignment and move assignments work here? I'll try it, but do you expect the compiler to not allow one of these statements to compile? The idea being that the compiler can't automatically generate the copy assignment or move assignment for the object because of its unusual destructor? – user3281410 Apr 03 '14 at 15:54
  • Ok I tried it. It compiled, and it seems like p2 and p3 both got set up fine. However, when I returned from the function I was in, I got an access violation error in the Clear() command of one of the local variables p1,p2, or p3 (I'm not sure how to tell which).Looking at jsantander's answer, I guess the destructors are all trying to delete the same information multiple times. – user3281410 Apr 03 '14 at 16:04
  • @user3281410 - OK. I would have expected the issue with Clear(). So on destruction you got an error. The test ran perfectly (perfect in the sense that it proves that copying is not safe). – PaulMcKenzie Apr 03 '14 at 16:07
3

For the record and combining all the responses in the lengthy discussion to the question. Problems are:

  1. The value returned is a local variable that goes out of scope. This invokes the PolyTree destructor
  2. The PolyTree contains a vector of PolyNode * pointers. Those are allocated when clipper.Execute() is invoked.
  3. However PolyTree::Clear() does delete the nodes... and Clear() is invoked by the destructor.
  4. So within the function, the content is correct (allocated by Execute()), when passed outside, in the absence of copy constructors and operator=, the destructor of the local variable is invoked an the nodes are cleared, the result received outside of the function is empty.

The code for PolyTree::Clear()

void PolyTree::Clear() 
{
for (PolyNodes::size_type i = 0; i < AllNodes.size(); ++i)
      delete AllNodes[i];
    AllNodes.resize(0); 
    Childs.resize(0);
}

Probably you should follow the pattern of Execute and define your function as:

void MeshHandler::trianglesToPolyTreeUnion(std::vector<Triangle> triangles,ClipperLib::PolyTree &tree) 
jsantander
  • 4,972
  • 16
  • 27
  • Thank you. I wish I could accept multiple answers, but I kind of already promised to accept lethal guitar's in my comment. – user3281410 Apr 03 '14 at 15:57
2

Assuming you don't want to modify the (obviously badly designed) Clipper library, you can do it like I suggested in my comment:

// Make sure to have this at the top of your header file:
#include <memory>

std::unique_ptr<ClipperLib::PolyTree> MeshHandler::trianglesToPolyTreeUnion(std::vector<Triangle> triangles)
{
    // Rest of your code...

    std::unique_ptr<ClipperLib::PolyTree> tree(new ClipperLib::PolyTree);
    clipper.Execute(ClipperLib::ClipType::ctUnion, *tree);
    return tree;
}

Then, when calling your function:

std::unique_ptr<ClipperLib::PolyTree> tree(yourMeshHandler.trianglesToPolyTreeUnion(/*...*/);

// make use of tree...

Still, I would suggest opening a ticket (if there's a bug tracker) or contacting the library's author about this issue.

lethal-guitar
  • 4,438
  • 1
  • 20
  • 40
  • 2
    I would also agree with lethal-guitar to contact the author about it. If anything, the author should disable copying and assignment, so that at the very least, a compiler (or linker) error gets generated if a copy is attempted. – PaulMcKenzie Apr 03 '14 at 16:11
0

Is there already a solution for this problem? I am dealing with the same problem. Still no luck. The polytree outputs only memory adres.

when using : qDebug()<< "child id " << polynode->Childs;

When we have 2 childs, the output in terminal is : std::vector(0x55f30d2a91b0, 0x55f30d258480)

I hope someone knows how to solve this..

max
  • 21
  • 2
  • This does not provide an answer to the question. You can search for similar questions, or refer to the related and linked questions on the right-hand side of the page to find an answer. If you have a related but different question, ask a new question, and include a link to this one to help provide context. See: [Ask questions, get answers, no distractions](https://stackoverflow.com/tour) – DjSh Nov 20 '19 at 19:10
-1

Your problem is in the third line from the bottom of trianglesToPolyTreeUnion. The tree you are creating is created on the stack and is only in scope within the function.

You should dynamically allocate the memory and return a pointer to the tree or make your tree object a class member so it is still within scope once the function returns.

jhaight
  • 299
  • 2
  • 4
  • 2
    No. That isn't the problem. It is perfectly OK to return an object by value *if* the object has correct copy semantics. – PaulMcKenzie Apr 03 '14 at 15:42