# buggy tetris game

## Recommended Posts

Hi, I've recently wrote a tetris game. I have a bug in it concerning the proper re-drawing of the last line on the screen. (apart from the obvious design problems) I've tried multiple times to correct the bug but I cannot find the cause. I'd be grateful if any of you would like to have a look and tell me. It's written in C++ and SDL. The game and source can be found here http://perlhobby.googlecode.com/svn/trunk/tetris/ the second version.

##### Share on other sites
I just played the game and didn't notice any problems. Can you describe it in more detail?

##### Share on other sites
Hi MikeTacular,

Thank you very much for taking a look.
Yes of course I can describe:
The console has the matrix of the active cells on the grid.
When you make a full row that row collapses and let all the rows above it
fall into it(that's how tetris works).
Now,if you will do this(make a full row) , it will indeed collapse but some
of the cells of the row that should collapse will not be updated properly
(in particular,some that should be now empty are still filled with color).
You can try that out and also if you try it ,please tell me if that behaviour
is also present on your system.

Thank you MikeTacular

##### Share on other sites
Oh wow, ok, now I notice it. It seems to only clear the bottom row, and it only does it once (actually, sometimes it doesn't clear it). If you fill the bottom row and then fill it again, it doesn't clear it again. Do you think you could post your code? It's easy to see that something is wrong, but it's impossible to tell you why it's wrong without seeing any code.

##### Share on other sites
Oh duh, I totally saw the source folder when I downloaded the binaries. Okay, I'm going through it now...

Holy... You make the player not just fill the row, but fill the row with the exact same color? You do realize that this makes it impossible to play the game for very long? Also try to use less macros. A lot less macros. #define for4(x) for(int x=0;x<4;x++) is scary.

##### Share on other sites
Quote:
 Original post by MikeTacularOh duh, I totally saw the source folder when I downloaded the binaries. Okay, I'm going through it now...Holy... You make the player not just fill the row, but fill the row with the exact same color? You do realize that this makes it impossible to play the game for very long? Also try to use less macros. A lot less macros. #define for4(x) for(int x=0;x<4;x++) is scary.

QFT
bool Grid::RowIsFull(int r) {// see if all colors and active attributes of all cells on that row are the same	Uint32 SameColor = GridCells[0][r].Color;	bool SameActive	 = GridCells[0][r].active;	for(int x=1;x<WIDTH;x++)		SameColor	&=GridCells[x][r].Color,		SameActive	&=GridCells[x][r].active;	return ( (SameColor	== GridCells[0][r].Color)&&			 (SameActive== GridCells[0][r].active)&& 			 (SameActive!=false)		   );}
doesnt get excecuted at all so it never finds any full rows.

also

void Grid::ImplodeRow(int r) {//this implodes a row so that it disappears and all other rows fall under							  //the effect of "gravity"	for(int y=r;y>=FirstUnemptyRow;y--) {		for(int x=0;x<WIDTH;x++) {			GridCells[x][y] = GridCells[x][y-1];		};	};}

You set FirstUnemptyRow to 19 yet y is never going to be greater than FirstUnemptyRow.

##### Share on other sites
Sorry,it seems I've not explained the code properly.
Grid::RowIsFull(int) is called by Grid::CheckForFullRows() is called by Piece::Move(int) .
So it does get called.
About the second part.The numbering of the rows are increasing from the top
of the screen to the bottom of the screen.
FirstUnemptyRow will always be the first counting from top of the screen to the
bottom of the screen which has at least one active gridcell.
The row which will be full will always be Unempty so it will always be after
the FirstUnemptyRow and because of the numbering of the rows that we have
stated before FirstUnemptyRow will always be less or equal to the full row(r in this case).

##### Share on other sites
Quote:
 Original post by spx2Sorry,it seems I've not explained the code properly.Grid::RowIsFull(int) is called by Grid::CheckForFullRows() is called by Piece::Move(int) .So it does get called.About the second part.The numbering of the rows are increasing from the topof the screen to the bottom of the screen.FirstUnemptyRow will always be the first counting from top of the screen to thebottom of the screen which has at least one active gridcell.The row which will be full will always be Unempty so it will always be afterthe FirstUnemptyRow and because of the numbering of the rows that we havestated before FirstUnemptyRow will always be less or equal to the full row(r in this case).

It gets called but does not detect a full row if the colors are different.

bool Grid::RowIsFull(int r)

Does not detect a full row if the color of the blocks are different. When you complete a row with the same colored blocks it works fine.

##### Share on other sites
Quote:
 Original post by Tha_HoodRatDoes not detect a full row if the color of the blocks are different. When you complete a row with the same colored blocks it works fine.

Yes of course,and that's the intended behaviour
The problem appears after the detection has been made and after the
ImplodeRow(int) routine has been called.
There must be some part where some cleaning of the screen is not done..probably

##### Share on other sites
Quote:
Original post by spx2
Quote:
 Original post by Tha_HoodRatDoes not detect a full row if the color of the blocks are different. When you complete a row with the same colored blocks it works fine.

Yes of course,and that's the intended behaviour
The problem appears after the detection has been made and after the
ImplodeRow(int) routine has been called.
There must be some part where some cleaning of the screen is not done..probably

Well I have not been able to reproduce your bug. Can you post a screen shot. I have played it at least 15 times. As long as I make all the cells in a row the same color it works as intended.

##### Share on other sites
I think you'd be better off fixing the design and code quality - detection and/or correction of the bug will follow from that.

Mostly it's just that things needs simplifying, for example your Grid class is really complicated (and possibly the source of your problem). Think about things from the point of view of responsibility - what has a grid (a regular arrangement of cells or points) really got to do with the world, the active piece, the end-of-game state or rendering? A grid class should model the grid, nothing more and nothing less.

Similar things can be said for most of your other classes too - keep things simple and keep things self contained (you use friend a lot).

As for the code there's a fair bit of bad practice in there too, especially this:

#define for4(x) for(int x=0;x<4;x++)

You made this so you can write:
for4(i)    for4(j)        //stuff

What you've done is the wrong kind of refactoring by a long shot I'm afraid. Something like this is much better:

// one possible implementation, more generic methods exist template <TertiaryFunction>void for_each_in(const piece & p, TertiaryFunction f){    for (int i = 0; i < PIECE_WIDTH; ++i)        for (int j = 0; i < PIECE_HEIGHT; ++j)            f(i, j, p(i, j));}// example use:// 1) create a functor if you need to retain state (like the graphics instance)//    or else just a regular function will doclass draw_cell{    graphics & gfx;public:    draw_cell(graphics & gfx) : gfx(gfx) { }    void operator()(int x, int y, const cell & c)    {        if (c.visible())            gfx.draw_rect(x * CELL_SIZE, y * CELL_SIZE,                          CELL_SIZE, CELL_SIZE,                          c.colour());    }};// 2) apply to a piecefor_each_in(my_piece, draw_cell(screen));

and things like this:

enum {	MOVE_LEFT  = 64,	MOVE_RIGHT = 128,	MOVE_DOWN  = 256,	MOVE_UP	   = 512//rotation};void Piece::Move( int Direction ) {	assert( ( Direction & (MOVE_UP|MOVE_DOWN|MOVE_RIGHT|MOVE_LEFT) ) != 0); // Direction is only among the values listed in assert

are better off like this:

enum Directions { LEFT, RIGHT, DOWN, UP };void Piece::Move(Directions direction) {    // no assert, 'direction' can only be one of those four values}

So, yeah, basically if you go back and simplify all your code, refactor out any duplicated code, give things clear and concise responsibilities and use idiomatic C++ then you'll be much better off and I've no doubt your bug will also be fixed along the way too. [smile]

## Create an account

Register a new account

• ### Forum Statistics

• Total Topics
628388
• Total Posts
2982403

• 10
• 9
• 16
• 24
• 11