Hello, I’m trying to make my code as clean as possible, but I’m having a bit of an issue trying to decide where I want my collision detection code. At the moment I have my code in a “PostUpdate” method in the main class:
foreach (var leftSprite in _sprites)
{
foreach (var rightSprite in _sprites)
{
// Don't do anything if they're the same sprite!
if (leftSprite == rightSprite)
continue;
// Don't do anything if they're not colliding
if (!leftSprite.Rectangle.Intersects(rightSprite.Rectangle))
continue;
if (leftSprite is Bullet)
{
if (leftSprite.Parent == rightSprite)
continue;
if (leftSprite.Parent is Player && rightSprite is Enemy)
{
leftSprite.IsRemoved = true;
var enemy = rightSprite as Enemy;
enemy.Health--;
if (enemy.Health <= 0)
enemy.IsRemoved = true;
}
if (leftSprite.Parent is Enemy && rightSprite is Player)
{
leftSprite.IsRemoved = true;
var player = rightSprite as Player;
player.Health--;
if (player.Health <= 0)
player.IsRemoved = true;
}
}
if (leftSprite is Player && rightSprite is Enemy)
{
var player = leftSprite as Player;
var enemy = rightSprite as Enemy;
player.Health--;
if (player.Health <= 0)
player.IsRemoved = true;
enemy.Health--;
if (enemy.Health <= 0)
enemy.IsRemoved = true;
}
if (leftSprite is Enemy && rightSprite is Player)
{
var player = rightSprite as Player;
var enemy = leftSprite as Enemy;
player.Health--;
if (player.Health <= 0)
player.IsRemoved = true;
enemy.Health--;
if (enemy.Health <= 0)
enemy.IsRemoved = true;
}
}
}
The general gist is I loop around all my sprites twice. If they collide, then do something depending on the types of the sprites.
I have a feeling I’m going about it completely wrong.
Could I get some advice on how I could clean this up?
At the end of the day, you’ve gotta put this code somewhere. There are a lot of best practices and guidelines for coding that I can suggest to you, but ultimately it’s up to you to decide if you want to follow them or not.
For example, not only is this a big block of code that goes in your main loop, it’s also not very flexible. By this I mean that if you decide you want to do something else on a collision, or if you want to add another object that handles collision differently, you’ve gotta extend this code and start adding checks and special cases. This might be fine with you or it might not be, again, it’s kind of up to you.
If you did want to make a change, maybe consider isolating some of this code and breaking up the tasks, splitting them into different classes that handle more generic pieces of work. Here’s some rough thoughts…
Each class can exist in a container. It’s the container’s job to own all of the sprites and, during its update method, loops through all of the sprites and tells them to update. (Handles your two for loops above)
Make a base sprite class that handles the bulk of your sprite code. Then derive classes from that base class to handle each of your specialized sprite (ie, Bullet).
This base sprite class maybe has a reference to the container that owns it, so it can get access to other sprites that exist with it, if it wants to.
Each sprite maybe has a reference to an object responsible for checking if it collides against another sprite. This object can be null if no collision checking for a sprite is necessary, but if not null, it can maybe loop through all the sprites in the parent container and check to see if a collision occurred.
If a collision does occur, this collision object can inform the sprite that owns it (event, delegate, etc…).
A sprite object has a method, say, OnCollides which has a parameter (BaseSprite other) that is the object it collided with. In this method it can maybe make a decision as to what to do when it has collided with something. In your case above, decrement health, check if dead, and if dead, set some kind of flag.
The container class can check for dead sprites and remove them (if desired).
This will result in pretty much the same stuff you’ve got above, though it will likely take a lot more code to realize. The advantage is that it’s much easier to read, maintain, and extend. It takes time up front, but the payoff is that it saves you a lot of time as you build your game and add more stuff to it.
As stated, what you do is entirely up to you. If you have this and it’s working for you, maybe just keep it. If you decide later that you want to add something to it, maybe that’s the time to refactor your code to make it more robust… or maybe the thing you need to add is fairly easy and you think it’s gonna be the “last thing for reals”, then just add it in. However, if you add another item after that, maybe that is the real time you want to refactor
Anyway, just some thoughts/ideas for you to consider. I hope it was helpful!
It’s really not good practice to have lots of ‘if xxx is xxx’ in your code - plus it’s confusing to read.
First I’d say subclass your Sprite class to have separate Player and Enemy classes, that will make things much clearer.
With the vast majority of collision detection I find it best to do it as the last thing in the update loop, so update the position of all sprites first, then run another loop to update collision detection for all sprites. If you separate different types of sprites into different classes and have a generic ‘DoCollisionDetection’ method that can be overridden it’s easy to make sure you only run the appropriate types of checks for each type of sprite (ie you only have to check a bullet collides with the player, you don’t have to do it the other way round as well).
For sprites that are often accessed from all over the place (e.g. the player) I usually cheat by having some kind of singleton reference that can easily be accessed from anywhere, like MyGame.Player. Some purists would frown at this but screw 'em - it makes life a hell of a lot easier and I think it makes code more readable.
