Entities and the things they do

Question

So I have an Entity, which is responsible for providing an abstract base for other objects to inherit. (Like Actor objects, which are people, Creature objects, which are animals, etc.)

The Entity provides a solid foundation for Actor and Creature to interact with, I'm just curious if it's not gone overboard or if I've missed something.

---

Library: Framework (Client and Server)

The Entity:

public abstract class Entity : ITrackableObject
{
    protected bool _isMoving;
    protected Size _size = new Size(32, 64);
    private PointF _position;

    public bool IsMoving { get { return _isMoving; } }
    public Guid Id { get; set; }
    public abstract EntityType EntityType { get; }
    public GenderType Gender { get; set; }
    public PointF Position { get { return _position; } set { if (_position != value) { var oldPosition = _position; _position = value; OnTrackableObjectChanged(new TrackableObjectChangedEventArgs(_position, value, Size, Size)); } } }
    public Direction Direction { get; set; }
    public Size Size { get { return _size; } }

    public RectangleF Bounds { get { return new RectangleF(Position, Size); } }

    public EntityProperties EntityProperties { get; set; }

    public Color NameColor { get; set; }

    public Point Home { get; set; }

    protected List<IInstruction> _instructions = new List<IInstruction>();
    public List<IInstruction> Instructions { get { return _instructions; } }

    public Entity()
    {
        EntityProperties = new EntityProperties();
        NameColor = Color.Preset.White;
        _instructions = new List<IInstruction>();
    }

    public virtual void Update(UpdateState updateState)
    {
        foreach (IInstruction instruction in Instructions)
            instruction.Update(updateState);

        Move(updateState.Force);
    }

    public virtual void Move(Vector2F vector)
    {
        if (vector.R > 0)
            _isMoving = true;
        else
            _isMoving = false;

        float r = Math.Max(Math.Abs(vector.X), Math.Abs(vector.Y));
        vector = Vector2F.FromRTheta(r, vector.Theta);
        Position = new Evbpc.Framework.Drawing.PointF(Position.X + vector.X, Position.Y + vector.Y);

        Direction? d = vector.GetDirection();

        if (d.HasValue)
            Direction = d.Value;
    }

    private void OnTrackableObjectChanged(TrackableObjectChangedEventArgs e) { var handler = _TrackableObjectChanged; if (handler != null) { handler(this, e); } }

    private EventHandler<TrackableObjectChangedEventArgs> _TrackableObjectChanged;

    PointF ITrackableObject.Position { get { return Position; } }

    SizeF ITrackableObject.Size { get { return Size; } }

    event EventHandler<TrackableObjectChangedEventArgs> ITrackableObject.TrackableObjectChanged { add { _TrackableObjectChanged += value; } remove { _TrackableObjectChanged -= value; } }
}

The UpdateState is pretty simple:

public class UpdateState
{
    public Vector2F Force { get; set; }
}

Then an Actor comes along:

public abstract class Actor : Entity
{
    public Inventory Inventory { get; set; }
    public List<Injury> Injuries { get; private set; }

    public Actor()
    {
        Injuries = new List<Injury>();
        Inventory = new Inventory();
    }
}

And a NonPlayableCharacter:

public class NonPlayableCharacter : Actor
{
    public sealed override EntityType EntityType { get { return EntityType.NonPlayableCharacter; } }

    public NonPlayableCharacter(Point origin)
        : base()
    {
        this.Position = origin;
        _instructions.Add(new WanderInstruction(this, 10f, Point.Round(this.Position)));
    }

    public override void Update(UpdateState updateState)
    {
        base.Update(updateState);
    }
}

And lastly, a Player:

public class Player : Actor
{
    public sealed override EntityType EntityType { get { return EntityType.Player; } }
}

---

Library: Framework.Xna (Client)

Pretty simple. Until you get to the other player, which is in a different project:

public class Player : Framework.Entities.Actors.Player, IDrawableActor, IUpdateableActor
{
    private Texture2D _texture;

    public Texture2D Texture { get { return _texture; } set { _texture= value; } }

