Sign in to follow this  
spx2

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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by MikeTacular
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.


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 this post


Link to post
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 this post


Link to post
Share on other sites
Quote:
Original post by spx2
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).



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 this post


Link to post
Share on other sites
Quote:
Original post by Tha_HoodRat

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.


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 this post


Link to post
Share on other sites
Quote:
Original post by spx2
Quote:
Original post by Tha_HoodRat

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.


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 this post


Link to post
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 do
class 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 piece
for_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]

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this