Change PassThroughInputManager to not sync new presses outside Handle()

[frenzibyte]

• Discussed in Resuming gameplay by clicking orange cursor can cause a hit on the underlying circle osu#9658 (comment), resolving one part of the issue.
• Closes PassThroughInputManager syncs key presses even if they were handled #1966

When the game is paused, the game input manager has UseParentInput set to false for input to be frozen. When resuming gameplay in osu!, we ask the user to press a key at the location of the cursor first, then toggle back UseParentInput and resume gameplay accordingly.

If the user had paused without holding down any key, then when toggling back UseParentInput, either at the setter of UseParentInput, or at any subsequent update frames of the input manager, the key that was pressed to resume gameplay will be synchronised with the game input manager, causing gameplay to receive an incorrect press event and pollute the score for the player.

This PR introduces a flag, SyncNewPresses, which can be disabled to prevent the above from happening, by making Sync() not apply press input from parent state, and only look at releasing instead.

[bdach]

I dunno. I threw the flag proposal out as a potential way of avoiding complications. But I look at this diff and see more complications.

I'd like this to be simpler. Isn't it an option to (spitballing) make Sync() virtual and override it to do nothing in the resume overlay case or something?

At the least the flag needs to be named different. Something like TriggerInputEventsOnSync or something. Ideally it should mean that, too.

[frenzibyte]

I'd like this to be simpler. Isn't it an option to (spitballing) make Sync() virtual and override it to do nothing in the resume overlay case or something?

I think you're misunderstanding the issue, the input manager of the resume overlay has nothing to do with this. Gameplay is the primary and full user of PassThroughInputManager and the UseParentInput flag, I disagree with adding further customisation to the class instead of changing its primary behaviour to, what I would say, better match expectations.

To reiterate, the point of this change is to make the input manager that contains the hit objects not sync new presses using Sync(), so it doesn't catch the press by the resume overlay if we intentionally block it from receiving it by the PassThroughInputManager.Handle(UIEvent) path. In other words, we want the following to happen:

• User presses osu! action to resume.
• ResumeOverlay receives the action press, but, using some method that I haven't decided on yet, it will not let the DrawableRuleset.KeyBindingInputManager (hit objects input manager) receive the action press via the Handle(UIEvent) path
• Then, since I want DrawableRuleset.KeyBindingInputManager to have "sync new presses" disabled, once it's enabled to receive parent input (set UseParentInput to true), it will not apply the user's press of the osu! action.

I suggest we do away with the flag and conform to the new behaviour introduced by this PR, and reassess when something bad happens.

[smoogipoo]

I don't like the increase in complexity here. I'd be tentatively okay with using the same logic as the mouse buttons, but I also have long forgotten why mouse buttons specifically differed.

[frenzibyte]

The mouse button behaviour looks to have just been added as-is since #1682 with no specific reason.

Re-reading the description, perhaps it's to block mouse buttons from appearing pressed if they were intercepted from reaching the PassThroughInputManager event handler method.

[bdach]

I suggest we do away with the flag and conform to the new behaviour introduced by this PR, and reassess when something bad happens.

I'm not saying yes or no to this until you tell me if #1672 is going to regress, and if yes, what local patch to fix it you're proposing here.

[frenzibyte]

I touched on that already in ppy/osu#9658 (comment) (see final point in bullet 2, where I linked the issue). But that being said, the diff will not change by much actually:

diff --git a/osu.Framework/Input/PassThroughInputManager.cs b/osu.Framework/Input/PassThroughInputManager.cs
index 40855fa26..e82e66ab6 100644
--- a/osu.Framework/Input/PassThroughInputManager.cs
+++ b/osu.Framework/Input/PassThroughInputManager.cs
@@ -48,15 +48,6 @@ public virtual bool UseParentInput
 
         private bool useParentInput = true;
 
-        /// <summary>
-        /// Whether to synchronise newly pressed buttons on <see cref="Sync"/>.
-        /// Pressed buttons are still synchronised at the point of loading the input manager.
-        /// </summary>
-        /// <remarks>
-        /// This does not apply for mouse buttons.
-        /// </remarks>
-        public bool SyncNewPresses { get; init; } = true;
-
         public override bool HandleHoverEvents => UseParentInput ? parentInputManager.HandleHoverEvents : base.HandleHoverEvents;
 
         internal override bool BuildNonPositionalInputQueue(List<Drawable> queue, bool allowBlocking = true)
@@ -251,7 +242,7 @@ protected virtual void SyncInputState(InputState state, bool applyPresses)
                 }
             }
 
-            if (SyncNewPresses || applyPresses)
+            if (applyPresses)
             {
                 // since we have a flag for syncing new presses, it is expected that we apply mouse button presses as well.
                 // this wasn't the case before the existence of the flag for another reason that's irrelevant here,

The applyPresses property will have to exist so that the input manager can apply new presses on LoadComplete to satisfy #1672.

[bdach]

Well I dunno what to say at this point if that is the extent of the changes incurred by the flag. You're basically presenting two choices: a schedule hack or increased complexity and the public flag has basically no relevance. At this point I'd be even willing to reconsider the schedule hack because at least it's self-contained to a degree I guess?

I'd even consider something like turning off sync entirely on the ruleset input manager after initial load or something over this if it doesn't incur further issues.

[frenzibyte]

Two things need to be clarified:

1. The proposal of scheduling resume operation to be performed one frame later does not replace this PR, because as soon as UseParentInput is set to true and the ruleset input manager sees the pressed key it will still register it as pressed. The intention of this PR is to make the input manager completely disregard the pressed key regardless of how long it is held.
2. Disabling synchronisation on ruleset input manager after initial load does not help either, because we still want to synchronise buttons that were previously held but are now released (i.e. a user holding a slider but paused midway then resumed but didn't continue holding).

I've juggled and reordered the class around in hopes it reduces complexity. Take another look at it and see what you think. I have updated test coverage to reflect the new expected behaviour of the class, and tested on osu! to ensure it's working correctly.

[frenzibyte]

I have removed the initial state synchronisation logic. Removing it does not regress the case brought in #1672 because scroll speed actions have been migrated to global key bindings, therefore not reading from the ruleset input manager to trigger the action. Also, if we compare this behavior on a normal Drawable, a drawable does not receive an OnKeyDown if it's added to the hierarchy after a key has been pressed, and when the key is up, it does not receive an OnKeyUp either (see test).

Therefore, for the sake of reducing complexity and matching general behaviour of drawable input handling, I have removed it, and also confirmed all tests both on o!f and osu! are passing, all while the PR still maintains its original intent, which is to resolve ppy/osu#9658 (only partly).