Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic test coverage for pen & touch playstyle #31680

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frenzibyte
Copy link
Member

Allows to test behaviour of OsuTouchInputMapper with a proper pen input flow in the future (whenever / if ever that happens). Currently works by representing pen input as a touch source of its own.

@Susko3
Copy link
Member

Susko3 commented Jan 26, 2025

I expected there to be tests for the hovering playstyle. Hovering the pen for position, and using touch for clicking.

InputManager.MoveMouseTo(getSanePositionForPen());
// or
InputManager.Input(new MousePositionAbsoluteInputFromPen
{
    DeviceType = TabletPenDeviceType.Unknown,
    Position = getSanePositionForPen(),
});

@frenzibyte
Copy link
Member Author

frenzibyte commented Jan 26, 2025

That will not work, OsuTouchInputMapper does not handle mouse/pen input, the first tapped touch for clicking will become the pointer instead.

@Susko3
Copy link
Member

Susko3 commented Jan 26, 2025

I feel like that playstyle is important, Walavouchey tried it when testing.

And it doesn't seem too hard to implement. Just null-out positionTrackingTouch when a real mouse (or pen) moves. (Not tested.)

diff --git a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
index e815d7873e..085c45b84c 100644
--- a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
+++ b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
@@ -31,6 +31,8 @@ public partial class OsuTouchInputMapper : Drawable
 
         private TrackedTouch? positionTrackingTouch;
 
+        private bool updatingMousePositionFromTouch;
+
         private readonly OsuInputManager osuInputManager;
 
         private Bindable<bool> tapsDisabled = null!;
@@ -49,6 +51,16 @@ private void load(OsuConfigManager config)
         // Required to handle touches outside of the playfield when screen scaling is enabled.
         public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => true;
 
+        protected override bool OnMouseMove(MouseMoveEvent e)
+        {
+            if (!updatingMousePositionFromTouch)
+            {
+                positionTrackingTouch = null;
+            }
+
+            return base.OnMouseMove(e);
+        }
+
         protected override void OnTouchMove(TouchMoveEvent e)
         {
             base.OnTouchMove(e);
@@ -135,7 +147,9 @@ private void handleTouchMovement(TouchEvent touchEvent)
             if (!osuInputManager.AllowUserCursorMovement)
                 return;
 
+            updatingMousePositionFromTouch = true;
             new MousePositionAbsoluteInput { Position = touchEvent.ScreenSpaceTouch.Position }.Apply(osuInputManager.CurrentState, osuInputManager);
+            updatingMousePositionFromTouch = false;
         }
 
         protected override void OnTouchUp(TouchUpEvent e)

@frenzibyte
Copy link
Member Author

frenzibyte commented Jan 26, 2025

I've tested it already long before I've gone with that framework PR. It's not easy to implement, it breaks very horribly with PassThroughInputManager due to the whole touch-to-mouse conversion. It's out of scope in this PR and any of my work towards resolving the recent gameplay regressions.

EDIT: I haven't tested your approach specifically, but I'm not sure it works correctly if the user were to move just slightly after a tap. But again, out of scope.

@Susko3
Copy link
Member

Susko3 commented Jan 26, 2025

it breaks very horribly with PassThroughInputManager due to the whole touch-to-mouse conversion

I've not looked into this at all, but this claim is what puzzles me the most. OsuTouchInputMapper is unconditionally handling all touches. There should be no touch-to-mouse conversion happening. Is some higher-level InputManager converting touch to mouse input? If so, I would say that is very broken.

@frenzibyte
Copy link
Member Author

frenzibyte commented Jan 26, 2025

My answer is generally to the idea of implementing hovering playstyle without looking at your diff, which I've attempted on my own in https://github.com/ppy/osu/compare/master...frenzibyte:osu:osu-fix-touch-handling?expand=1 (explaining how it deals with PassThroughInputManager is very tiring)

As for your approach, it may work, but I'm not totally fond with it and I'm not entertaining it here. Feel free to open a PR for it and I'll test it when I get the chance, but it shouldn't be a blocker on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants