C++ variable data being overwritten

Question

Been a few years since I've written C/C++, and now I'm facing a problem I just cannot seem to solve on my own.

Given the following struct:

struct InputData
{
    float diameter;
    float length;
    int vertIndex;
    struct InputData *parent;
    vector<InputData*> children;
    bool deadEnd;

    InputData(float dia, float lngth)
    {
        diameter = dia;
        length = lngth;
        vertIndex = NULL;
        parent = NULL;
        deadEnd = false;
    }
};

I start out by defining a number of nodes, and their parent/child relationship:

InputData i0 = InputData(3.0f, 3.0f);
InputData i1 = InputData(2.0f, 2.0f);
InputData i2 = InputData(1.0f, 1.0f);
InputData i3 = InputData(1.0f, 1.0f);
InputData i4 = InputData(1.0f, 1.0f);
InputData i5 = InputData(1.01f, 0.5f);

i0.children.push_back(&i1);
i1.children.push_back(&i2);
i2.children.push_back(&i3);
i3.children.push_back(&i4);
i4.children.push_back(&i5);

i1.parent = &i0;
i2.parent = &i1;
i3.parent = &i2;
i4.parent = &i3;
i5.parent = &i4;

Note that i5 as the only node doesn't have any children.

I then move on to do some work using this data (calling BuildMeshVertices(&i0, &vertices) from main()), and end up adding a child to i5:

void BuildMeshVertices(InputData* current, vector<SimpleVertex> *vertices)
{
    //Do work

    if(current->children.size() == 1)
    {
        BuildMeshVertices(current->children[0], vertices);
    }
    else if(current->children.size() == 0 && current->deadEnd == false)
    {
        InputData iDeadEnd = InputData(1.01f, 0.5f);
        iDeadEnd.deadEnd = true;
        iDeadEnd.parent = current;
        current->children.push_back(&iDeadEnd);     

        BuildMeshVertices(&iDeadEnd, vertices);
    }
}

After which everything is fine. i0 has one child (i1), i1 has a one child (i2), so on and so forth and i5 now has a child as well.

I call another function (BuildMeshIndices()), and suddenly a few lines into this function (line 63) the data for the newly added child to i5 is being overwritten. i5 still points to the right child, but the data for this child is suddenly garbled.

Here's a screenshot before and after (sorry about the link, but I weren't allowed to use IMG tags)

I cannot figure out why this happens, but I've got a feeling it's got something to do with my poor memory management?

UPDATE Also it doesn't have to be done this way. If for example changing the children vector to a vector of values is the preferred C++ way, I would much prefer that. I've tried to comment on the answers, but I'm not sure you guys are seeing the comments (According to the FAQ you need 50 reputation to leave comments)?

Below is the full source code (with everything unnecessary stripped out, but enough to reproduce the error):

#include "stdafx.h"
#include <vector>

using std::vector;

struct InputData
{
    float diameter;
    float length;
    int vertIndex;
    struct InputData *parent;
    vector<InputData*> children;
    bool deadEnd;

    InputData(float dia, float lngth)
    {
        diameter = dia;
        length = lngth;
        vertIndex = NULL;
        parent = NULL;
        deadEnd = false;
    }
};

//--------------------------------------------------------------------------------------
// Vertex types
//--------------------------------------------------------------------------------------
struct SimpleVertex
{
    float Pos;

    SimpleVertex(float Position)
    {
        Pos = Position;
    }
};

void BuildMeshVertices(InputData* current, vector<SimpleVertex> *vertices)
{
    current->vertIndex = vertices->size();

    //Add vertices..

    if(current->children.size() == 1)
    {
        BuildMeshVertices(current->children[0], vertices);
    }
    else if(current->children.size() == 0 && current->deadEnd == false)
    {
        InputData iDeadEnd = InputData(1.01f, 0.5f);
        iDeadEnd.deadEnd = true;
        iDeadEnd.parent = current;
        current->children.push_back(&iDeadEnd);     

        BuildMeshVertices(&iDeadEnd, vertices);
    }
}

