Add Interface IMoves Support to Spatial Maps

[Chris3606]

Currently, SpatialMap implementations require you to pass the position of an object in when it is added, and call the Move function whenever it moves. This can be inconvenient, since nearly any object that is going to be placed in a SpatialMap will record its own position anyway.

As such, these implementations could be modified to require a Position field and Moved event of their objects, and do the updating manually. This would require throwing exception if the move is invalid, unless the move event allowed cancellation.

[Chris3606]

@Thraka do you have any thoughts on this with regard to SadConsole compatibility? The fact that SadConsole uses MonoGame Point class is a good example of a potential issue here -- it could become confusing to integrate with systems that have their own position-oriented structure. At the same time, having objects keep up to date in this manner could greatly simplify the hoops you currently have to jump through to integrate with ISpatialMap implementations, eg. currently needing to call the Move function each time something moves.

[Chris3606]

As an extended example of an issue; let's say we have the SomeMapThing class, which is a relevant subsection of SadConsole's Entity:

class SomeMapThing
{
    public Point Position { get; set; } // MonoGame point
   // (Other stuff)
}

Now, if we have an interface that GoRogue requires SpatialMap objects to implement, say:

interface IHasPosition
{
    Coord Position { get; } 
    event EventHandler Moved;
}

We create a version of SomeMapThing that can go into SpatialMap with:

class NewMapThing : SomeMapThing, IHasPosition
{
   // Name is the same as the SomeMapThing.Position property so we have to implement explicitly
    public Coord IHasPosition.Position
    {
        get => base.Position.ToCoord(); // Some extension function that converts Point to Coord
        set
        {
            if (base.Position.ToCoord() != value)
            {
                base.Position = value.ToPoint(); // Coord to Point conversion function
                Moved?.Invoke(this, EventArgs.Empty);
            }
        }
    }
    // Makes sure setting this invokes the move event
    public new Point Position
    {
        get => base.Position;
        set => ((IHasPosition)this).Position = value.ToCoord();
    }
}

While this would allow the implementation of ISpatialMap in a way that did not require using the Move function to keep in sync, this implementation has a few problems, IMO:

1. If someone sets SomeMapThingInstance.Position, with that object held in a reference of type MyMapThing, the Moved event is not invoked.
   • The fix for this would be for MyMapThing.Position to be virtual, and we use override instead of new. However, this results in GoRogue requiring things of other libraries to be able to integrate, which I am not a fan of.
2. This implementation is extremely verbose, requiring uses of what I would qualify as obscure/easy-to-mess-up-with features of C# (explicit interface implementation, new property keyword), which IMO makes it less user-friendly.

Thus, personally I think I'm more a fan of keeping the requirement as-is. Particularly in the case where an integration library is involved, it is a bit of work, but easily possible, to transparently call the Move function whenever a Position is set (it only requires that the position setter ensure that the Move function does not fail), or to do the opposite (set the Position whenever an object moves), which I feel like is a more flexible set of options.

[Thraka]

This is a tough one. Let me think on it.

One other scenario to account for is having the object in two different spatial maps at the same time? Is that possible? I was thinking about this in the sense of having a single spatial map representing all objects on the map right? But then you have a 2nd map representing all monsters or items within 10 spaces of the character. If the player moves, both maps need to update. So using a single map to update the player does't exactly work correctly because if you change player via map1, map2 is now out of sync.

[Chris3606]

This is a good point -- it is certainly possible to have an item in more than one spatial map, generally speaking. In the current system, this would simply be a matter of moving the items in both spatial maps (call two Move functions), or hook the spatial map's ItemMoved events together in such a way that if you move one, the other Move is called as well (ISpatialMap will make sure the position has actually changed before it fires the ItemMoved event, so no infinite loop, as of the fix for issue #84). Another option (and the one I typically go with) is, if the objects in the SpatialMap record their position (eg, have a position field) and a Moved event, you can take whatever Map class you put the SpatialMap in, and as entities are added, put a handler in their Moved event that calls Move in the SpatialMap instances (I should actually have a code example of this soon for the sadconsole integration side-project I mentioned on your discord).

In the example I wrote above, it gets a bit more tricksie to pull off, but works basically the same as the one I described above. I do think that it becomes a bit less obvious what's going on in that case, however.

I myself haven't found as much of a need to do something like you describe above, where you have a SpatialMap for only things around the player -- with options like MultiSpatialMap, and LayeredSpatialMap, I typically find it a unnecessary for my cases. However, I could definitely see a case where someone might want to (the one you described is absolutely legitimate), and it's definitely not something I would want to prevent by the architecture.

[Thraka]

I did happen to design SadConsole with an OnPositionChanged method that is called when the position changes 😄 The whole new property with having two properties named the same is a terrible way to design around the issue. But it's a necessary evil 😄 that I'm glad I foresaw with OnPositionChanged

