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

Enable mouse/pen aim + touch click play style #31704

Closed

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Jan 27, 2025

This PR enables a playstyle where a hovering mouse or pen is used for position, and finger touches are used for clicking (as a keyboard). This is enabled by a new flag (trackPositionOfIndirectTouches), which will prevent touches from "stealing" the cursor position. The flag is dynamic: if a user directly touches a hitcircle with their finger, the game will assume the previous playstyle of aiming with a finger and using fingers for clicking. But if a user then hovers their pen or mouse, the game will instantly switch to the playstyle described in this PR.

The implementation is not perfect (see TestFirstTouchBrokenWhenMousePressed), but it's possible to work around some of the issues (see TestFirstTouchWorksWhenMousePressedAndMouseButtonsDisabled). It's recommended that users who wish to aim with the pen pressing down on the tablet surface, and don't want to use the pen for clicking, disable "mouse clicks during gameplay". (This is currently under the "Mouse" section, as there is no "Pen" section in input settings.)

The changes are scoped to OsuTouchInputMapper, so it only affect touch input (i.e. mouse/keyboard/tablet input will not regress).

Testing

A previous version of this branch was tested by frenzi and Walavouchey.

As seen in the Walavouchey replay, a common user "error" is accidentally directly tapping on a circle. Here, a hitcircle happened to appear exactly under where the user is trying to tap for a click, stealing the position from the pen, resulting in a sliderbreak.

This is not a new issue, as the same thing can happen when using touch instead of a pen for aiming.

A user can fix this by not tapping on the playfield, maybe best accomplished by resizing or moving the playfield using Screen scaling.

The following logic could be updated (in a separate PR) to fix this (perhaps it should consider notelock):

public bool CheckScreenSpaceActionPressJudgeable(Vector2 screenSpacePosition) =>
// This is a very naive but simple approach.
//
// Based on user feedback of more nuanced scenarios (where touch doesn't behave as expected),
// this can be expanded to a more complex implementation, but I'd still want to keep it as simple as we can.
NonPositionalInputQueue.OfType<DrawableHitCircle.HitReceptor>().Any(c => c.CanBeHit() && c.ReceivePositionalInputAt(screenSpacePosition));

osu_2025-01-27_15-36-41

Copy link
Member

@frenzibyte frenzibyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine conceptually. I still would've preferred a rewrite of OsuTouchInputMapper that explicitly recognizes pen input as a "pointer" so as to avoid these extra set of conditionals but there are blockers in the way that require reassessment of how the o!f input subsystem works and I'm pretty much full with all of this stuff already. @peppy requesting second-hand code review on this.

Comment on lines +645 to +647
beginTouch(TouchSource.Touch3);
checkPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0); // strange, an indirect touch, but it's not registered
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably rewrite this to actually test expectations and put an ignore marker on the test itself. Feels wrong to assert wrong behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's the correct behaviour here. Maybe I was too harsh in naming the test "broken". This behaviour is correct in the sense that it closely matches what would happen if you pressed your pen and clicked the keyboard X. With touch input, it's a necessity to release the tracked touch, but pen input supports hover, the user explicitly chose to click with the pen—they should disable pen clicks if they don't want them.

Hmm, maybe tapping with the pen pressed down should press the OsuAction.RightButton without releasing the left. This would emulate clicking with the C key.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this—pressing the pen down will completely prevent touches from triggering the left button:

diff --git a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
index d6eec41584..d2087c9f04 100644
--- a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
+++ b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
@@ -80,12 +80,12 @@ protected override void OnTouchMove(TouchMoveEvent e)
 
         protected override bool OnTouchDown(TouchDownEvent e)
         {
-            OsuAction action = trackedTouches.Any(t => t.Action == OsuAction.LeftButton)
+            OsuAction action = trackedTouches.Any(t => t.Action == OsuAction.LeftButton) || osuInputManager.PressedActions.Contains(OsuAction.LeftButton)
                 ? OsuAction.RightButton
                 : OsuAction.LeftButton;
 
             // Ignore any taps which trigger an action which is already handled. But track them for potential positional input in the future.
-            bool shouldResultInAction = osuInputManager.AllowGameplayInputs && !tapsDisabled.Value && trackedTouches.All(t => t.Action != action);
+            bool shouldResultInAction = osuInputManager.AllowGameplayInputs && !tapsDisabled.Value && trackedTouches.All(t => t.Action != action) && !osuInputManager.PressedActions.Contains(action);
 
             // If we can actually accept as an action, check whether this tap was on a circle's receptor.
             // This case gets special handling to allow for empty-space stream tapping.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That diff further leads to the idea that OsuTouchInputMapper is purposefully not wanting to be aware about the pen's state, and instead substitutes that with OsuInputManager.PressedActions. I don't think this is good, having a set of flags alongside checking PressedActions besides trackedTouches all to support pen input smells like the class is in need for a top-down refactor. I would keep the behaviour broken, update the test to match the expected behaviour achieved by the diff above, and mark it ignored.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That diff further leads to the idea that OsuTouchInputMapper is purposefully not wanting to be aware about the pen's state, and instead substitutes that with OsuInputManager.PressedActions.

OsuTouchInputMapper should never check mouse state to see if a pen is pressed. There is too many variables and settings that affect whether a mouse click (or pen press) will result in an OsuAction. OsuInputManager.PressedActions is the only and best source of whether an OsuAction is pressed by a pen.

@frenzibyte frenzibyte requested a review from peppy January 28, 2025 08:20
@Walavouchey
Copy link
Member

Walavouchey commented Jan 28, 2025

this branch + latest framework master logs replay

  1. pen dragging and touch tapping work just as before
  2. hovering while touch tapping doesn't work (i switch to this ~25s into the map and fail). cursor position doesn't update in this state, even after lifting the pen completely and continuing to tap the screen video
  3. in general, hovering moves the cursor relatively (like a mouse) while dragging moves it absolutely (like a tablet should be) video

edit: latest framework master (with ppy/osu-framework#6511) + this pr works pretty well again

@peppy
Copy link
Member

peppy commented Jan 29, 2025

i can't make much sense of this sorry. it's adding one too many layers to OsuTouchInputManager.

@Susko3 Susko3 changed the title Enable mouse/pen aim + touch click play style Prevent touch input from stealing osu! cursor position of mouse/pen input Feb 14, 2025
@Susko3
Copy link
Member Author

Susko3 commented Feb 14, 2025

I really tried to keep it as simple as possible, but I think I was too ambitious in naming this PR "Enable mouse/pen aim + touch click play style". The new goal for this PR is to prevent touches from stealing mouse position, which is what the code actually does.

@frenzibyte this means that, for now, the broken tests you commented on will remain broken. I will update the tests and put a [Ignore] as you advised.

i can't make much sense of this sorry. it's adding one too many layers to OsuTouchInputManager.

OsuTouchInputMapper assumes it has exclusive control of osu! cursor position and OsuActions. It needs to be made aware that the mouse can change the position externally. Alternatively: do you think moving arbitration of whether touch or mouse has priority over position to a different component would make this PR better?

@Susko3 Susko3 changed the title Prevent touch input from stealing osu! cursor position of mouse/pen input Enable mouse/pen aim + touch click play style Feb 14, 2025
@Susko3
Copy link
Member Author

Susko3 commented Feb 14, 2025

I'm going to make a new PR that moves the mouse/touch position arbitration logic out of OsuTouchInputMapper.

@Susko3 Susko3 closed this Feb 14, 2025
@peppy
Copy link
Member

peppy commented Feb 15, 2025

Alternatively: do you think moving arbitration of whether touch or mouse has priority over position to a different component would make this PR better?

Can you make a very rough PoC (even if it doesn't work / code isn't filled, just abstract methods / flows) to help me understand? At the end of the day, the more layman-friendly we can make these input flows the better

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.

Cannot play osu! with pen + touch input
4 participants