void BuildMeshIndices(InputData* current, vector<unsigned long> *indices)
{
    indices->push_back(current->vertIndex+2);
    indices->push_back(current->vertIndex+0);
    indices->push_back(current->vertIndex+1);
    indices->push_back(current->vertIndex+3);
    indices->push_back(current->vertIndex+0);
    indices->push_back(current->vertIndex+2);

    InputData *parent = current->parent;

    unsigned long vOffset;

    if(parent != NULL && parent->children.size() == 1)
    {   
        vOffset = (unsigned long)current->vertIndex;

        indices->push_back(vOffset+7);
        indices->push_back(vOffset+5);
        indices->push_back(vOffset+4);
        indices->push_back(vOffset+6);
        indices->push_back(vOffset+5);
        indices->push_back(vOffset+7);

        indices->push_back(vOffset+10);
        indices->push_back(vOffset+8);
        indices->push_back(vOffset+9);
        indices->push_back(vOffset+11);
        indices->push_back(vOffset+8);
        indices->push_back(vOffset+10);

        indices->push_back(vOffset+15);
        indices->push_back(vOffset+13);
        indices->push_back(vOffset+12);
        indices->push_back(vOffset+14);
        indices->push_back(vOffset+13);
        indices->push_back(vOffset+15);

        indices->push_back(vOffset+18);
        indices->push_back(vOffset+16);
        indices->push_back(vOffset+17);
        indices->push_back(vOffset+19);
        indices->push_back(vOffset+16);
        indices->push_back(vOffset+18);
    }

    if(current->children.size() == 1 && current->deadEnd == false)
    {
        BuildMeshIndices(current->children[0], indices);
    }
}

int _tmain(int argc, _TCHAR* argv[])
{
    InputData i0 = InputData(3.0f, 3.0f);
    InputData i1 = InputData(2.0f, 2.0f);
    InputData i2 = InputData(1.0f, 1.0f);
    InputData i3 = InputData(1.0f, 1.0f);
    InputData i4 = InputData(1.0f, 1.0f);
    InputData i5 = InputData(1.01f, 0.5f);

    i0.children.push_back(&i1);
    i1.children.push_back(&i2);
    i2.children.push_back(&i3);
    i3.children.push_back(&i4);
    i4.children.push_back(&i5);

    i1.parent = &i0;
    i2.parent = &i1;
    i3.parent = &i2;
    i4.parent = &i3;
    i5.parent = &i4;

    // Create vertex buffer
    vector<SimpleVertex> vertices;

    BuildMeshVertices(&i0, &vertices);

    // Create index buffer
    vector<unsigned long> indices;

    BuildMeshIndices(&i0, &indices);

    return 0;
}

Answer 1

Change raw pointers to smart pointers and you will get ride of memory management problems.

You don't need to copy all boost to your project, just required headers.

#include <vector>
#include <boost/shared_ptr.hpp>
#include <boost/weak_ptr.hpp>

struct InputData
{
    float diameter;
    float length;
    unsigned long vertIndex;
    boost::weak_ptr<InputData> parent;
    std::vector< boost::shared_ptr<InputData> > children;
    bool deadEnd;

    InputData(float dia, float lngth, boost::weak_ptr<InputData> p = boost::weak_ptr<InputData>(), bool de = false)
        : diameter(dia), length(lngth), vertIndex(0), parent(p), deadEnd(de) {}
};

struct SimpleVertex
{
    float Pos;

    SimpleVertex(float position) : Pos(position) {}
};

void BuildMeshVertices(boost::shared_ptr<InputData> current, std::vector<SimpleVertex>& vertices)
{
    current->vertIndex = vertices.size();

    //Add vertices..
    if(current->children.size() == 1)
    {
        BuildMeshVertices(current->children[0], vertices);
    }
    else if(current->children.size() == 0 && current->deadEnd == false)
    {
          // this was a stack variable, so the pointer became invalid when going out of ambit.
        boost::shared_ptr<InputData> iDeadEnd( new InputData(1.01f, 0.5f, current, true) );
        current->children.push_back(iDeadEnd);         

        BuildMeshVertices(iDeadEnd, vertices);
    }
}

