🔒 Locked 🎲 GameMaker

Breaking down nested switch statements

Started by adam_o Jan 21, 2008 at 1:14 PM 13 replies 2.2k views

Original Post

Jan 21, 2008 at 1:14 PM

Hello all, In my programming, I have a nested switch statement that I want to simplify or get rid of. Now I didn't know whether to post this in this forum or in the "Alternative Game Libraries" forum (because the code is SDL), so I apologize if I messed up. The problem is that this code takes much too long to go through, for understandable reasons. What I'm trying to do is figure out how to streamline it or increase the performance while still accomplishing the same tasks. Here's the code:

void event_loop()
{
	SDL_Event event;
	while(!done && SDL_WaitEvent(&event))
	{
		switch (event.type) 
		{
			case SDL_MOUSEBUTTONDOWN:
			{
				switch (event.button.button)
				{
					case SDL_BUTTON_LEFT:
					{
						int x_loc = event.button.x;
						int y_loc = event.button.y;
						fileout << "(" << x_loc << "," << y_loc << ")" << endl << flush;
						break;
					}
					default:
					{
						break;
					}
				}
				break;
			}
			case SDL_USEREVENT:
			{
				switch (event.user.code) 
				{
					case RUN_GAME_LOOP:
					{
						if (!stop)
						{
							tile_down();
							translate();
						}
						break;
					}
					default:
					{
						break;
					}
				}
				break;
			}
			case SDL_QUIT:
			{
				done = true;
				break;
			}
			case SDL_KEYDOWN:
			{
				switch(event.key.keysym.sym)
				{
					case SDLK_ESCAPE:
					{
						done = true;
						break;
					}
					case SDLK_DOWN:
					{
						if (!stop)
						{
							tile_down();
							translate();
						}
						break;
					}
					case SDLK_UP:
					{
						if (!stop)
						{
							tile_rotate();
							translate();
						}
						break;
					}
					case SDLK_LEFT:
					{
						if (!stop)
						{
							tile_left();
							translate();
						}
						break;
					}
					case SDLK_RIGHT:
					{
						if (!stop)
						{
							tile_right();
							translate();
						}
						break;
					}
					case SDLK_RETURN:
					{
						if (!stop)
						{
							while (collision_check())
							{
								tile_down();
							}
							translate();
						}
						break;
					}
					case SDLK_SPACE:
					{
						stop = !stop;
						if(stop)
						{
							SDL_bool success;
							success = SDL_RemoveTimer(timer);
							if(!success)
							{
								exit(201);
							}
						}
						if(!stop)
						{
							timer = SDL_AddTimer(timer_speed, game_timer, 0);
							if(!timer)
							{
								exit(202);
							}
						}
						break;
					}
					default: break;
				}
			}
            default: break;
        }
    }
}

Antheus

Jan 21, 2008 at 1:45 PM

The goggles, they do nothing :)

