# Tetris Problems

This topic is 3484 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

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.

##### Share on other sites

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.

##### Share on other sites
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.

##### Share on other sites
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.

##### Share on other sites
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.)

##### Share on other sites
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.

##### Share on other sites
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)

##### Share on other sites
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),...

##### Share on other sites
Thanks again. You've been great help.

##### Share on other sites
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.

##### Share on other sites
Quote:
 Original post by DarkAntI'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.
Did you try what I posted earlier? That is:
d_tile(0, &SDL_FreeSurface)
If so, did it not work? And if not, what did you try? Can you post some code?

##### Share on other sites
Well I've tried a few things:
class Tile{  typedef boost::shared_ptr<SDL_Surface>  surface_ptr; public:  Tile(int y_coord, int x_coord,       int x_img_loc, int y_img_loc,       int width = 32, int height = 32);  Tile(){d_id = getNewID(); d_tile(0,&SDL_FreeSurface);}  ~Tile();  ... private:  ...  surface_ptr d_tile;};

That gave me this error message
error C2064: term does not evaluate to a function taking 2 arguments

After reading some documentation that I only partially understood I found that the template had this format
template<class Y, class D> shared_ptr(Y * p, D d);

"Requirements: p must be convertible to T *. D must be CopyConstructible. The copy constructor and destructor of D must not throw. The expression d(p) must be well-formed, must not invoke undefined behavior, and must not throw exceptions.-from Boost's documentation on shared pointers"

From what I gathered 'd' is a functor. So I added this
typedef struct  {    void operator()(SDL_Surface* surface) const	{		cout << "deleting surface" << endl;		if(surface)			SDL_FreeSurface(surface);	}  } surface_destructor;

and changed the initializer to
Tile(){d_id = getNewID(); d_tile = surface_ptr(0,surface_destructor());}

I got the same error message so I googled some more and ran across this site http://learningcppisfun.blogspot.com/2007/05/custom-deleters-with-smart-pointers.html

So instead of trying to understand what was going on(brilliant of me I know...) I just tried to copy his pattern and ended up with this
typedef boost::shared_ptr<SDL_Surface,surface_destructor>  surface_ptr(0,surface_destructor());...Tile(){d_id = getNewID(); d_tile.release();}

And got this error message(well, along with his 147 friends)
error C2977: 'boost::shared_ptr' : too many template arguments

You know I'm starting to think I don't have a clue as to what I'm doing :P

##### Share on other sites
Ok, let's back up to this:
class Tile{  typedef boost::shared_ptr<SDL_Surface>  surface_ptr; public:  Tile(int y_coord, int x_coord,       int x_img_loc, int y_img_loc,       int width = 32, int height = 32);  Tile(){d_id = getNewID(); d_tile(0,&SDL_FreeSurface);}  ~Tile();  ... private:  ...  surface_ptr d_tile;};
You'll note that in my earlier post I stated that the specified code went 'in the initializer list'. If you're not sure what an initializer list is, Google will tell you, but meanwhile here's an example:
class Tile{  typedef boost::shared_ptr<SDL_Surface>  surface_ptr; public:  Tile(int y_coord, int x_coord,       int x_img_loc, int y_img_loc,       int width = 32, int height = 32);  Tile() : d_tile(0,&SDL_FreeSurface)      {d_id = getNewID();}  ~Tile();  ... private:  ...  surface_ptr d_tile;};
Now, I've never used shared_ptr in exactly this way, so I may have something wrong. I would give the above a try though (I'm pretty sure it's right), and then post back if you have any more problems.

##### Share on other sites
Sorry, I did try putting it in Tile's initializor list initially, but it didn't work. I got this error:
error C2661: 'boost::shared_ptr<T>::shared_ptr' : no overloaded function takes 2 arguments

I don't understand why it would have to be in the class initializer. Couldn't I have a shared pointer point to an array of structs and have it use a custom deleter? I wouldn't have an initializor list to use if that were the case.

##### Share on other sites
Quote:
 Sorry, I did try putting it in Tile's initializor list initially, but it didn't work. I got this error: error C2661: 'boost::shared_ptr::shared_ptr' : no overloaded function takes 2 arguments
Can you post the code? (That is, the version of your code where you initialize your shared pointer object via the initializer list.)
Quote:
 I don't understand why it would have to be in the class initializer. Couldn't I have a shared pointer point to an array of structs and have it use a custom deleter? I wouldn't have an initializor list to use if that were the case.
