2D Platformer Collision Detection/Resolution

Question

I am working on a 2D tile based platformer and was hoping to get someone to review my collision detection. I've only included the X collision check since the Y is essentially the same. Any suggestions/criticisms are appreciated.

 // Corrects X movement if a collision occurs.
 private void checkXCollision()
 {
 // Find the range of tiles the player intersects
 Point topLeft = new Point(((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize(), ((int)ytemp - collisionHeight/2) / map.getTileSize());
 Point bottomRight = new Point(((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize(), ((int)ytemp + collisionHeight/2) / map.getTileSize());
 // Left
 if(dx < 0)
 {
 boolean collision = false;
 for(int row = topLeft.y; row <= bottomRight.y; row++)
 {
 if(map.getType(row, topLeft.x) == Tile.SOLID) 
 { 
 collision = true; 
 break;
 }
 }
 if(collision)
 {
 dx = 0;
 // Move player to edge of solid tile
 int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();
 xtemp = col * map.getTileSize() + collisionWidth / 2;
 }
 else
 {
 xtemp += dx;
 }
 }
 // Right
 if(dx > 0)
 {
 boolean collision = false;
 for(int row = topLeft.y; row <= bottomRight.y; row++)
 {
 if(map.getType(row, bottomRight.x) == Tile.SOLID) 
 { 
 collision = true;
 break;
 }
 }
 if(collision)
 {
 dx = 0;
 // Move player to edge of solid tile
 int col = ((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize();
 xtemp = (col + 1) * map.getTileSize() - collisionWidth / 2 - 1;
 }
 else
 {
 xtemp += dx;
 }
 }
 }

Answer 1

• You have some code duplication there

  for(int row = topLeft.y; row <= bottomRight.y; row++)
  {
   if(map.getType(row, topLeft.x) == Tile.SOLID) 
   { 
   collision = true; 
   break;
   }
  }
  

  which should be extracted to a method like

  private boolean hasCollision(int start, int stop, int x){
   for(int row = start; row <= stop; row++)
   {
   if(map.getType(row, x) == Tile.SOLID) 
   { 
   return true;
   }
   }
   return false;
   } 
  

• If a collision is found you have this

  dx = 0;
  // Move player to edge of solid tile
  int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();
  

  Do you on purpose set dx = 0 and calling then (xtemp + dx) ?

• You are also doing a lot of collisionWidth/2 which should be extracted to a variable.

• you should extract the left and right checking to separate methods.

• in Java the convention is to place the opening brace { on the same line.

• using a guard clause for dx == 0 well remove the creation of the Point's

• comments should describe why something is done. **What is done should be described by the code itself by using meaningful names.

Implementing most the points above leads to

// Corrects X movement if a collision occurs.
private void checkXCollision()
{
 if (dx == 0) {
 return;
 }
 Point topLeft = new Point(((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize(), ((int)ytemp - collisionHeight/2) / map.getTileSize());
 Point bottomRight = new Point(((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize(), ((int)ytemp + collisionHeight/2) / map.getTileSize());
 if(dx < 0){
 checkLeftXCollision(topLeft, bottomRight);
 } else {
 checkRightXCollision(topLeft, bottomRight);
 }
}
private void checkLeftXCollision(Point topLeft, Point bottomRight) {
 if(hasCollision(topLeft.y, bottomRight.y, topLeft.x)) {
 dx = 0;
 int col = ((int)(xtemp + dx) - collisionWidth/2) / map.getTileSize();
 xtemp = col * map.getTileSize() + collisionWidth / 2;
 } else {
 xtemp += dx;
 }
}
private void checkRightXCollision(Point topLeft, Point bottomRight) {
 if(hasCollision(topLeft.y, bottomRight.y, bottomRight.x)) {
 dx = 0;
 int col = ((int)(xtemp + dx) + collisionWidth/2) / map.getTileSize();
 xtemp = (col + 1) * map.getTileSize() - collisionWidth / 2 - 1;
 } else {
 xtemp += dx;
 }
} 
private boolean hasCollision(int start, int stop, int x){
 for(int row = start; row <= stop; row++)
 {
 if(map.getType(row, x) == Tile.SOLID) 
 { 
 return true;
 }
 }
 return false;
}