Why isn't this moving?

Started by
13 comments, last by Programmer557 10 years, 11 months ago

All right, I've made these slight changes and the sprite moves now, but it will only move down, and it will not move more than once. The message "You Cannot Move Here" also appears. This isn't supposed to happen unless the player is at the edge of the screen and tries to move in the direction of the edge. Here is the altered code:


if (SDLK_UP) 
					{
						if (PlayerLocationy == 0)
						{
							Message = DeniedMessage;
							ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL);
							Message = NULL;
						}
						else
						{
							PlayerLocationy = PlayerLocationy - 42;
							ApplySurface(0,421,Sprites,Screen,&TextBox);
						}
					}
					
					if (SDLK_DOWN) 
					{
						if (PlayerLocationy == 378)
						{
							Message = DeniedMessage;
							ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL);
							Message = NULL;
						}
						else
						{
							PlayerLocationy = PlayerLocationy + 42;
							ApplySurface(0,421,Sprites,Screen,&TextBox);
							break;
						}
					}
					
					if (SDLK_LEFT) 
					{
						if (PlayerLocationx == 0)
						{
							Message = DeniedMessage;
							ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL);
							Message = NULL;
						}
						else
						{
							PlayerLocationx = PlayerLocationx - 42;
							ApplySurface(0,421,Sprites,Screen,&TextBox);
						}
					}
					
					if (SDLK_RIGHT) 
					{
						if (PlayerLocationx == 378)
						{
							Message = DeniedMessage;
							ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL);
							Message = NULL;
						}
						else
						{
							PlayerLocationx = PlayerLocationx + 42;
							ApplySurface(0,421,Sprites,Screen,&TextBox);
						}
					}
					
					if (SDLK_ESCAPE) 
					{
						Message = EscapeMessage;
						ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL);

						if (SDL_KEYDOWN == SDLK_y)
						{
							QUIT = true;
						}
					}

I noticed that Khatharr had said something about not repeating the same functions, but I don't know how to do that quite yet.

Advertisement

if (SDLK_UP)


This isn't quite right, either. SDLK_UP, SDLK_DOWN, SDLK_LEFT, SDLK_RIGHT, and SDLK_ESCAPE are non-zero constants; all of these compare true no matter what. You want to test if the key symbol from the event is equal to these constants.

Additionally, you should use if-else-if blocks, because an event cannot be contain than one key press at a time, so it is a waste of time for your machine to check if it is each symbol after it already found which one it is. This would be perfect for a switch statement, if you are proficient in their use.

if (SDLK_ESCAPE) { Message = EscapeMessage; ApplySurface((ScreenWidth - Message->w)/2,(ScreenHeight - Message->h + 420)/2,Message,Screen,NULL); if (SDL_KEYDOWN == SDLK_y) { QUIT = true; } }


There's still this problem, here.
This code is just in response to what you asked me about. There's still problems here that other people have touched on. Additionally, your render code seems too close to your logic. You should try to separate these into different sections unless the program structure is exceedingly simplistic.
const int MOVE_DIST = 42;
const int BOUNDARY_MIN = 0;
const int BOUNDARY_MAX = 378;

//Handle movement (see other people's comments about testing against SDLK_ constants)
if(SDLK_UP)    {PlayerLocationy -= MOVE_DIST;}
if(SDLK_DOWN)  {PlayerLocationy += MOVE_DIST;}
if(SDLK_LEFT)  {PlayerLocationx -= MOVE_DIST;}
if(SDLK_RIGHT) {PlayerLocationx += MOVE_DIST;}

//Correct out-of-bounds condition
Message = NULL;
if(PlayerLocationy < BOUNDARY_MIN) {
  Message = DeniedMessage;
  PlayerLocationy = BOUNDARY_MIN;
}
if(PlayerLocationy > BOUNDARY_MAX) {
  Message = DeniedMessage;
  PlayerLocationy = BOUNDARY_MAX;
}
if(PlayerLocationx < BOUNDARY_MIN) {
  Message = DeniedMessage;
  PlayerLocationx = BOUNDARY_MIN;
}
if(PlayerLocationy > BOUNDARY_MAX) {
  Message = DeniedMessage;
  PlayerLocationx = BOUNDARY_MAX;
}

//Render
if(Message != NULL) {
  int mess_x = (ScreenWidth - Message->w) / 2;
  //This next line is not clear. Get rid of the magic number and clarify order of operations
  int mess_y = (ScreenHeight - Message->h + 420) / 2;

  ApplySurface(mess_x,mess_y,Message,Screen,NULL);

  Message = NULL;
}
else {
  ApplySurface(0,421,Sprites,Screen,&TextBox);
}

The key here is that the things you're likely to change are easy to change and it's clear what's going on. You can change the constants at the top or the render stuff at the bottom and not have to worry about changing it all 4 times. The organization and alignment of the first set of tests also makes it easier to spot if there's any errors lurking there, such as having PlayerLocationy where there should be a PlayerLocationx, or a - where there should be a +.
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Additionally, your render code seems too close to your logic. You should try to separate these into different sections unless the program structure is exceedingly simplistic.


+1 for separation between logic and rendering.

This code is just in response to what you asked me about. There's still problems here that other people have touched on. Additionally, your render code seems too close to your logic. You should try to separate these into different sections unless the program structure is exceedingly simplistic.


const int MOVE_DIST = 42;
const int BOUNDARY_MIN = 0;
const int BOUNDARY_MAX = 378;

//Handle movement (see other people's comments about testing against SDLK_ constants)
if(SDLK_UP)    {PlayerLocationy -= MOVE_DIST;}
if(SDLK_DOWN)  {PlayerLocationy += MOVE_DIST;}
if(SDLK_LEFT)  {PlayerLocationx -= MOVE_DIST;}
if(SDLK_RIGHT) {PlayerLocationx += MOVE_DIST;}

//Correct out-of-bounds condition
Message = NULL;
if(PlayerLocationy < BOUNDARY_MIN) {
  Message = DeniedMessage;
  PlayerLocationy = BOUNDARY_MIN;
}
if(PlayerLocationy > BOUNDARY_MAX) {
  Message = DeniedMessage;
  PlayerLocationy = BOUNDARY_MAX;
}
if(PlayerLocationx < BOUNDARY_MIN) {
  Message = DeniedMessage;
  PlayerLocationx = BOUNDARY_MIN;
}
if(PlayerLocationy > BOUNDARY_MAX) {
  Message = DeniedMessage;
  PlayerLocationx = BOUNDARY_MAX;
}

//Render
if(Message != NULL) {
  int mess_x = (ScreenWidth - Message->w) / 2;
  //This next line is not clear. Get rid of the magic number and clarify order of operations
  int mess_y = (ScreenHeight - Message->h + 420) / 2;

  ApplySurface(mess_x,mess_y,Message,Screen,NULL);

  Message = NULL;
}
else {
  ApplySurface(0,421,Sprites,Screen,&TextBox);
}

The key here is that the things you're likely to change are easy to change and it's clear what's going on. You can change the constants at the top or the render stuff at the bottom and not have to worry about changing it all 4 times. The organization and alignment of the first set of tests also makes it easier to spot if there's any errors lurking there, such as having PlayerLocationy where there should be a PlayerLocationx, or a - where there should be a +.

Thank you very much. With some slight modification, my program is now working how I want it to. Thank you also, everyone else who posted.

This topic is closed to new replies.

Advertisement