Using the observer pattern with collision detection

Question

My Game class has a property of type Player, which inherits from Entity:

player = new Player( "knight.png" );

It also has a property of type World, which has a collection of Entities (Robot inherits from Entity):

world = new World( new List<Entity>
{
    new Robot( "guy_1.png" ),
    new Robot( "guy_2.png", new MoveBehavior( Directions.Up ) )
};

Finally, it has a Physics object with which I register the Player and World properties:

physics.Register( player );
physics.Register( world );

This is a typical observer pattern. The Register method on the Physics class adds each object to its own collection of Entities.

public void Register( Entity entity )
{
    if ( !entity.Collidable )
    {
        return;
    }

    entity.Physics = this;

    entities.Add( entity );
}

public void Register( World world )
{
    foreach ( var entity in world.Entities.Where( e => e.Collidable ) )
    {
        Register( entity );
    }
}

In my game loop, I update each of the Entities:

foreach ( var entity in Entities )
{
    entity.Update( gameTime );
}

When Update is called on the Robot class, it will execute its behavior:

private readonly IBehavior behavior;

public override void Update( GameTime gameTime )
{
    if ( behavior != null )
    {
        behavior.DoIt( gameTime, this );
    }
}

Since there are two Robots in the World (see above), one of them will stand still and one of them will move. Now, when DoIt is called on the MoveBehavior class is where I do some collision detection:

private readonly Vector2 direction;

public bool DoIt( GameTime gameTime, Entity entity )
{
    if ( entity.DetectCollision( direction ) )
    {
        // Do nothing
        return false;
    }

    entity.Move( direction );

    return true;
}

(By the way, this is a simplified example. I'll get to my question really soon.)

Remember, when I registered the Player and World properties they each got assigned to them the Physics object. This is the DetectCollision method on the Entity class:

public bool DetectCollision( Vector2 direction )
{
    var newPosition = position + direction;

    if ( physics != null && physics.DetectCollision( newPosition, frame ) )
    {
        return true;
    }

    return false;
}

Finally we jump into the DetectCollision method on the Physics class. This method looks for a collision between any of the Entities that are registered with it:

private IList<Entity> entities;

public bool DetectCollision( Vector2 position, Rectangle frame )
{
    var collisions = entities.Count( e => e.Position.Y < position.Y + frame.Height
                                          && e.Position.X < position.X + frame.Width
                                          && e.Position.Y + e.Frame.Height > position.Y
                                          && e.Position.X + e.Frame.Width > position.X );

    // An entity will always collide with itself
    // We want to know if it will collide with another entity
    return collisions > 1;
}

Whew! I have a few problems with this implementation that I need advice on:

1. All of the properties of the Entity class are publicly exposed so they are accessible in the Physics class. e.g., I check entity.Collidable and I assign a value to entity.Physics inside the Register method, and I use e.Position and e.Frame in DetectCollision. I know I can change each of these to e.getCollidable(), e.setPhysics(), e.getPosition() and e.getFrame(), but I'm not sure what the point of doing that would be. Especially since I'm using C# auto-properties which does that for you anyways (I think).

2. It seems incredibly complicated. I realize collision detection is complicated no matter what you do but is there a better way to register all my objects with the physics engine?

I'm sure my algorithm could be better, but my primary concern (at least regarding this question) is the game design and not the actual collision detection algorithm.

Edit

I will try to clarify my first question. My Entity class has the following properties:

public bool Collidable { get; set; }

public Rectangle Frame { get; set; }

public IPhysics Physics { get; set; }

public Vector2 Position { get; set; }

I think I understand the principle of encapsulation enough. If anything I am probably confused about auto-properties. I believe the above will be compiled to something resembling this:

private Rectangle _collidable;

public Rectangle GetCollidable()
{
    return _collidable;
}

public void SetCollidable( Rectangle value )
{
    _collidable = value;
}

for each of the properties with {get; set;}. So my first question is: what is the point of doing that manually? Just so I can say my code is encapsulated? I know I can also do this:

public bool Collidable { get; private set; }

public Rectangle Frame { get; private set; }

public IPhysics Physics { get; private set; }

public Vector2 Position { get; private set; }

But doesn't that violate some principle of encapsulation? After some debate (with myself) my Entity class is looking like this:

private readonly bool collidable;

private readonly Rectangle frame;

private IPhysics physics;

private Vector2 position;

public bool IsCollidable()
{
    return collidable;
}

public void SetPhysics( IPhysics physics )
{
    this.physics = physics;
}

Then I refactored the DetectCollision method on the Physics class to call an overload of DetectCollision on the Entity class:

public bool DetectCollision( Vector2 position, Rectangle frame )
{
    var collisions = entities.Count( e => e.DetectCollision( position, frame ) );

    // An entity will always collide with itself
    // We want to know if it will collide with another entity
    return collisions > 1;
}

So now I have two versions of DetectCollision on Entity:

public bool DetectCollision( Vector2 direction )
{
    var newPosition = position + direction;

    if ( physics != null && physics.DetectCollision( newPosition, frame ) )
    {
        return true;
    }

    return false;
}

public bool DetectCollision( Vector2 otherPosition, Rectangle otherFrame )
{
    return position.Y < otherPosition.Y + otherFrame.Height
           && position.X < otherPosition.X + otherFrame.Width
           && position.Y + frame.Height > otherPosition.Y
           && position.X + frame.Width > otherPosition.X;
}

Is this better? I don't know! It seems to be a good compromise to me but it also seems insanely complicated. So the workflow goes something like this every time the game updates:

Game.Update() -> Entity.Update() -> Robot.Update()
-> MoveBehavior.DoIt() -> Entity.DetectCollision()
-> Physics.DetectCollision() -> Entity.DetectCollision() (overload)

And it does this like a million times per second!

Answer 1

Your vision about what is an auto-property is correct. Your first question is essentially whether you should use an auto-property or a property instead, which I answer this way:

Is the property implementation likely to change?

Example:

public class Person{
  public int Age{get; set;}
}

With this you are able to change the age to -1. To prevent that, you would change your code to the following:

public class Person{
  private int age;
  public int Age{
    get{return age;} 
    set{
      if(age >= 0)age = value;
      else throw new Exception();
    }
  }
}

You had to: Introduce a field, write get and set code. If you hadn't used an auto-property in the first place, you would have to write less. Although I leave up to you if you would rather use an auto-property or a property. For me there are some cases that I don't use them and they could fit. Properties do not disrespect the principle of encapsulation. In my example (in second code segment) if you exposed the field age you could set it to a negative number, but you couldn't do that if you used the property. Basically you can change the implementation of the property whenever you want, but you wouldn't have to change the code that references that property.

Your re-factored code is actually better and I am glad that you improved it that way! With that code you are able to unit test the method DetectCollision in a better way, and separate the collision logic.

Finally, if you have to register many objects on your physics object, you may have (at least) two approaches:

• Implement a fluent method call so you could write

  physics.Register(player).Register(world);
  

  instead of

  physics.Register(player);
  physics.Register(world);
  

• Or write a new class that includes all possible Entities and register it in your physics object. This can be bothersome if you want to add a new type of entity in the future and adds an additional piece that you have to remember (know its purpose). This is also like returning to the same problem because now you would have to register all objects here!

Answer 2

One thing to consider with this design is that the Physics class seems to have a case of Feature Envy. Perhaps the collision detection logic should live in the Physics class instead of the Entity class.

Given:

Physics physics = new Physics();
Entity[] surroundingEntities = ... ;

I may be speculating too much, but I think you want to be able to write in Entity:

var meInNewPosition = AfterSubmittedMove(this);
var anyCollisions = physics.Collisions(meInNewPosition, surroundingEntities).Any();
if (!anyCollisions) {
  this.CompleteMove();
} else {
  this.RejectMove();
}

where Collisions() would call DetectCollisions on all possible entities pairs. Any() ensures that it will return after the first one detected.

Think about the API you want between your classes, and then write methods to implement that API. Test Driven Development is invaluable here.

Answer 3

OK, what I don't like about your pattern and what I feel leads you to complain about its overcomplicatedness, is your behavior pattern.

The problem is that you want to have a generic DoIt() method on the behavior but a particular behavior has specific dependencies. In this case the physics object. You get around this by adding the physics object to Entity, but you can see how this can grow out of control. Say your robot either has wheels or hover propulsion, wheeled robots sink when they enter water, hover robots can move as normal. Now you need to add RobotFeet to your Entity so that the behavior can decide what to do.

Adding the property to Entity is the wrong way to go.

You could instead inject the Physics object into the Behavior when it is constructed

OR

Make a generic Behaviour so you can reference the subclass of entity in DoIt. ie. DoIt(Robot entity)

OR

Abandon the behavior pattern and put the move logic in the Robot class