Allow overriding ProcessMouse and ProcessKeyboard on RogueLikeMap

[AnotherEpigone]

As discussed on the SadConsole discord, those methods should probably be virtual like they are in SadConsole.ScreenObject.

Would also need to make sure the mouse and keyboard events don't get eaten by the backing renderer.

[Chris3606]

those methods should probably be virtual like they are in SadConsole.ScreenObject.

Completed as part of the fix for #47 (see #50).

Would also need to make sure the mouse and keyboard events don't get eaten by the backing renderer.

I'm unclear on this point. The desired default behavior is for the BackingObject to do all mouse/keyboard processing internally, because the IScreenObject methods as implemented on the map are intended to forward to the backing object. In general, this should be transparent and a user should not have to worry about the existence of the backing object at all, short of a few references to it when debugging.

Are you referring to the issue you encountered where the mouse state needed to be transformed by creating it based on the BackingObject in order for the mouse events to be processed correctly? This is #47 and is fixed by #50, in a way that removes the requirement for you as a user to worry about transforming the state. After the linked PR, the following should work as expected:

protected virtual bool ProcessMouse(MouseScreenObjectState state)
{
    // Custom mouse logic

    // Optionally:
    base.ProcessMouse(state);
}

Does this solve the second part of your issue?

[AnotherEpigone]

Yes, if that example does work as intended it solves my problem.

The second part referred to the issue I encountered after making ProcessMouse virtual locally. ProcessMouse was always called on BackingObject, but not on the class inheriting from RogueLikeMap. I'm not sure the exact cause of that since I didn't have the SadConsole code to step through.

My override only got called after I set UseMouse = false on BackingObject, and changed the UseMouse implementation on RLM to not reference BackingObject (so it could be true for my map, and false for BackingObject).

As long as it's possible to inherit RLM, override ProcessMouse and have it actually get called, my problem is solved.

[AnotherEpigone]

I checked out your fix-keyboard-mouse branch and confirmed the above is not fixed.

[Chris3606]

Never seen this behavior in the past; let me do some testing and get back to you.

[Chris3606]

Hm, I'm not able to replicate this on the fix-keyboard-mouse branch.

I created an example map subclass which does not interact at all with BackingObject directly:

public class TestMap : RogueLikeMap
    {
        public TestMap(int width, int height, int numberOfEntityLayers, Distance distanceMeasurement,
            uint layersBlockingWalkability = uint.MaxValue,
            uint layersBlockingTransparency = uint.MaxValue,
            uint entityLayersSupportingMultipleItems = uint.MaxValue,
            FOV? customPlayerFOV = null, AStar? customPather = null,
            IComponentCollection? customComponentContainer = null,
            Point? viewSize = null,
            IFont? font = null,
            Point? fontSize = null)
            : base(width, height, numberOfEntityLayers, distanceMeasurement, layersBlockingWalkability,
                layersBlockingTransparency, entityLayersSupportingMultipleItems, customPlayerFOV, customPather,
                customComponentContainer, viewSize, font, fontSize)
        {

        }

        protected override bool ProcessMouse(MouseScreenObjectState state)
        {
            System.Diagnostics.Debug.WriteLine("ProcessMouse called!");

            return base.ProcessMouse(state);
        }
    }

Then simply used that as the ExampleGame map. The output is below; as you can see the Debug.WriteLine is happily spamming my console:

I get the same result with UseMouse explicitly set to true, as well as when I avoid calling base.ProcessMouse entirely; the only time it isn't spamming is if i set TestMap.UseMouse to false.

Perhaps there's something else going on here; do you have a reproducing code example you're able to share?

[Chris3606]

I pushed the test code I used here, for (mostly my own) reference :D

[Chris3606]

Upon further discussion, we were able to determine that the issue happens when a Map is a child of some other screen object.

Upon further investigation, I believe the issue is here: https://github.com/Thraka/SadConsole/blob/master/SadConsole/ScreenObject.cs#L105

This is called by SetObjParent in ScreenObjectCollection when items are added/removed as children. Unfortunately, it's referencing "this" when it adds to/removes from the collection; and when map forwards to the backing object, "this" doesn't reference the proper item; it references the backing field. So what then happens, is BOTH the map AND its backing field are in the children list, via the following call chain:

someScreenObject.Children.Add(RogueLikeMap);
--> Calls someScreenObject.Children.SetParentObj(obj) where obj is RogueLikeMap
    --> Calls RogueLikeMap.Parent.setter
        --> Calls RogueLikeMap.BackingObject.Parent.setter
            --> Calls _parentObject?.Children.Add(this); after _parentObject has been updated to reference someScreenObject
                --> Repeats the entire call chain, but with obj == RogueLikeMap.BackingObject

Normally this loop isn't a problem because when it reaches the part about calling Parent->set the second time, _parentObject == value which causes an instant return. But in this case it's resulted in both objects being in Children, which is not supposed to happen.

In this case, I'm guessing children are processed for mouse control in the reverse order in which were added; so the BackingObject.ProcessMouse gets called first, returns true, and thus you have no opportunity to override the behavior since the return value of true , preventing it from cascading to other children and/or screen objects.

In general, if the map and the BackingObject are both being added to Children, the core of this issue where the function is called twice will apply not only to ProcessMouse, but also to any other function wherein ScreenObject's implementation calls the implementation of its Children; which includes Update. This would become even more of a problem in this particular case, since ScreenSurface's Update implementation calls the Update function of not only its children, but also its components; so all the components have update called twice as well.

Assuming the above interpretation is correct, I'm unsure how to viably fix this so long as the current BackingObject architecture for maps is in place and SadConsole's method of maintaining the parent field remains the same.

ScreenSurface.Parent is not virtual, so there is no opportunity to override the backing surface's implementation. Further, Parent in ScreenObject is used in functions such as UpdateAbsolutePosition, so the backing object's Parent field must be set correctly for those implementations to work. Therefore, simply ignoring BackingObject.Parent is not viable.

Attempting to purposely turn off RLM.UseMouse while maintaining BackingObject.UseMouse may not be a general solution because the backing object implementation of ProcessMouse again depends on its version of that value being correct to function. Further, similar fixes would not solve issues with Update and other functions.

I could try to purposely remove the backing object from the components list each time it is added; but that will not prevent extraneous added/removed events from firing. Unsure what effects that could cause.

Although it is obviously possible, I'd prefer not to implement specific surfaces/objects for RogueLikeMap and RogueLikeMapAdvanced, as it would rather complicate the implementations and user-experience; though admittedly I'm unsure of alternative solutions currently.