I'm writing a little Tetris clone in C++ with SDL and I've run into a few flaws which I would like help with. Problem 1: Let me define a few things in my program. Objects: Tile: A single square in a Tetris game. A tile holds its coordinates and its SDL Surface. Block: the 4 piece Tetris block that falls down. A block contains 4 tile objects. Play_Area: This object contains an array of of Tile pointers. If a Tile pointer == NULL then the that space is considered empty. So here's the problem. When a block falls down and collides with the bottom of the play_area I have to transfer the 4 tiles contained in the block object to the play_area. This is how I've implemented that(its in a for loop to get all 4 tiles):

d_area[y][x] = new Tile();
memcpy(d_area[y][x], &block->getTiles(i), sizeof(Tile));

Then I delete the old block and create a new one. This creates a bit of a problem. The destructor of the block object calls the destructors of all 4 tiles. Each tile destructor tells SDL to free its SDL Surface. Now the tile copies I made in play_area have SDL_Surface pointers pointing to nothing. I could copy the tile and then create a new SDL Surface for it, blitting the original tile onto the copy's surface. I'm sure there's a better way to handle this problem. Problem 2: This is more of a coding issue. I'm having trouble handling different pixel formats. As a result I have code that looks like this in two places.

void Tile::rotateLeft()
{
	int bytes_per_pixel;
	Uint32* pixels = 0;
	Uint8* pixels_copy = 0;
	int x2, y2;

	if(d_tile == NULL)
		convertStat2Dyn();
	bytes_per_pixel = d_tile->format->BytesPerPixel;
	pixels = (Uint32 *)d_tile->pixels;
	pixels_copy = (Uint8 *)calloc(d_tile->w*d_tile->h, bytes_per_pixel);
	memcpy(pixels_copy, pixels, bytes_per_pixel*d_tile->w*d_tile->h);
	
	switch(bytes_per_pixel){
		case 1: // 8 bpp
			for(int x1 = 0; x1 < d_tile->w; x1++)
				for(int y1 = 0; y1 < d_tile->h; y1++){
					x2 = y1;
					y2 = d_tile->h-1-x1;
					putPixel8(d_tile, x2, y2, &pixels_copy[(y1*d_tile->pitch)+x1*3]);
				}
			break;
		case 2: // 16 bpp
			for(int x1 = 0; x1 < d_tile->w; x1++)
				for(int y1 = 0; y1 < d_tile->h; y1++){
					x2 = y1;
					y2 = d_tile->h-1-x1;
					putPixel16(d_tile, x2, y2, (Uint16 *)pixels_copy[(y1*d_tile->pitch/2)+x1]);
				}
			break;
		case 3: // 24 bpp
			for(int x1 = 0; x1 < d_tile->w; x1++)
				for(int y1 = 0; y1 < d_tile->h; y1++){
					x2 = y1;
					y2 = d_tile->h-1-x1;
					putPixel24(d_tile, x2, y2, &pixels_copy[(y1*d_tile->pitch)+x1*3]);
				}
			break;
		case 4: // 32 bpp
			for(int x1 = 0; x1 < d_tile->w; x1++)
				for(int y1 = 0; y1 < d_tile->h; y1++){
					x2 = y1;
					y2 = d_tile->h-1-x1;
					putPixel32(d_tile, x2, y2, (Uint32 *)pixels_copy[(y1*d_tile->pitch)+x1]);
				}
			break;
	}
	free(pixels_copy);
}

Its pretty painful to look at. I'd like to simplify this function considerably. Maybe throw it in a template of some sort. I'm not really sure how.

Regarding your first question...

Strictly speaking, you shouldn't be using memcpy() in C++. AFAIK, memcpy() just copies raw memory from one location to another, which doesn't play nice with non-POD types. You'll want to instead use plain old operator=() (and perhaps std::copy(), if you have a number of objects to copy, and both the source and destination can be accessed via iterators).

