Sync initial input state of PassThroughInputManager from parent

[frenzibyte]

• Resolves Issue with osu!catch not holding onto your dashes/movement keys upon restarting osu#29090

The double-schedule is ugly, but mandatory to ensure the syncing process does not apply until the entire drawable hierarchy contained by the input manager is loaded / ready to handle input via event handlers.

While it's somewhat obvious, note that scheduling twice inside a LoadComplete means to execute in the next update frame rather than the current one, since LoadComplete comes before updating the scheduler in the current update frame (see this)

See: #6398 (comment)

[bdach]

I don't have the mental willpower to face this head on right now but this is like the fifth change touching this class in a super specific manner and when I read "double schedule" I just recoil.

There has to be a better way to do this. I refuse to accept that this is the best that can be done.

[frenzibyte]

Really it's just being adjusted inline with user expectations. I'm pretty sure I said this before, but this class's first-and-foremost usage is gameplay, it's barely used anywhere else.

...and when I read "double schedule" I just recoil.

What do you generally expect when you use schedule basically anywhere? It schedules to next frame, right? This is not the case in LoadComplete, it'll still execute within the same frame. I have to schedule twice.

There has to be a better way to do this. I refuse to accept that this is the best that can be done.

If this refers to touching the class in general, you can't really expect me to define any other place to store the content of syncInputState anywhere other than in here, it would look out of place and disconnected from its partial-counterpart syncReleasedInputs. If this refers to the double-schedule, I can't imagine anything working without it. It can't become a single schedule because of the misfortune of me having to use it in LoadComplete.

Unless you want me to look at the entire issue from a very different perspective where I can make this somehow work without doing this "initial input state sync" logic, at which point I don't know.

[bdach]

I'm pretty sure I said this before, but this class's first-and-foremost usage is gameplay, it's barely used anywhere else.

Shouldn't be in framework then should it? Why are we justifying framework changes by specific game usage scenarios? Bit backwards isn't it?

I dunno. Rather than making the hyperspecific scenarios work the framework component should provide usages with the tools to implement desired behaviours themselves, if possible.

[smoogipoo]

Is there a reason we haven't just made KeyBindingContainer into some sort of InputManager? It sounds like we're adding a bunch of workarounds for it essentially being one in all but name.

Or expose some sort of InputManager.Resync() and call this.GetContainingInputManager().Resync() in KBC?

[frenzibyte]

Signaling a parent input manager to resync input when a key binding container is loaded feels very backwards to me. I'm personally quite anxious from attempting to KeyBindingContainer : InputManager as I'm sure it'll be a refactor too much for me to handle.

Going by what @bdach stated above, I can go for exposing a general Sync method and move the call to RulesetInputManager (still requiring the double-schedule but at a very local level now). Something like:

diff --git a/osu.Framework/Input/PassThroughInputManager.cs b/osu.Framework/Input/PassThroughInputManager.cs
index 3112b4045..e2cbb6f1c 100644
--- a/osu.Framework/Input/PassThroughInputManager.cs
+++ b/osu.Framework/Input/PassThroughInputManager.cs
@@ -42,10 +42,7 @@ public virtual bool UseParentInput
                 useParentInput = value;
 
                 if (UseParentInput)
-                {
-                    syncReleasedInputs();
-                    syncJoystickAxes();
-                }
+                    Sync(SyncBehaviour.ReleasedInput);
             }
         }
 
@@ -56,10 +53,6 @@ protected override void LoadComplete()
             base.LoadComplete();
 
             parentInputManager = GetContainingInputManager();
-
-            // allow a frame for children to be prepared before passing input from parent.
-            // this is especially necessary if our child is a KeyBindingContainer since the key bindings are not prepared until LoadComplete is called on it.
-            Schedule(() => Schedule(syncInitialState));
         }
 
         public override bool HandleHoverEvents => parentInputManager != null && UseParentInput ? parentInputManager.HandleHoverEvents : base.HandleHoverEvents;
@@ -184,16 +177,23 @@ protected override void Update()
 
             // There are scenarios wherein we cannot receive the release events of pressed inputs. For simplicity, sync every frame.
             if (UseParentInput)
-            {
-                syncReleasedInputs();
-                syncJoystickAxes();
-            }
+                Sync(SyncBehaviour.ReleasedInput);
         }
 
         /// <summary>
