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.

Sign up for GitHub

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

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

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
85054ce
Add failing tests
Susko3 Jan 27, 2025
0871590
Clear out `positionTrackingTouch` when receiving real mouse input
Susko3 Jan 27, 2025
43995a5
Prevent new indirect touches from stealing mouse position
Susko3 Jan 27, 2025
094b71e
Also reset mouse state
Susko3 Jan 27, 2025
c2ab9b8
Fix tests exploding because of PTIM sync killing indirect touch posit…
Susko3 Jan 27, 2025
be88fa7
Invert conditional for readability
peppy Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 108 additions & 3 deletions osu.Game.Rulesets.Osu.Tests/TestSceneOsuTouchInput.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
using osu.Game.Tests.Visual;
using osuTK;
using osuTK.Graphics;
using osuTK.Input;

namespace osu.Game.Rulesets.Osu.Tests
{
Expand All @@ -45,7 +46,7 @@ public partial class TestSceneOsuTouchInput : OsuManualInputManagerTestScene
[SetUpSteps]
public void SetUpSteps()
{
releaseAllTouches();
resetState();

AddStep("Create tests", () =>
{
Expand Down Expand Up @@ -597,6 +598,96 @@ public void TestTouchJudgedCircle()
checkPosition(TouchSource.Touch2);
}

[Test]
public void TestMousePositionPriorityWhenIndirectTouch()
{
moveMouseTo(TouchSource.Touch1);
checkPosition(TouchSource.Touch1);

// touch at a different position
beginTouch(TouchSource.Touch2);
endTouch(TouchSource.Touch2);

checkPosition(TouchSource.Touch1);
assertKeyCounter(1, 0);
}

[Test]
public void TestTouchPositionPriorityWhenDirectTouch()
{
addHitCircleAt(TouchSource.Touch2);

moveMouseTo(TouchSource.Touch1);
checkPosition(TouchSource.Touch1);

// touch the hit circle
beginTouch(TouchSource.Touch2);
endTouch(TouchSource.Touch2);

checkPosition(TouchSource.Touch2);
assertKeyCounter(1, 0);
}

[Test]
public void TestFirstTouchBrokenWhenMousePressed() // mouse pressed is how pens are reported when contacting the tablet screen
{
addHitCircleAt(TouchSource.Touch1);
addHitCircleAt(TouchSource.Touch2);

moveMouseTo(TouchSource.Touch1);

pressMouse(MouseButton.Left);
checkPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0);

moveMouseTo(TouchSource.Touch2);

beginTouch(TouchSource.Touch3);
checkPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0); // strange, an indirect touch, but it's not registered
Comment on lines +645 to +647
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.


endTouch(TouchSource.Touch3);
checkNotPressed(OsuAction.LeftButton); // strange, the osu! left button is not pressed, even though the mouse left button is still pressed
assertKeyCounter(1, 0);

// further touches work fine
beginTouch(TouchSource.Touch3);
checkPressed(OsuAction.LeftButton);
assertKeyCounter(2, 0);

endTouch(TouchSource.Touch3);
checkNotPressed(OsuAction.LeftButton);
}

[Test]
public void TestFirstTouchWorksWhenMousePressedAndMouseButtonsDisabled()
{
AddStep("Disable mouse buttons", () => config.SetValue(OsuSetting.MouseDisableButtons, true));

addHitCircleAt(TouchSource.Touch1);
addHitCircleAt(TouchSource.Touch2);

moveMouseTo(TouchSource.Touch1);

pressMouse(MouseButton.Left);
checkNotPressed(OsuAction.LeftButton);
assertKeyCounter(0, 0);

moveMouseTo(TouchSource.Touch2);

beginTouch(TouchSource.Touch3);
checkPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0);

endTouch(TouchSource.Touch3);
checkNotPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0);

releaseMouse(MouseButton.Left);
checkNotPressed(OsuAction.LeftButton);
assertKeyCounter(1, 0);
}