void event_loop(){  SDL_Event event;  while(!done && SDL_WaitEvent(&event)) process_event(event);}void process_event( const SDL_Event & event ){  switch (event.type) {    case SDL_MOUSEBUTTONDOWN : on_mouse_down(event); break;    case SDL_USEREVENT       : on_user_event(event); break;    case SDL_KEYDOWN         : on_key_down(event); break;    case SDL_QUIT            : done = true; break;    // and so on....  }};  void on_mouse_down( const SDL_Event & event ){  if (event.button.button)  {    case SDL_BUTTON_LEFT:	{	int x_loc = event.button.x;	int y_loc = event.button.y;	fileout << "(" << x_loc << "," << y_loc << ")" << endl << flush;	break;    }    default: ;  }};void on_user_event( const SDL_Event & event ){  switch (event.user.code)   {    case RUN_GAME_LOOP:      if (!stop)      {          tile_down();	  translate();      }      break;    default: ;  }};// and so on...

Falken42

Jan 21, 2008 at 1:47 PM

Performance-wise, that code is not slow. It may not be nice to look at, but huge switch statements do not imply slow code. How do you think Windows messages are handled? Any decent compilier (with optimizations on) will automatically break down a switch into a jump table, and just call into the necessary code.

I would suggest calling a function for each type of event.type, so you can keep the code under control. For example, something like this:

void doMouseEvent(SDL_Event *event){    switch (event->button.button)    {        case SDL_BUTTON_LEFT:        {            int x_loc = event->button.x;            int y_loc = event->button.y;            fileout << "(" << x_loc << "," << y_loc << ")" << endl << flush;            break;        }        default:        {            break;        }    }}void doUserEvent(SDL_Event *event){    // add code here}void doKeyDownEvent(SDL_Event *event){    // add code here too}void event_loop(){    SDL_Event event;    while(!done && SDL_WaitEvent(&event))    {        switch (event.type)        {            case SDL_MOUSEBUTTONDOWN:                doMouseEvent(&event);                break;            case SDL_USEREVENT:                doUserEvent(&event);                break;            case SDL_QUIT:                done = true;                break;            case SDL_KEYDOWN:                doKeyDownEvent(&event);                break;            default: break;        }    }}

Hope this helps.

adam_o

Jan 21, 2008 at 2:06 PM

Quote:
Original post by bpoint
Performance-wise, that code is not slow. It may not be nice to look at, but huge switch statements do not imply slow code. How do you think Windows messages are handled? Any decent compilier (with optimizations on) will automatically break down a switch into a jump table, and just call into the necessary code.

I would suggest calling a function for each type of event.type, so you can keep the code under control. For example, something like this:

*** Source Snippet Removed ***

Hope this helps.

It's slow on my computer (200ms +/- 50), and my computer's not slow (1 GHz). Also, this is on a Mac, might that change anything?

Another thing, all you did was break up the code into different functions. Does this speed it up, or does it just make it look nice?

Antheus

Jan 21, 2008 at 2:30 PM

Quote:
Original post by adam_o
It's slow on my computer (200ms +/- 50), and my computer's not slow (1 GHz). Also, this is on a Mac, might that change anything?

That's not why it's slow. The switch statement alone should be able to check ~100-1000 million!! events per second.

If anything is slow, then it's either your handlers, or you're measuring the time incorrectly.

Whatever your 200ms is, it's not switch statement.

Quote:
Another thing, all you did was break up the code into different functions.

Yes, for readability.

Quote:
Does this speed it up, or does it just make it look nice

It can't be done faster than what you already have.

adam_o

Jan 21, 2008 at 3:12 PM

Quote:
Original post by Antheus
Whatever your 200ms is, it's not switch statement.
...
It can't be done faster than what you already have.

Ok, I've figured out that the only slow part (everything else is almost instantaneous) is when I hit return, which signifies the "hard drop." Here's the code:

						if (!stop)						{							while (collision_check())							{								tile_down();							}							translate();						}						break;

I have a feeling that the while loop is taking a while (lol pun) but it should only loop a maximum of 14 or 15 times, and tile_down() is not a big function... although I noticed a redundancy:

void tile_down(){	next_piece = curr_piece;	next_piece.y--;	if (!collision_check())	{		new_tile();	} else	{		curr_piece = next_piece;	}}

It also does collision_check() in tile_down()! And collision_check() is a rather intensive function (at least it would cause some latency if it was done 30 times instead of only 15):

bool collision_check(){	for (int i = 0; i < 4; i++)	{		for (int j = 0; j < 4; j++)		{			if (next_piece.arr_size  [j] != NO_TILE)			{				if (i + next_piece.x == SCREENW || i + next_piece.x == -1 || j + next_piece.y == -1)				{					return false;				} else				{					if (j + next_piece.y < SCREENH)					{						if (arr_tetris_screen [i + next_piece.x] [j + next_piece.y] != NO_TILE)						{							return false;						}					}				}			}		}	}	return true;}

Now I need the collision_check() function in tile_down() for regular drops, but I don't need it in this second one, I believe. Would it be easier if I modified the tile_down() code and dropped it into the "return" handling code?

Adam_42

Jan 22, 2008 at 4:39 AM

That collision check function looks like it should be plenty quick enough to me. The code inside the loops will only get run 16 times for each call. Is it only slow when you press return?

Assuming it's not the real cause of the slowness needs to be found. Are you compiling it in release mode when testing performance? Have you profiled it? AMD have a free profiler called CodeAnalyst if you need one. That can tell you where you're spending most of your CPU time.

adam_o

Jan 22, 2008 at 8:55 AM

Quote:
Original post by Adam_42
That collision check function looks like it should be plenty quick enough to me. The code inside the loops will only get run 16 times for each call. Is it only slow when you press return?

Yes, only when I hit return. It's not very significant, like I said, 200 ms +/- 50, but that's a lot when you hit return in rapid succession and look and only see two bricks dropped down. It's the hesitation that messes it all up. The reason why I believe it is so slow is because it's not just running this 16 times, it's running it 16 times for every "collision_check()" in the code, which I believe to be about 30 in that loop. 30 * 16 = 480. So that code is being run 480 times, which may be causing the slowness.

Quote:
Original post by Adam_42
Assuming it's not the real cause of the slowness needs to be found. Are you compiling it in release mode when testing performance? Have you profiled it? AMD have a free profiler called CodeAnalyst if you need one. That can tell you where you're spending most of your CPU time.

Yes, I am compiling it in release form, why? Is that slower or something? I have not profiled it, but I have a Mac, so yours will not work, but Apple includes some in their developer tools, I'll check it out.

Trenki

Jan 22, 2008 at 9:15 AM

Assuming GCC is the compiler used under Mac you can also use some compiler switches for profiling. First of all you should make sure you compile with optimization turned on -O2 or -O3 does this.

For profiling with GCC you would have to compile and link your program with -pg and link with the gmon library.

After this you let the program run for a while and then quit. You can now use gprof on the executable to get the profiling results. Make sure you do not accidentally strip the debug symbols from the resulting executable or gprof won't work.

Zahlman

Jan 22, 2008 at 11:47 AM

What is translate()?

adam_o

Jan 22, 2008 at 6:01 PM

Quote:
Original post by Trenki
Assuming GCC is the compiler used under Mac you can also use some compiler switches for profiling. First of all you should make sure you compile with optimization turned on -O2 or -O3 does this.

For profiling with GCC you would have to compile and link your program with -pg and link with the gmon library.

After this you let the program run for a while and then quit. You can now use gprof on the executable to get the profiling results. Make sure you do not accidentally strip the debug symbols from the resulting executable or gprof won't work.

I have almost no clue what that means, all I know is that GCC is something that I use, although I don't know how or why.

To answer Zahlman's question, translate() is a function that translates the array that stores tile information and also translates the tile in motion, displaying it to the screen. No latency there.

Adam_42

Jan 23, 2008 at 4:57 AM

Try using Shark to profile it.

By the way, running that collision_check code 480 times a frame really shouldn't be that slow. Can you add some code to report how many times it is actually called when you press return?

adam_o

Jan 23, 2008 at 9:13 AM

I already posted that code in the form of the first post and then the collision_check() and other such code, but I'll do it again:

case SDLK_RETURN:{	if (!stop)	{		while (collision_check())		{			tile_down();		}			translate();	}	break;}//...bool collision_check(){	for (int i = 0; i < 4; i++)	{		for (int j = 0; j < 4; j++)		{			if (next_piece.arr_size  [j] != NO_TILE)			{				if (i + next_piece.x == SCREENW || i + next_piece.x == -1 || j + next_piece.y == -1)				{					return false;				} else				{					if (j + next_piece.y < SCREENH)					{						if (arr_tetris_screen [i + next_piece.x] [j + next_piece.y] != NO_TILE)						{							return false;						}					}				}			}		}	}	return true;}

Basically, the while loop makes collision_check() be called twice. I was thinking of adding a parameter to tile_down (either a 1 or 0) that basically told it whether or not to loop, because right now, every time it loops, it calls collision_check() twice for every one move down. The 480 number was assuming that the brick was at the very top and it had to check all the way down to the bottom. I think there may be an easier way to deal with this, but I don't know what...

Falken42

Jan 23, 2008 at 12:05 PM

Quote:
Original post by adam_o
I think there may be an easier way to deal with this, but I don't know what...

My suggestion would be to simply add a parameter to tile_down(), which tells it if it should call collision_check() or not. Since in your drop-down code you know for a fact that collision_check() is true at that point, you can just pass that information down as an optimization.

Something like this, perhaps:

void tile_down(bool skip_collision_check){	next_piece = curr_piece;	next_piece.y--;	if ((skip_collision_check == false) && !collision_check())   // <- changed this line	{		new_tile();	} else	{		curr_piece = next_piece;	}}// ...code...case SDLK_RETURN:{	if (!stop)	{		while (collision_check())		{			tile_down(true);     // <- changed this line		}			translate();	}	break;}

Then, everywhere else tile_down() is called (or to be more correct, anywhere you need tile_down() to perform a collision check), pass false to ensure the collision check is done.

adam_o

Jan 23, 2008 at 6:16 PM

Quote:
Original post by bpoint
Quote:
Original post by adam_o
I think there may be an easier way to deal with this, but I don't know what...

My suggestion would be to simply add a parameter to tile_down(), which tells it if it should call collision_check() or not. Since in your drop-down code you know for a fact that collision_check() is true at that point, you can just pass that information down as an optimization.

Something like this, perhaps:
*** Source Snippet Removed ***

Then, everywhere else tile_down() is called (or to be more correct, anywhere you need tile_down() to perform a collision check), pass false to ensure the collision check is done.

Ok, sounds great. Bpoint++.

Topic Locked

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