-        /// Syncs initial state of this input manager to the current state of the parent input manager.
+        /// Syncs the state of this input manager to the current state of the parent input manager, according to the given <paramref name="behaviour"/>.
         /// </summary>
-        private void syncInitialState()
+        protected void Sync(SyncBehaviour behaviour)
+        {
+            if (behaviour == SyncBehaviour.PressedInput)
+                syncPressedInputs();
+            else
+                syncReleasedInputs();
+
+            syncJoystickAxes();
+        }
+
+        private void syncPressedInputs()
         {
             if (parentInputManager == null)
                 return;
@@ -277,5 +277,11 @@ private void syncJoystickAxes()
                 }
             }
         }
+
+        protected enum SyncBehaviour
+        {
+            PressedInput,
+            ReleasedInput,
+        }
     }
 }

diff --git a/osu.Game/Rulesets/UI/RulesetInputManager.cs b/osu.Game/Rulesets/UI/RulesetInputManager.cs
index 31c7c34572..adec23b097 100644
--- a/osu.Game/Rulesets/UI/RulesetInputManager.cs
+++ b/osu.Game/Rulesets/UI/RulesetInputManager.cs
@@ -75,6 +75,15 @@ private void load(OsuConfigManager config)
             tapsDisabled = config.GetBindable<bool>(OsuSetting.TouchDisableGameplayTaps);
         }
 
+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+
+            // allow a frame for children to be prepared before passing input from parent.
+            // this is especially necessary if our child is a KeyBindingContainer since the key bindings are not prepared until LoadComplete is called on it.
+            Schedule(() => Schedule(() => Sync(SyncBehaviour.PressedInput)));
+        }
+
         #region Action mapping (for replays)
 
         public override void HandleInputStateChange(InputStateChangeEvent inputStateChange)

Does that sound more fathomable?

[smoogipoo]

Can you explain how it's better to let user code arbitrarily handle this themselves with two schedules? This looks like a workaround - if another o!f user does the same thing what do we say "yeah just schedule your thing 2 frames into the future because reasons".

[bdach]

Can you explain how it's better to let user code arbitrarily handle this themselves with two schedules?

If that question is directed at me then I agree it's not.

I can't give a counterproposal at this stage because I'd basically have to go revisit every single passthrough input manager change from the past year and rethink the entire thing from scratch. The diffs keep getting more and more cursed but issues still continue to pop up. From a high level view that does not bode well for this code as far as I'm concerned.

[smoogipoo]

Was directed at frenzi's latest diff. At least the PR's code is contained within o!f, but with that diff it's slipped to being handled partially at osu!'s end.

Both are cursed, I absolutely agree with @bdach . For me, the part that really sucks is that the schedules are arbitrary done because children are loaded at some later point in time, so this PR will immediately break as soon as that becomes not the case. Oh, what's that -- a LoadComponentAsync()? Whoops! Shall we schedule another 10 frames in the future to potentially cover that case?

That's why I suggested Resync() that would be called from a child IM (or KBC as it would be in this case) when it's appropriate to do so. It inverts the topology.

[frenzibyte]

It's not really some later point in time, and this is not a 2 frame schedule. If the child is asynchronously loaded then I don't think it's the input manager's concern to try and make sure input is synced after that time, neither do we have actual use cases for that?

Again, this is a single frame schedule, but because it's done in LoadComplete, the schedule has to be applied twice.

If everyone agrees to the idea of triggering a parent input manager resync from a child key binding container, then...sure, I guess.

[peppy]

I don't quite understand the part where you're saying the "second schedule is only required due to being called in LoadComplete" – can you elaborate on that?

[frenzibyte]

Excuse me for this very boring & detailed explanation but it definitely seems like no matter how many times I explained this above, it doesn't seem like I'm being understood.

First of all, I would like to hope that we all share this one idea that "scheduling an action" means to have it execute after a full update cycle on the entire drawable hierarchy present in the game.

In this specific case, taking it step by step, I cannot call syncInitialState immediately without any schedule because none of this drawable's children have been loaded yet (signified by the fact that this is PassThroughInputManager.LoadComplete, which comes before any child of PassThroughInputManager is loaded).