private void addHitCircleAt(TouchSource source)
{
AddStep($"Add circle at {source}", () =>
Expand All @@ -620,6 +711,15 @@ private void beginTouch(TouchSource source, Vector2? screenSpacePosition = null)
private void endTouch(TouchSource source, Vector2? screenSpacePosition = null) =>
AddStep($"Release touch for {source}", () => InputManager.EndTouch(new Touch(source, screenSpacePosition ??= getSanePositionForSource(source))));

private void moveMouseTo(TouchSource source) =>
AddStep($"Mouse mouse to {source} position", () => InputManager.MoveMouseTo(getSanePositionForSource(source)));

private void pressMouse(MouseButton button) =>
AddStep($"press {button} button", () => InputManager.PressButton(button));

private void releaseMouse(MouseButton button) =>
AddStep($"release {button} button", () => InputManager.ReleaseButton(button));

private Vector2 getSanePositionForSource(TouchSource source)
{
return new Vector2(
Expand All @@ -637,14 +737,19 @@ private void assertKeyCounter(int left, int right)
AddAssert($"The right key was pressed {right} times", () => rightKeyCounter.CountPresses.Value, () => Is.EqualTo(right));
}

private void releaseAllTouches()
private void resetState()
{
AddStep("Release all touches", () =>
AddStep("Reset state", () =>
{
config.SetValue(OsuSetting.MouseDisableButtons, false);
config.SetValue(OsuSetting.TouchDisableGameplayTaps, false);
foreach (TouchSource source in InputManager.CurrentState.Touch.ActiveSources)
InputManager.EndTouch(new Touch(source, osuInputManager.ScreenSpaceDrawQuad.Centre));

foreach (var button in InputManager.CurrentState.Mouse.Buttons)
InputManager.ReleaseButton(button);

InputManager.MoveMouseTo(Vector2.Zero);
});
}

Expand Down
53 changes: 41 additions & 12 deletions osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@ public partial class OsuTouchInputMapper : Drawable

private TrackedTouch? positionTrackingTouch;

private bool updatingMousePositionFromTouch;

/// <summary>
/// Whether new indirect touches can start tracking position.
/// </summary>
/// <remarks>
/// This is set to <c>false</c> when the current cursor position is defined by a mouse or pen, preventing new touches overriding the position.
/// </remarks>
private bool trackPositionOfIndirectTouches = true;

private readonly OsuInputManager osuInputManager;

private Bindable<bool> tapsDisabled = null!;
Expand All @@ -49,6 +59,19 @@ 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)
{
// MouseMoveEvents that don't move the mouse are a fluke and not from a real mouse device. They are probably from PassTroughInputManager syncing state (seen in testing).
// Ignore them since the user is not actually using a mouse or pen for aiming.
if (!updatingMousePositionFromTouch && e.ScreenSpaceMousePosition != e.ScreenSpaceLastMousePosition)
{
trackPositionOfIndirectTouches = false; // user is moving their mouse or pen, assume hovering play style -- position from mouse hover, clicking from touches.
positionTrackingTouch = null;
}

return base.OnMouseMove(e);
}

protected override void OnTouchMove(TouchMoveEvent e)
{
base.OnTouchMove(e);
Expand Down Expand Up @@ -92,28 +115,32 @@ private void updatePositionTracking(TrackedTouch newTouch)
if (newTouch.DirectTouch)
{
positionTrackingTouch = newTouch;
trackPositionOfIndirectTouches = true; // a direct circle click, assume switching to touch-only play style
return;
}

// Otherwise, we only want to use the new touch for position tracking if no other touch is tracking position yet..
if (positionTrackingTouch == null)
if (trackPositionOfIndirectTouches)
{
positionTrackingTouch = newTouch;
return;
}

// ..or if the current position tracking touch was not a direct touch (and didn't travel across the screen too far).
if (!positionTrackingTouch.DirectTouch && positionTrackingTouch.DistanceTravelled < distance_before_position_tracking_lock_in)
{
positionTrackingTouch = newTouch;
return;
// Otherwise, we only want to use the new touch for position tracking if no other touch is tracking position yet..
if (positionTrackingTouch == null)
{
positionTrackingTouch = newTouch;
return;
}

// ..or if the current position tracking touch was not a direct touch (and didn't travel across the screen too far).
if (!positionTrackingTouch.DirectTouch && positionTrackingTouch.DistanceTravelled < distance_before_position_tracking_lock_in)
{
positionTrackingTouch = newTouch;
return;
}
}

// In the case the new touch was not used for position tracking, we should also check the previous position tracking touch.
// If it still has its action pressed, that action should be released.
//
// This is done to allow tracking with the initial touch while still having both Left/Right actions available for alternating with two more touches.
if (positionTrackingTouch.Action is OsuAction touchAction)
if (positionTrackingTouch?.Action is OsuAction touchAction)
{
osuInputManager.KeyBindingContainer.TriggerReleased(touchAction);
positionTrackingTouch.Action = null;
Expand All @@ -135,7 +162,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)
Expand Down
Loading