protected virtual void OnPositionChanged(Point oldLocation) { }

So in the case of SadConsole, you could merge your design with mine and have something pretty simple.

class NewMapThing : Entities.Entity, IHasPosition
{
    private Coord _roguePosition;

    // Name is the same as the SomeMapThing.Position property so we have to implement explicitly
    public Coord IHasPosition.Position
    {
        get => _roguePosition;
        set
        {
            if (_roguePosition != value)
                Position = value.ToPoint(); // Set the real master position
        }
    }

    protected override void OnPositionChanged(Point oldLocation)
    {
        // if points don't match, spatial map needs update (or anything who subscribed to moved)
        if (_roguePosition != Position.ToCoord())
        {
            _roguePosition = Position.ToCoord();
            Moved?.Invoke(this, EventArgs.Empty);
        }
    }
}

The Moved event design does seem nice. It allows the programmer to decide when to notify the move. And with the interface the GoRogue point can be hidden without issue. Also, if you had someone using GoRogue with nothing else, they can just as easily implement IHasPosition.Position implicitly and have that be the pure way for positioning. So Moved lets you integrate better with others like SadConsole.

Either way, when you have the scenario of there being two types of positioning properties, something tricky has to be designed 😄 So I wouldn't let that hold back a good design that makes that tricky part easier, like Moved.

[Chris3606]

This is true. The above solution, provided OnPositionChanged is called properly when the master position changes, works well. That much said, one could take the current implementation and above class (with the moved event), and create something like:

public class Map
{
    SpatialMap<NewMapThing> _mapThings;
    // Or some other way of exposing to the user of Map
    public IReadOnlySpatialMap<NewMapThing> MapThings => _mapThings.AsReadOnly();

    public void AddThing(NewMapThing thing)
    {
        if (_mapThings.Add(thing))
            thing.Moved += OnThingMoved;
    }

    public void RemoveThing(NewMapThing thing)
    {
        if (_mapThings.Remove(thing))
            thing.Moved -= OnThingMoved
    }

    private void OnThingMoved(object s, EventArgs e)
    {
       var mapThing = (NewMapThing)s;
        _mapThings.Move(mapThing, mapThing.Position); // Provided this returns the NEW position 
    }
}

And get the same effect, with I believe about the same amount of work. (?) The small problem with this implementation, and the example you posted, is that if the master position changes in a way that the SpatialMap won't allow (eg. something that causes Move to return false), the spatial map has no method of rejecting that change. The solution is, of course, to make sure this doesn't happen in your implementation of the setter. I must say, though, I do believe this is more obvious when the SpatialMap.Move function exists. I'm not really sure which is the more intuitive solution there.

[Chris3606]

Here's a thought, I wonder if I could manage both? While it is a bit of extra work, I could possibly re-organize the underlying interfaces a little (pull the move functions out, for instance) and implement the IHasPosition version on top of the current one.

[Thraka]

Another thought!

What about NOT doing move in the spacialmap? Instead of IHasPosition which has a position and move event, have a IMoved which has the Move event only? And the event sends the new position. This way it's 100% up to the implementer to handle moving, and you purely react to the move.

[Chris3606]

That's not a bad thought at all. I'll come back to this when I can get to a computer and do some playing around

[Chris3606]

Hm. Coming back to this, I absolutely need the actual position when the item is added. So if I did the above, I'd end up having to take the position at least in the Add function, though this would basically be a replacement for the Move function(s).

Not entirely convinced its worth it in all honesty, however, it's worth a look.

[Thraka]

I'm still trying to figure this one out.. what to do.. what to do..

[Chris3606]

I still haven't been able to come up with a super satisfactory solution to this. I'm thinking if anything, it would just come to implementing a version of the spatial maps that does the linking of move events to the Move function for you, and throws exception if that move happens to somewhere it should't -- the current system of Move functions returning false when they fail is something I aim to change anyway so that they throw exceptions, however that's a breaking change. As such, particularly with the additions of somewhat limited use in the immediate future (as the GoRogue SadConsole integration library shows, it' isn't difficult and is arguably more intuitive to just wrap the Move call into internals of a Map class, I'll probably push this to 3.0/3.x

[Chris3606]

Since the changes to spatial maps in 3.x, this would just amount to creating an interface which defines a position and a Moved event (or perhaps just a moved event), and auto-registering a handler for that event which performs the move syncs:

public void MovedEventArgs : EventArgs
{
    public readonly Point OldPos;
    public readonly Point NewPos;
    
    // boilerplate
}
public interface IMoves
{
    public event EventHandler<MovedEventArgs> Moved;
}

This functionality could be pulled directly from IGameObject's current interface.

A variation of the spatial map classes can then be created which mandates its items be subclasses of this interface, and uses spatial map's existing add/remove events to hook these appropriately.