void BuildMeshIndices(boost::shared_ptr<InputData> current, std::vector<unsigned long>& indices)
{
    unsigned long vi = current->vertIndex;
    unsigned long  ioffset[] = { vi+2, vi, vi+1, vi+3, vi, vi+2};
    indices.insert(indices.end(), ioffset, ioffset+6);

    boost::shared_ptr<InputData> parent = current->parent.lock();
    if (parent && parent->children.size() == 1)
    {   
        unsigned long offs = current->vertIndex;
          unsigned long voffset[] = 
          { offs+7, offs+5, offs+4, offs+6, offs+5, offs+7,
            offs+10, offs+8, offs+9, offs+11, offs+8, offs+10,
            offs+15, offs+13, offs+12, offs+14, offs+13, offs+15,
            offs+18, offs+16, offs+17, offs+19, offs+16, offs+18 };
          indices.insert(indices.end(), voffset, voffset+24);
    }

    if(current->children.size() == 1 && current->deadEnd == false)
    {
        BuildMeshIndices(current->children[0], indices);
    }
}

int main()
{
    boost::shared_ptr<InputData> i0( new InputData(3.0f, 3.0f) );
    boost::shared_ptr<InputData> i1( new InputData(2.0f, 2.0f) );
    boost::shared_ptr<InputData> i2( new InputData(1.0f, 1.0f) );
    boost::shared_ptr<InputData> i3( new InputData(1.0f, 1.0f) );
    boost::shared_ptr<InputData> i4( new InputData(1.0f, 1.0f) );
    boost::shared_ptr<InputData> i5( new InputData(1.01f, 0.5f) );

    i0->children.push_back(i1);
    i1->children.push_back(i2);
    i2->children.push_back(i3);
    i3->children.push_back(i4);
    i4->children.push_back(i5);

    i1->parent = i0;
    i2->parent = i1;
    i3->parent = i2;
    i4->parent = i3;
    i5->parent = i4;

    // Create vertex buffer
    std::vector<SimpleVertex> vertices;
    BuildMeshVertices(i0, vertices);

    // Create index buffer
    std::vector<unsigned long> indices;
    BuildMeshIndices(i0, indices);

    return 0;
}

Think you still have a half C, half C++ dirty code... you should choose a language.

Answer 2

The moment your BuildMeshVertices function exits then iDeadEnd (i5's child) is deconstructed because you declared it on the stack and by exiting the function the entire stack frame is invalidated and all object deconstructed. You either want to dynamically allocate iDeadEnd or radically re-think how you define a tree. You'd be better off having each struct hold a vector of InputData (not InputData*) and then setting them up as follows:

InputData i0 = InputData(3.0f, 3.0f);
i0.children.push_back( InputData( 2.0f, 2.0f ) );
i0.children[0].children.push_back( InputData( 1.0f, 1.0f ) );

etc

Even that is far from ideal for obvious reasons. Defining a tree of elements is never the most fun of things to do though.

Answer 3

You are instancing iDeadEnd on the STACK, and taking a pointer to the stack address! When the functions terminate and the stack unwinds, the data for iDeadEnd is going to garble.

InputData *iDeadEnd = new InputData(1.01f, 0.5f);
iDeadEnd->deadEnd = true;
iDeadEnd->parent = current;
current->children.push_back(iDeadEnd);         

BuildMeshVertices(iDeadEnd, vertices);

The problem you now have is that the memory for iDeadEnd needs to be explicitly deallocated when you're done with it.

Answer 4

You should use dynamic memory to work with pointers. InputData will be destroyed when you go out of the function BuildMeshVertices, so the data will be garbaged or you will get a memory exception.

You should do something like

InputData * iDeadEnd = new InputData(1.01f, 0.5f);

instead of

InputData iDeadEnd = InputData(1.01f, 0.5f);

Answer 5

You are pushing the pointer to a stack object into your vector. Once execution leaves scope that stack object will get destroyed and the memory will be reused, resulting in a bogus value. Try

InputData *iDeadEnd = new InputData(1.01f, 0.5f);
iDeadEnd->deadEnd = true;
iDeadEnd->parent = current;
current->children.push_back(iDeadEnd);

Then you'll have to free that memory at the appropriate time.