Mouse needs to have its state updated before processing on the map

Per further discussion with Thraka, I wanted to provide some elaboration on why this or some equivalent is needed, and outline a few options for fixing it:

Issue Walkthrough

The mouse-related logic starts here.

What happens is the current screen object hierarchy is flattened and reversed. This ensures that the top-most object in the rendering pipeline is evaluated first for mouse input.

So the processing loops through that flattened/reversed list. For each item it does:

Get the object
Build a MouseScreenObjectState based on the mouse and the current object
Call the object.ProcessMouse method and provide the mouse state object that was created.
If ProcessMouse returns true to indicate it handled the mouse, stop processing the mouse.

The data being put into the MouseScreenObjectState object by its constructor all has to do with the concept of a positioned, sized, object. Really IScreenObject is neither. It provides a position, but in its own world, it's meaningless. When a the IScreenObject becomes an IScreenSurface, position now has meaning based on the font of the surface. Size is also provided now too, based on the width/height of the surface. So, the MouseScreenObjectState constructor creates much of its data only if the screen object passed to it is an IScreenSurface.

This is a bit problematic for the current implementation of RogueLikeMapBase. The current implementation of RogueLikeMapBase has a protected BackingObject field which is an IScreenObject of some sort. The map class implements IScreenObject by forwarding the functions, properties, and events required by that interface to their corresponding implementation in its BackingObject. This allows the map to be added directly, like any other screen object, to the SadConsole heirarchy, but allows the subclass to control what type of screen object is actually used.

This becomes an issue for RogueLikeMap because, in that case, it sets BackingObject to an IScreenSurface. So the map is not a screen surface, but its backing object is, and the intent is for the map to be treated like a surface by SadConsole code. When the mouse processing loop executes, SadConsole sees the map, and creates a mouse state based on it; but really, it should have treated the map like a surface because its backing object is a surface.

The code in the original description performs a transformation on the mouse state that bases the state off the backing object, and represents one possible solution for solving this problem.

Potential Solutions

There are a few discussed solutions:

Solution as in Description

The solution presented in the description simply modifies the ProcessMouse implementation to perform the transformation on the state, then call the backing object's ProcessMouse. This solution is fairly simple, however provides a notable disadvantage that anybody overriding the ProcessMouse function has to be aware of the transformation and either make sure to call the base implementation, or do the transformation themselves. This is less than ideal because unless you understand the implementation details of RogueLikeMapBase and the BackingObject (which, generally, a user should not have to), it isn't clear that such transformation is necessary.

Perform the Transformation in a Non-Virtual Function

We could also perform the transformation in a non-virtual ProcessMouse function, and have that non-virtual implementation call something virtual that a user can use to override mouse behavior. This prevents the need for users to understand the implementation details. It should be possible to implement this in a way that is fairly transparent to the user:

        // Explicit implementation of interface
        bool IScreenObject.ProcessMouse(MouseScreenObjectState state)
        {
            state = new MouseScreenObjectState(BackingObject, state.Mouse.Clone());
            return ProcessMouse(state);
        }

        // Function user overrides, which can have same name thanks to explicit implementation of interface function.
        protected virtual bool ProcessMouse(MouseScreenObjectState state) => BackingObject.ProcessMouse(state);

This is the solution PR #50 currently implements, and this or some variation of this is my currently preferred solution.

Implement IScreenSurface on RogueLikeMap

A third option is to simply have RogueLikeMap implement IScreenSurface by forwarding implementations to its BackingObject. This solves the issue without needing special transformation in the mouse functions, but creates two other issues.

First, a user would be able to call the IScreenSurface functions on the map, which would ultimately allow editing of the surface. This is clearly is unintended/undesired. An explicit interface implementation could mitigate this somewhat, but would still allow the issue with proper casting, and make the source of resulting bugs unclear to a user.

Second, if a user ever chooses to implement RogueLikeMapBase themselves for some reason, they would have to be concerned with either performing the transformation of state, or ensuring that their class implements IScreenSurface, and the reasoning for this necessity is, as mentioned previously, non-obvious unless you understand SadConsole and integration library internals.

thesadrogue / TheSadRogue.Integration