You wouldn't create a shared pointer pointing to an array of SDL_Surface objects, but rather an array (or other container) of shared pointers, e.g.:
typedef boost::shared_ptr<SDL_Surface> surface_ptr_t;typedef std::vector<surface_ptr_t> surface_ptrs_t;surface_ptrs_t surfaces;
Anyway, don't give up! You're on the right track. (It would help though if you could post your code in its current form so we can see exactly what you're doing.)

##### Share on other sites
Quote:
Original post by jyk
Can you post the code? (That is, the version of your code where you initialize your shared pointer object via the initializer list.)
Quote:
 I don't understand why it would have to be in the class initializer. Couldn't I have a shared pointer point to an array of structs and have it use a custom deleter? I wouldn't have an initializor list to use if that were the case.
You wouldn't create a shared pointer pointing to an array of SDL_Surface objects, but rather an array (or other container) of shared pointers, e.g.:
typedef boost::shared_ptr<SDL_Surface> surface_ptr_t;typedef std::vector<surface_ptr_t> surface_ptrs_t;surface_ptrs_t surfaces;

What I was trying to say was that if I used a shared pointer outside of a class and I still wanted to use a custom deleter then how would I do that. I wouldn't have a class initializer to use.

Quote:
 Anyway, don't give up! You're on the right track. (It would help though if you could post your code in its current form so we can see exactly what you're doing.)

Haha, I've spent way too much time on this problem for me to give up. Thanks again for continuing to help me out.

As for the code
class Tile{  typedef boost::shared_ptr<SDL_Surface>  surface_ptr; public:  Tile(int y_coord, int x_coord,       int x_img_loc, int y_img_loc,       int width = 32, int height = 32);  Tile() : d_tile(0,&SDL_FreeSurface){d_id = getNewID();}  ... private:  ...  surface_ptr d_tile;};

##### Share on other sites
you could try cleaning it up a little this way...

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;switch(bytes_per_pixel){		case 1: // 8 bpp			putPixel8(d_tile, x2, y2, &pixels_copy[(y1*d_tile->pitch)+x1*3]);			break;		case 2: // 16 bpp					putPixel16(d_tile, x2, y2, (Uint16 *)pixels_copy[(y1*d_tile->pitch/2)+x1]);			break;		case 3: // 24 bpp					putPixel24(d_tile, x2, y2, &pixels_copy[(y1*d_tile->pitch)+x1*3]);			break;		case 4: // 32 bpp					putPixel32(d_tile, x2, y2, (Uint32 *)pixels_copy[(y1*d_tile->pitch)+x1]);			break;	}}

##### Share on other sites
Quote:
 Original post by CryoVenomyou could try cleaning it up a little this way...

Yeah, that looks much better, but I end up going through the switch statement d_tile->w*d_tile->h times instead of just once. I guess it really doesn't matter for the case of a Tetris game. I think a better way might be to just use function pointers.
int (Tile::*putPixel)(SDL_Surface* s, int x, int y, UintX* some_pixel)

The UnitX is where I'd have problems. I guess I could make all of the putPixel functions take a Uint8* and then grab the appropriate amount of bytes based on that offset. Or maybe I could put that function pointer into a template.

##### Share on other sites
Ok, I tried the 'd_tile(0,&SDL_FreeSurface)' syntax, and it didn't work. Sorry about that :-|

I've never used the 'custom deleter' feature of shared_ptr in exactly this way, so I had to take a look at the Boost docs. It looks like what you really want to do is leave the shared pointer out of the initializer list altogether (its default initializer will still be invoked), and then submit your deleter whenever you reset the shared pointer, like this:
surface.reset(SDL_CreateRGBSurface(...), &SDL_FreeSurface);
A pitfall of using shared pointers and custom deleters in this way is that if a shared pointer 'lives' past the point where SDL is shut down, invoking the deleter function (SDL_FreeSurface() in this case) may cause problems.

This really shouldn't be a problem unless you have global or static shared pointer objects (inadvisable), but just in case, you might wrap SDL_FreeSurface() in a function that first checks to make sure SDL is still active, and throws or asserts if it's not.

##### Share on other sites
That worked quite nicely. Btw, you were right about not using memcpy. It caused me a bit of trouble. Anyway, thanks again!