Skip to main content
gamedev.net
🔒 Locked

Class array problem

Started by Pedro Candeias 5 replies 1.5k views
Original Post
Pedro Candeias
Pedro Candeias
Hey everyone,
A few days ago I started my first attempt to do a "proper" game, and i decided to do a platformer. I had previously done some simple ones such as tic tac toe and duckhunt. I used SDL for the graphics.
So i started by creating an object class, and then some class's who inherit from that one, so that can move, or so that they only use part of the surface image, etc.
One of those class's is a Tile class, and that's where im having trouble. The class itself works fine, but when i try to create an array of that class, it just simply doesn't work. and the sdl window goes into a strange state.
The i tried it in 3 different ways. First was with pointer like so:
Tile *lTiles;
lTiles = new Tile[size];
for(int i = 0; i < size; i++)
{
lTile = Tile(...);
}
Second method was same but without the pointers, just using a fix size array. Didn't work either. I get no error's and the warning's aren't related to it either. I also tryed it with new Tile(...).
I'm probably just doing something terribly wrong and stupid but i hope u guys can help me find it.
Also i know the tile class itself works coz when i try to run a single one without the array it works just fine.
I hope im not posting this in the wrong place, i know there's an sdl dedicated area, but as this is more of a problem with the class itself i decided not to post there.
I also added the code in case it's needed(keep in mind im still a newbie);
rip-off
rip-off
It appears your Object class does not obey the Rule of Three, so neither does your Tile class. Also you do not properly initialise the variables in your default Object constructor, which leads to undefined behaviour if the destructor is invoked.

You shouldn't need a complex type for your tile array. One option is to have an array of integers/enums, and use that as an index to a lookup table. Another point is to avoid manual point use, by managing the memory separately.

You might have one class/array for the type:

#include <memory>

struct TileType
{
std::shared_ptr<SDL_Surface> surface;

TileType(std::shared_ptr<SDL_Surface> surface) : surface(surface)
{
}
};

// The tile type collection (e.g. inside main())
std::vector<TileType> tileTypes;
// load/initialise tile types

// E.g.
SDL_Surface *surface = IMG_Load("grass.png");
if(surface)
{
std::shared_ptr<SDL_Surface> pointer(surface);
tileTypes.push_back(TileType());
}
else
{
// Error handling...
}


Once the types have been loaded or initialised


struct Tile
{
int type;
Tile() : type(0)
{
}
};

std::vector<Tile> tiles(width * height);
// Load / initialise tile map from file
// This is a totally random map
for(int i = 0 ; i < tiles.size() ; ++i)
{
tiles.type = rand() % tileTypes.size();
}

// How you might render
for(int y = 0 ; y < height ; ++y)
{
for(int x = 0 ; x < width ; ++x)
{
// Transform from 2D to 1D co-ordinates.
int tileIndex = (y * width) + x;
int typeIndex= tiles[index].type;
TileType &type = tileTypes[typeIndex];

SDL_Rect dest = { x, y };
SDL_BlitSurface(type.surface, 0, type.surface, &dest);
}
}

The above used a technique to "flatten" a two dimensional array into a single vector, because you seem to have a single dimensional array in your example code. This is the same equation that the compiler uses if you had a two-dimensional static array. However, some beginners might be more comfortable working with nested vectors, so they can use the more familiar <tt>array[y][x]</tt> lookup.

Also note how I avoided most manual memory management in the code above. The standard container std::vector<> and standard smart pointer std::shared_ptr<> are used to avoid worrying about manually cleaning up the memory.
blog | twitter
Pedro Candeias
Pedro Candeias
Ok so i've been also trying to do it with a vector class and i get the same proble. It works fine if i've only 1 item in the array but the moment i've more then that it start's bugging. The way i tryed to do it with the vectors was:
std::vector<Tile*> lTiles;
for(int i = 0; i < size; i++)
{
Tile tempTile(...);
lTiles.push_back(&tempTile);
}
Same thing, no errors, no warnings related to it, just a laggy and unfunctional window. Btw the main.cpp is a bit weird coz i've been just trying it out in there.
_the_phantom_
_the_phantom_
Erm, you are pushing a pointer to a stack allocated object which is being created and destroyed in a loop.
This, on its own, is bad bad broken voodoo.
Pedro Candeias
Pedro Candeias
Lol yes i jsut realized that. Thx man that fixed the problem i had with the vectors.
Btw the reason i did tiles that way is because it also contains the collision box and that other stuff.
rip-off
rip-off

It works fine if i've only 1 item in the array but the moment i've more then that it start's bugging.
[/quote]
Looking at your original source, the reason why this wasn't working for an array is indeed because of the rule of three. When you allocate an array, all elements are default constructed. You then loop over the elements, constructing a temporary tile and you then assign this value to the elements. This assignment uses the Tile class's assignment operator, which is default generated by the compiler. This generated operator simply assigns each member across. The assignment operator for a pointer is a raw bitwise copy, so the pointers are copied into the array.

What happens next is that the anonymous Tile instances are destroyed, which calls SDL_FreeSurface() on the surface. The call to SDL_FreeSurface invalidates all copies of the pointer, including the one(s) now in the array. Because you had a single tilesheet, this became invalid, and the next attempt at an assignment would fail (because the next call to the destructor would attempt to call SDL_FreeSurface() on an invalid surface).

In the code I gave you, I attempted to avoid these problems by using boost::shared_ptr<>, which manages the lifetime of the SDL_Surfaces for us. I actually used it incorrectly, it should be as follows:

SDL_Surface *surface = IMG_Load("grass.png");
if(surface)
{
std::shared_ptr<SDL_Surface> pointer(surface, &SDL_FreeSurface);
tileTypes.push_back(TileType());
}
else
{
// Error handling...
}

So my original code was also invoking undefined behaviour. I didn't compile or test it however. Anyway, the point is that std::shared_ptr is "smart" about deallocation, it only deallocates the passed object once all references to it have been removed. Copying it into many tiles would not be a problem.


Same thing, no errors, no warnings related to it, just a laggy and unfunctional window.
[/quote]
I get quite a few error messages and warnings compiling your code. I suggest you investigate your compiler settings and ramp up your warning level, and perhaps set the option to treat warnings as errors (you may want to take some time to fix any warnings before taking this step!).

However, your question suggests a fundamental assumption, that the compiler will warn you if you are doing wrong. This is not the case. In many ways, C++'s design works against the compiler from doing this. Indeed, none of these warnings point at the actual mistake that is causing you trouble.

The C++ standard also "allows" for a wide variety of undefined behaviour. That is what you are currently experiencing. The standard makes imposes no requirement upon compilers to check for such behaviour during compilation, nor does it specify any rational behaviour for programs that "successfully" compile from it.

Simply put, if your source code causes undefined behaviour, anything could happen, including it appearing to work, crashing immediately, erasing your hard drive and breaking only on the customer's machine, or when you are demoing your feature to your boss.


Btw the reason i did tiles that way is because it also contains the collision box and that other stuff.
[/quote]
That isn't necessary. The collision box for a given tile can be computed on demand from it's indices.
blog | twitter
Pedro Candeias
Pedro Candeias
Wow nice thx so much. Even though the vector thing was working that thing kept bugging me. I couldn't figure out what was wrong but it. Thx for taking the time to take a look into the code.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.