Without additional changes though, this won't fix the problem you mention (i.e. dangling pointers due to the surface being freed in the destructor).

Basically, what you need is some sort of reference counting for your SDL surfaces, so that they are only freed when the last object referencing them is destroyed. The easiest way to do this would almost certainly be to use boost::shared_ptr, along with a custom deleter that points to the SDL 'free surface' function.

Post back if you need any more details on this.

Yes, I was thinking about using shared pointers, but I didn't want to include boost just to get at the shared pointers. I'm sure I could use other parts of boost. I guess it'll be a learning experience.

I think you just need to rework your block class a little. Really the way I think you are doing it there should never be a reason to destroy a block, only a reason to destroy tiles. A block is just a relationship that holds the positions for four tiles to control things like moving, rotating, and collision detection. Once the block collides and can no longer move all you need to do is move your pointer to a new instance of a block containing four new tiles and the four tiles that were in the old block stay put, unless a row is deleted. So deleting a block shoud delete the pointers that point to the tiles but not actually delete the tiles themselves.

Quote:Yes, I was thinking about using shared pointers, but I didn't want to include boost just to get at the shared pointers. I'm sure I could use other parts of boost.

Once you start using any part of Boost (such as shared_ptr), you'll almost certainly find yourself using other Boost libraries as well.

That said, with Boost, you generally only pay for what you use, so if all you want is shared_ptr, just include shared_ptr.hpp and you'll be fine. (It is true that even some of the simpler Boost headers have pretty deep inclusion trees, but in most cases this shouldn't be a problem.)

Dwiff, that was what I wanted to do originally. For a normal tetris clone that would work fine, but in the future I'd like to deviate from the normal tetris playstyle and implement a physics engine(something like Box2D). I'm not sure, as I haven't played around with Box2D yet, but I think hanging on to the original block object might get messy. This is why I wanted to delete the old one and make a new one.

Quote:Original post by jyk

Quote:Yes, I was thinking about using shared pointers, but I didn't want to include boost just to get at the shared pointers. I'm sure I could use other parts of boost.

Once you start using any part of Boost (such as shared_ptr), you'll almost certainly find yourself using other Boost libraries as well.

That said, with Boost, you generally only pay for what you use, so if all you want is shared_ptr, just include shared_ptr.hpp and you'll be fine. (It is true that even some of the simpler Boost headers have pretty deep inclusion trees, but in most cases this shouldn't be a problem.)

That's good to know. I was a bit worried about how including boost would affect my compile time. That said I really shouldn't be worrying about compile time with a small project such as this.

I've almost finished implementing shared pointers in my program. I just have one last problem. I'm not very familiar with smart pointers and I'm trying to do this

d_tile = SDL_CreateRGBSurface(	SDL_SWSURFACE, d_img_loc.w, d_img_loc.h,				s_tileset->format->BitsPerPixel,                                s_tileset->format->Rmask, 				s_tileset->format->Gmask,                                s_tileset->format->Bmask,                                s_tileset->format->Amask );

Where d_tile is defined by:
typedef boost::shared_ptr<SDL_Surface> surface_ptr;
...
surface_ptr d_tile;

This is the error I get

error C2679: binary '=' : no operator found which takes a right-hand operand of type 'SDL_Surface *' (or there is no acceptable conversion)

Try:

d_tile.reset(SDL_CreateRGBSurface(...));

Also, don't forget that when you create the shared pointer object in the first place, you need to submit a custom deleter; otherwise, it'll try to invoke delete on the SDL surface pointer, which will probably end badly.

I can't remember the exact syntax, but I think it'll look something like this:

// In the initializer list......d_tile(0, &SDL_FreeSurface),...

Thanks again. You've been great help.

I've been at this for a little while, but I can't figure out how to make the shared pointer use the custom deleter. I've found all sorts of neat tricks that use the custom deleter, but I haven't found any examples that plainly explain how to implement a simple custom deleter.