    public void Draw(SpriteBatch s, GameTime gameTime)
    {
        int xSource = 0; // Down   
        if (Direction == Direction.Up)
            xSource = 3; // Up
        if (Direction == Direction.Left)
            xSource = 6; // Left
        if (Direction == Direction.Right)
            xSource = 9;

        if (IsMoving)
        {
            s.Draw(Texture,
                new Microsoft.Xna.Framework.Rectangle((int)Position.X,
                    (int)Position.Y,
                    Size.Width,
                    Size.Height),
                new Microsoft.Xna.Framework.Rectangle((xSource * Size.Width) + ((gameTime.TotalGameTime.Milliseconds / 125 % 3) * Size.Width),
                    Size.Height,
                    Size.Width,
                    Size.Height),
                Microsoft.Xna.Framework.Color.White,
                0,
                new Microsoft.Xna.Framework.Vector2(0, 0),
                //(e.Direction == Direction.Left ? SpriteEffects.FlipHorizontally : SpriteEffects.None),
                SpriteEffects.None,
                0);
        }
        else
        {
            s.Draw(Texture,
                new Microsoft.Xna.Framework.Rectangle((int)Position.X,
                    (int)Position.Y,
                    Size.Width,
                    Size.Height),
                new Microsoft.Xna.Framework.Rectangle((xSource * Size.Width),
                    0,
                    Size.Width,
                    Size.Height),
                Microsoft.Xna.Framework.Color.White,
                0,
                new Microsoft.Xna.Framework.Vector2(0, 0),
                //(e.Direction == Direction.Left ? SpriteEffects.FlipHorizontally : SpriteEffects.None),
                SpriteEffects.None,
                0);
        }
    }

    public override void Update(UpdateState uState)
    {
        base.Update(uState);
    }
}

There's an IDrawableActor:

public interface IDrawableActor : IDrawableEntity
{
}

Finally, the IDrawableEntity:

public interface IDrawableEntity
{
    Texture2D Texture { get; set; }

    void Draw(SpriteBatch s, GameTime gameTime);
}

My main concern is whether or not this is necessary. I have to split drawing code out of the main Player class, so that the Client and Server can reference this library.

Obviously I'm interested in all suggestions, but of particular importance to me:

• In C# you can only inherit one class. The drawing code in Framework.Xna.Player.Draw would likely need to be duplicated for anything that is an IDrawableEntity. Is this a good approach?
• Should I create interfaces for Entity, Actor, Framework.Player and Framework.Xna.Player?
• At some point, I would like to expand Entity to also be able to represent objects. This would mean moving a lot of the code in this Framework.Entity class into a Framework.MovingEntity. Are there any other concerns I should consider?

Answer 1

Any time you see things like "Size", "Gender", and "Home" in the same class, yes, you've gone overboard. This class violates a number of object-oriented design principles. You want reusable logic that keeps flexibility to throw your monsters like rocks and cast spells on your rocks that turn them into rockmonsters. At least that's what I wanted when I tried to do a very similar design to this, and it devolved into complete chaos, mainly because I was violating the:

Single Responsibility Principle (SRP)

The SRP states that one class/method/module should only have one reason to change. Your Entity class has dozens. You will want to improve your physics beyond "IsMoving", "Position", and "Direction". You will come up with common life-form attributes other than "Gender". You will want to know more about an Entity's habits and lifecycle than just "Home". All of these changes will force Entity to change, bloat, and become unmaintainable. All of these responsibilities should be broken out into separate classes. How? Well, to start:

Favor Composition Over Inheritance

I can see you struggling with your inheritance tree. A Player is an Actor which is a MovingEntity which is an Entity. What about ThrowableEntity? EatableEntity? StorableEntity? KillableEntity? You're enshrining certain aspects of game objects to the detrement of every other aspect you might care about in the future. This shows that inheritence is not the right architecture to represent the relationships between these aspects. Favor composition instead. There are many ways to do this, but a great place to start is:

Entity-Component-System (ECS) Architecture

The ECS architecture was invented in the game development community for this exact purpose: you want to be able to mix and match aspects arbitrarily. Some things are throwable + collectable (rocks), movable + killable (monsters), operable (switches), killable + operable (doors), collectable + edible (mushrooms), movable + killable + edible (mushroom monsters), etc. Each of those aspects gets one devoted System, which operates over some set of Components.

A Component is a concise set of data which describes an entity's state relative to some aspect. For instance, an Entity may have components like "Position" {int X, int Y}, "Gender", "Home", "Texture" etc. These are not properties in the Entity class, but separate anemic Value Types that are associated with an entity.

