-1
\$\begingroup\$

I'm having an issue with my collision detection code. I'm making a top down shooter and when I shoot, the bullet will only destroy an enemy if it's the closest enemy. The bullet will go right through the 2nd closest enemy.

Here is my code:

// Iterate through bullets, if bullet ccollides with enemy, remove both
for (int i = 0; i < Bullet.Bullets.Count; i++)
{
    if (Collisions.IsColliding(
      (int)Bullet.Bullets[i].position.X, (int)Bullet.Bullets[i].position.Y, 
      Bullet.sprite.Width, Bullet.sprite.Height,
      (int)Enemy.Enemies[i].position.X, (int)Enemy.Enemies[i].position.Y, 
      Enemy.sprite.Width, Enemy.sprite.Height))
    {
        Enemy.Remove(Enemy.Enemies[i]);
        Bullet.Remove(i);
    }
}

And here is my is colliding method:

public static bool IsColliding(int x1, int y1, int w1, int h1, int x2, int y2, int w2, int h2)
{
    return x1 < x2 + w2 &&
           x2 < x1 + w1 &&
           y1 < y2 + h2 &&
           y2 < y1 + h1;
}

Can anyone please point me in the right direction? Thank you.

\$\endgroup\$
4
  • \$\begingroup\$ What is the expected behaviour? \$\endgroup\$
    – Vaillancourt
    Feb 9, 2016 at 13:38
  • \$\begingroup\$ The expected behaviour is that the enemy and bullet is removed even if it isn't the closest to the player \$\endgroup\$ Feb 9, 2016 at 13:41
  • \$\begingroup\$ What is Bullet.Bullets? What type of collection is it? \$\endgroup\$
    – Vaillancourt
    Feb 9, 2016 at 13:49
  • \$\begingroup\$ It's a list of Bullets \$\endgroup\$ Feb 9, 2016 at 13:50

1 Answer 1

2
\$\begingroup\$

You are only checking th i-th bullet with the enemy of the same index and not with every enemy there is.

You should do something like this

for (int i = 0; i < Bullet.Bullets.Count; i++)
{
    for (int j = 0; j < Enemy.Enemies.Count; j++)
    {
        if (Collisions.IsColliding(
          (int)Bullet.Bullets[i].position.X, (int)Bullet.Bullets[i].position.Y, 
          Bullet.sprite.Width, Bullet.sprite.Height,
          (int)Enemy.Enemies[j].position.X, (int)Enemy.Enemies[j].position.Y, 
          Enemy.sprite.Width, Enemy.sprite.Height))
        {
            Enemy.Remove(Enemy.Enemies[j]);
            Bullet.Remove(i);
        }
    }
}

This way you will check every bullet with every enemy, it's not the most efective way of doing it but it should work if you don't have tons of bullets and enemies at the same time.

\$\endgroup\$
7
  • 2
    \$\begingroup\$ This will still modify the collections based on indices while iterating through them. I'm not sure about c#, but this is potentially dangerous and prone to crashes and/or bugs and/or undefined behaviours. \$\endgroup\$
    – Vaillancourt
    Feb 9, 2016 at 13:54
  • \$\begingroup\$ Face palm... Should probably not write code at 3am then wake up at 8am for work. Thanks very much. \$\endgroup\$ Feb 9, 2016 at 13:54
  • \$\begingroup\$ @AlexandreVaillancourt called it. I get an argument is out of range exception when I shoot a bullet at any enemy but the closest it throws an exception. Here is the full code: gist.github.com/robbiewoods05/4661196fd2396db230d3 \$\endgroup\$ Feb 9, 2016 at 14:27
  • 1
    \$\begingroup\$ No problem :) But be aware that if you use break in there this code will find at most 1 collision per execution, so if you have many bullets and enemies at the same time some bullets may end up going right through the targets, if you don't want that to happen you should consider one of the other 2 solutions. \$\endgroup\$ Feb 9, 2016 at 15:24
  • 1
    \$\begingroup\$ To fix this I'd usually do something like Enemy.Enemies[i].IsDead = true; and then after finishing iteration write Enemy.Enemies.RemoveAll(e => { return e.IsDead;} ); so removals happen in a batch after we've found our collisions. One can also swap if order isn't important and you want to avoid copy costs of removing enemies early in the list. \$\endgroup\$
    – DMGregory
    Feb 9, 2016 at 15:46

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .