SDL Destructor Called too early

Question

I'm programming a tile-based map in SDL, and the constructor of the Map class is being used to set the image used to represent each MapCell object contained within it. However, I'm having an issue with the destructor of my Sprite class that's being used to free the SDL_Surface* the object holds. The destructor is being called early, and I'm not entirely sure why. Here's a stripped-down version of my Map constructor just showing how the cell's sprites are allocated.

Map::Map(string fileName, int tileWidth, int tileHeight)
{   
    string mapData = ReadMap(fileName);
    _cells = new MapCell[_width*_height];

    for(int y = 0; y < _height; y++)
    {
        for(int x = 0; x < _width; x++)
        {
            int currentCell = y*_width+x;

            if(mapData[currentCell] == '0' || mapData[currentCell] == 'P' || mapData[currentCell] == 'X')
            {
                _cells[currentCell]._sprite = Sprite(Point2D(x*tileWidth, y*tileHeight), "Assets/Graphics/Grass.bmp");
                if(mapData[currentCell] == 'P')
                    _cells[currentCell]._sprite = Sprite(Point2D(x*tileWidth, y*tileHeight), "Assets/Graphics/Player.bmp");
                if (mapData[currentCell] == 'X')
                    _cells[currentCell]._sprite = Sprite(Point2D(x*tileWidth, y*tileHeight), "Assets/Graphics/Target.bmp");
            }
            else if(mapData[currentCell] == '1')
                _cells[currentCell]._sprite = Sprite(Point2D(x*tileWidth, y*tileHeight), "Assets/Graphics/Wall.bmp");
        }
    }
}

The destructor seems to be called immediately after the Sprite object is created. What am I missing here? I've also tried allocating the _sprite member of MapCell on the heap, but it causes the same issue. As far as I can tell, it's not going out of scope because the Sprite object created is part of the Map object.

The following are my constructors and destructor for the Sprite class:

Sprite::Sprite(void)
{
    _texture = NULL;
    _position = Point2D::Zero();
}

Sprite::Sprite(Point2D position, std::string texPath)
{
    _texture = Content::LoadBMP(texPath);
    _position = position;
}

Sprite::~Sprite(void)
{
    SDL_FreeSurface(_texture);
}

Here's my Main if it helps at all:

int main( int argc, char* args[] )
{
    const int TILEWIDTH = 32;
    const int TILEHEIGHT = 32;

    // Initialization
    InitSDL();
    Map map = Map("Assets/Maps/Map3.txt", TILEWIDTH, TILEHEIGHT);
    Window::SetSize(Rectangle(0, 0, map.GetWidth()*TILEWIDTH, map.GetHeight()*TILEHEIGHT));

    PathFinder pathFinder = PathFinder();
    List<Point2D> path = pathFinder.FindPath(map, map.GetPlayerStart(), map.GetTarget());
    List<Sprite> PathNodes = List<Sprite>();
    for(int i = 0; i < path.GetCount(); i++)
        PathNodes.Add(Sprite(*path(i)*32, "Assets/Graphics/PathNode.bmp"));

    bool quit = false;
    SDL_Event Event;

    while(quit == false)
    {
        while(SDL_PollEvent(&Event))
        {
            if(Event.type == SDL_QUIT)
                quit = true;
        }

        map.Draw();

        for(int i = 0; i < path.GetCount(); i++)
        {
            if(PathNodes(i)->GetPosition() != map.GetPlayerStart()*32 && PathNodes(i)->GetPosition() != map.GetTarget()*32)
                PathNodes(i)->Blit();
        }

        Window::Flip();
    }

    //Quit SDL
    SDL_Quit();

    return 0;    
}

score 3 · Accepted Answer · answered Feb 27 '13 at 11:22

The problem is the assignment x._sprite = Sprite(...). This creates a Sprite temporary, copies its fields into _sprite, then destroys the temporary. Furthermore, it does not call the destructor on _sprite before doing the assignment, so the old _texture will just leak.

If you want to avoid that, have a .set or .load function on Sprite to update the contents of the Sprite instead of copying, and create private assignment and copy-constructor methods to avoid accidental abuse.

Exactly : The "Rule of 3" and boost::noncopyable are the important bits here. — Roddy, Feb 27 '13 at 13:17

score 2 · Answer 2 · edited May 23 '17 at 12:03

2

for(int i = 0; i < path.GetCount(); i++)
        PathNodes.Add(Sprite(*path(i)*32, "Assets/Graphics/PathNode.bmp"));
                    // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                    // a temporary here!

When that expression finishes, the destructor of temporary Sprite gets called, frees the surface and leaves PathNodes objects with a dangling pointer to a freed surface. There's probably more such expressions in your code.

Obey The Rule of Three and write a proper copy constructor and assignment operator for Sprite class.

See the documentation ofSDL_Surface to see what needs to be done (you'll may have to increment the refcount member of the surface manually).

edited May 23 '17 at 12:03

Community

1
1

answered Feb 27 '13 at 11:24

jrok

54,456
9
109
141

In this case, to avoid mucking with refcounts, it may be simpler to simply privatize the copy-constructor and assignment methods. This way, you ensure that each texture is only associated with a single `Sprite` object. – nneonneo Feb 27 '13 at 11:30
1

If that's viable for OP, sure. Or possibly make `_texture ` `std::sharer_ptr` with `SDL_Freesurface` as deleter. – jrok Feb 27 '13 at 11:31

SDL Destructor Called too early

2 Answers2