Components don't have behavior; instead, Systems have the responsibility of operating on specific types of components. The Movement system cares about the Position and Velocity components, the Ecology system cares about Gender and Fertility, the Rendering system cares about Textures and NameColors. Periodically, a system loops through all the components it knows about and acts on them. For instance, the Movement system takes all the positions and velocities, adds velocities to positions, then stores the new positions. When the system fires can vary: the Movement system may fire 60 times per second, but the Ecology system might only fire every time an in-game day elapses.

I strongly recommend you read the Wikipedia article and browse the Entity-systems wiki.

Answer 2

You could change this:

    if (vector.R > 0)
        _isMoving = true;
    else
        _isMoving = false;

to this:

_isMoving = vector.R > 0;

---

You also left some Dead Code in this as well, clean out your cobwebs comments before posting :)

Answer 3

Some pointers about your properties

public PointF Position { get { return _position; } set { if (_position != value) { var oldPosition = _position; _position = value; OnTrackableObjectChanged(new TrackableObjectChangedEventArgs(_position, value, Size, Size)); } } }

What should I say other than DON'T do something like this because

• setting a breakpoint is almost impossible
• it is not readable
• you will come into problems if you need to maintain it or try to find a bug

So let me say this again: DON'T DO THIS

public bool IsMoving { get { return _isMoving; } }  

This should be using autoimplemented properties like

public bool IsMoving { get; protected set; }  

You don't do any validation if you are changing the protected bool _isMoving so there is no need to introduce a separate variable for this.

The same is true for

public Size Size { get { return _size; } }  

---

protected List<IInstruction> _instructions = new List<IInstruction>();  

Either initialize like this or in the constructor. Doing both is not needed.

Also there is no need that beeing a List<T> a IList<T> is sufficient.

By changing the property public List<IInstruction> Instructions to using autoimplemented property like above (using a protected setter) should be done too.

---

Client.Draw()

Here you have some duplicated code. The only difference for IsMoving or not is the second parameter for the s.Draw() method. Putting the creation of this parameter inside the if and else and calling the method after the if..else will remove the code duplication.

---

About the questions which have particular importance to you, I quite don't get it, but try to answer anyway:

• If the IDrawableEntity interface doesn't contain Draw(SpriteBatch, GameTime) then I would either add it or create an interface having only the Draw(SpriteBatch, GameTime) and let the Player implement it. Then by using composition (over inheritance) I would create an object only responsible for the drawing part which is called by the implemented method of the interface.

• If you expect more classes coming which needs the same methods etc yes, you should create interfaces. Also for testing purpose it is always better to have the object implement a interface, so mocking can be done easily.

• I don't understand ;-)

Answer 4

public virtual void Update(UpdateState updateState)
{
    foreach (IInstruction instruction in Instructions)
        instruction.Update(updateState);

    Move(updateState.Force);

1. The declaration for instruction is a bit verbose. Prefer the var keyword when the type is obvious. (This use isn't entirely obvious, but certainly good enough.)
2. Brackets. Use them. Always.

---

public Actor()
{
    Injuries = new List<Injury>();
    Inventory = new Inventory();
}

You really shouldn't be creating dependencies in your constructor. This can make code a nightmare to test. Thankfully, this is pretty easy to fix.

public Actor(IList<Injury> injuries, Inventory inventory)
{
    Injuries = injuries;
    Inventory = inventory;
}

Don't want to actually create these dependencies outside of the class? Fine. Use a poor man's IoC container.

public Actor(IList<Injury> injuries, Inventory inventory)
{
    Injuries = injuries;
    Inventory = inventory;
}

public Actor()
    :this(new List<Injuries>(), new Inventory())
{ }

But wait! Didn't I just say not to create dependencies in the constructor?! Yes. I did, but that's really just to make things easy to test. By having an overload that does allow dependency injection, we have the ability to easily test in isolation. Nothing says we can't also provide a convenience ctor that creates the dependencies for production code. We've just made things more flexible is all.

---

new Microsoft.Xna.Framework.Rectangle((int)Position.X,

A using directive would go a long way toward reducing verbosity here.

using Microsoft.Xna.Framework;

//....

new Rectangle((int)Position.X, ...