2D Collision Detection

Question

I wrote this a while ago but wanted to see if I went about it the right way and if my brain is working correctly. I was thinking about these two projects the other day and I recently released the source. This game was originally for a Windows Form Game Jam so you have full context.

This code worked but I always felt there were inaccuracies in how it handled it. For example, there had been times a rock would clip through a ship.

private void CollisionDetection()
{
 foreach(PictureBox s in rocks)
 {
 //These tell the difference between, I'm not sure if this is all correct, its just what I came up with.
 int Xdifference = (s.Location.X + (s.Width / 2)) - (player.Location.X + (player.Width / 2));
 int Ydifference = (s.Location.Y + (s.Height/ 2)) - (player.Location.Y + (player.Height/ 2));
 //get the actual distance apart
 double dist = Math.Sqrt((Xdifference * Xdifference) + (Ydifference * Ydifference));
 //And finally, does the real comparison
 if(dist < ((s.Width / 2) + (player.Width / 2)) && damagable)
 {
 health -= 10;
 damagable = false;
 impactHit.Play();
 }
 }
}

Full file is found here and full source, here.

Answer 1

Collision detection

You can calculate the collision much easier with the Rectangle structure.

Use the Contains method if you are interested in single points that

Determines if the specified point is contained within this Rectangle structure.

var pictureRect = new Rectangle(
 pictureBox.Location.X, 
 pictureBox.Location.Y, 
 pictureBox.Width, 
 pictureBox.Height
);
var isCollision = pictureRect.Contains(player.Location.X, player.Location.Y);

---

If you want to know whether the rectangles have a collision then use the IntersectsWith method that

Determines if this rectangle intersects with rect.

var isCollision = pictureRect.IntersectsWith(player.Rectangle());

---

Create and an extension to easier get the rectangle from a PictureBox.

public static Rectangle Rectangle(this PictureBox pictureBox)
{
 return new Rectangle(
 pictureBox.Location.X, 
 pictureBox.Location.Y, 
 pictureBox.Width, 
 pictureBox.Height
 );
}

---

Naming

foreach(PictureBox s in rocks)

You should use more meaningful names than just s. A variable like pictureBox or pb would be much better because it's derived from the collection. The s does not have any meaning. Although in this case a rock would be perfect.

---

Health & Damage

health -= 10;

This operation is not clear at first and maybe you have other places where you need to decrease health. Consider using constants and a helper method for this.

private static class Damage
{
 public const int WhenHitRock = 10;
}
void DecreaseHealth(int value)
{
 health -= value;
}

---

Everything together

The result of applying all suggestions and proper names would be a self-documenting code that no longer has to be commented.

When you write comments you only should explain why you are doing something because the code already shows what.

foreach (var rock in rocks)
{ 
 var isPlayerRockCollision = rock.Rectangle().IntesectsWith(player.Rectangle());
 if (isPlayerRockCollision)
 {
 DecreaseHealthBy(Damage.WhenHitRock);
 damagable = false;
 impactHit.Play();
 return;
 }
}

Notice that there is a return inside the if. You set the demagable to false so I think it's not necessary to test other rocks for a hit. You can stop at the first one.

• Denis: This should be the accepted answer in my opinion. IntersectsWith() is the method if you need to detect collisions 95% of the time.
• Tyler C: Added as accepted answer. It kept getting edited and got better and better. Thanks a lot.
• t3chb0t: @TylerC yeah, the ideas kept comming so I needed to edit it multiple times - hope it helps ;-)
• Tyler C: @t3chb0t this is perfect, I wasn't sure I was going about it the right way. I appreciate the time and effort you put into this.
• t3chb0t: @NumLock I've googled for it and this sounds like a good idea - at least for more complex projects but I've never used it myself before. You're welcome to add a review about it.

Answer 2

Just a quick answer.

• You are calculating s.Width / 2 for each Picturebox two times. Just calculate it once, store the result in a variable and use the variable.

• You are calculating player.Width / 2 twice for each Picturebox which should be done outside of the loop and stored in a variable. The same applies to the calculation of player.Height/ 2. This values won't change unless they are changed inside impactHit.Play().

• You should check in the if condition first if damageble is true because its faster .

• Tyler C: Thanks for the help, on the third though: If the first condition is false it skips the second calculation?
• Heslacher: Yes thats right when using the && operator.
• Der Kommissar: @Heslacher (player.Location.X + (player.Width / 2)) can all be calculated before the loop, same with Y / Height.
• Der Kommissar: Likewise, you should test performance between the current code and (int)(s.Location.X * 1.5) (same with Y).