So, I want to schedule the call of syncInitialState once so that it is called in the next frame of this drawable, not this frame of this drawable where LoadComplete is called at. That way, I have ensured that if there is any child to be loaded directly in this frame, then by the next frame it is already in a loaded state.

Now, this does not work. I scheduled syncInitialState once, but it got executed right within this frame. This is all happening because of this:

osu-framework/osu.Framework/Graphics/Drawable.cs

Lines 478 to 492 in b4ed721

	if (loadState == LoadState.Ready)
	loadComplete();
	Debug.Assert(loadState == LoadState.Loaded);
	UpdateTransforms();
	if (!IsPresent)
	return true;
	if (scheduler != null)
	{
	int amountScheduledTasks = scheduler.Update();
	FrameStatistics.Add(StatisticsCounterType.ScheduleInvk, amountScheduledTasks);
	}

As it can be seen, loadComplete is called before Scheduler.Update(), therefore whatever I schedule in LoadComplete will be executed within the same frame, which is against expectations (I would hope).

Therefore, I have to schedule twice inside LoadComplete to get the desired effect of having my action actually execute in the next frame, to actually behave like a schedule. QED.

---

Now after writing all of this, I realise it should be perfectly valid to use ScheduleAfterChildren, but there are some caveats osu!-side that I will have to handle on a case-by-case basis, namely:

1. Fix OsuCursorContainer no longer expanding cursor when holding button before player is loaded. This is happening because OsuCursorContainer is in a hidden state due to GlobalCursorDisplay not making it visible yet. The fix would be to make the cursor always accept input regardless of its visibility state.
2. Fix KeyCounterDisplay not registering input when holding them before player is loaded. This is happening because if I schedule once after children, then the scheduled action will still be executed before KeyCounterDisplay is loaded, because it resides outside of this input manager's hierarchy. The fix is a bit of a UI one.

I will just proceed with that approach instead and do away with this confusion.

[frenzibyte]

Changed to ScheduleAfterChildren in 00fc4ff.

[bdach]

So you've changed a double schedule to a single slightly different schedule but also piled up two game-side changes on top to fix various breakage...?

My head hurts. Looks like I will have to go and revisit this entire passthrough input manager insanity from ground zero because this is insanity.

[smoogipoo]

May I suggest not looking into it too much and leaving this issue for a rainy day? I'm already in the process of rewriting focus management and discovered many related issues, such as input being redirected to multiple places (e.g. multiple textboxes at once) when PITM is involved. Safe to say PITM is entirely busted in concept and in practice.

The osu!-side issue is not p:0 so this doesn't need to be resolved immediately.

The only solution I'd be content with for now is a sync call at the appropriate time from the child (KBC) to the parent (IM), as I mentioned in #6398 (comment), if that's enough to resolve every issue and doesn't require similar osu!-side changes.

[bdach]

I would love to not have to look at this, so I'll take that suggestion any day of the week.

[frenzibyte]

@bdach I can't really process replies like these without actually looking into my statements above your comment and understanding why it is required and responding based on that. All I'm getting here is that whatever I provide is considered "insanity" in your vision, this is not helpful sorry.

@smoogipoo Your suggestion will still require both of my osu!-side changes without thinking much about it. Matter of fact, even by merging PassThroughInputManager and KeyBindingContainer to a single component, I can confidently say my osu!-side changes will still be required, unless the syncing is covered once again by double scheduling or is done at a completely different point than LoadComplete.

While this issue is not p:0, it is p:1 and has been for a long time now, but I'm fine taking my hands off as you intend to apply major input flow refactors in the mean time.

[bdach]

If the changes are unrelated to whatever this PR is turning out to be, then why are they linked back? They should be standalone, and this PR should depend on them if anything.

My point is very simple: Every time I see this class being touched I see more complexity coming. Whether it be a double-schedule-that-is-kinda-just-a-single-schedule, a single schedule-after-children, or whatever else the previous changes did, this single class becomes more and more complex, and more game-side workarounds are piled on around it. Complexity is bad. I don't want any more. I've argued for simpler since #6221 and this is the fifth PR touching this class to fix various breakage. So at this point I think continuing to pile on fixes is essentially sunk cost fallacy and this needs to be reexamined from a higher level.

[frenzibyte]

Solution here deemed to be more of a hacky workaround than an actual solution to the linked issue thread. Closing as such.