-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
base: master
Are you sure you want to change the base?
Conversation
64ed55d
to
91e944d
Compare
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(),
}); |
That will not work, |
I feel like that playstyle is important, Walavouchey tried it when testing. And it doesn't seem too hard to implement. Just null-out 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) |
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 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. |
I've not looked into this at all, but this claim is what puzzles me the most. |
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 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. |
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.