-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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 empty space tap-streaming support for osu! ruleset on touchscreen devices #22375
Conversation
@@ -116,9 +165,224 @@ public void TestPositionalInputUpdatesOnlyFromMostRecentTouch() | |||
endTouch(TouchSource.Touch2); | |||
checkPosition(TouchSource.Touch2); | |||
|
|||
// note that touch1 was never ended, but becomes active for tracking again. | |||
// note that touch1 was never ended, but is no longer valid for touch input due to touch 2 occurring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor change to existing behaviour, but I think it's actually better than what we had (stops any kind of positional feedback when more than one finger is on screen and moving at once).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once a TrackedTouch
with DirectTouch = true
is designated to be the positionTrackingTouch
(hereinafter the 'pointer touch'), this empty space tap-streaming input method works well and behaves as expected.
My main concern is that for this to happen, the the pointer touch needs to be pressed directly on a circle (and that press counts as an OsuAction
). This is counter-intuitive for the user, as the expected behaviour is that the pointer touch doesn't activate any OsuAction
. For a user using this input method, it's intuitive to:
- Initiate the pointer touch by first tapping on empty space (inside the playfield)
- Move the pointer touch over a circle
- Touch another finger(s) (usually outside the playfield) to use empty space tap-streaming for
OsuAction
s
The following diff feels good to play, and doesn't seem to break existing multitouch 'only tap on the circles' behaviour. (Most test fail though.)
diff --git a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
index 9842e24d05..8e3af051d8 100644
--- a/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
+++ b/osu.Game.Rulesets.Osu/UI/OsuTouchInputMapper.cs
@@ -2,6 +2,7 @@
// See the LICENCE file in the repository root for full licence text.
using System.Collections.Generic;
+using System.Diagnostics;
using System.Linq;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
@@ -67,7 +68,20 @@ protected override bool OnTouchDown(TouchDownEvent e)
{
// If no direct touch is registered, we always use the new (latest) touch for positional tracking.
if (positionTrackingTouch?.DirectTouch != true)
- positionTrackingTouch = trackedTouch;
+ {
+ if (trackedTouch.DirectTouch || positionTrackingTouch == null)
+ positionTrackingTouch = trackedTouch;
+ else
+ {
+ // this touch is not a direct touch, so we treat it as the beginning of empty space tap-streaming
+ // promote the existing touch into a direct touch, so that it sticks as the 'pointer touch'
+ positionTrackingTouch.DirectTouch = true;
+ Debug.Assert(positionTrackingTouch.Action != null);
+
+ osuInputManager.KeyBindingContainer.TriggerReleased(positionTrackingTouch.Action.Value);
+ positionTrackingTouch.Action = null;
+ }
+ }
else
{
// If not a direct circle touch, consider whether to release the an original direct touch.
@@ -123,7 +137,7 @@ private class TrackedTouch
public OsuAction? Action;
- public readonly bool DirectTouch;
+ public bool DirectTouch;
public TrackedTouch(TouchSource source, OsuAction? action, bool directTouch)
{
Here's a little demo, focus on the black touch indicators on the left of the playfield. Ignore the square right-click indicator, that's just windows bugging out. (The miss at the end is my touchscreen lagging.)
2023-01-24.00-23-36.mp4
@@ -44,6 +44,8 @@ protected override void OnTouchMove(TouchMoveEvent e) | |||
handleTouchMovement(e); | |||
} | |||
|
|||
private TrackedTouch? positionTrackingTouch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should be in line 23, grouped together with trackedTouches
as the two serve a similar purpose.
public bool CheckScreenSpaceActionPressJudgeable(Vector2 screenSpacePosition) => | ||
NonPositionalInputQueue.OfType<DrawableHitCircle.HitReceptor>().Any(c => c.ReceivePositionalInputAt(screenSpacePosition)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spinner
Doesn't correctly account for spinners. If a touch begins spinning a spinner I think this should return true
and mark the TrackedTouch
as a DirectTouch
. This will help positionTrackingTouch
be the expected touch when a spinner is the first object after a break (I would expect the user to release all touches during a break, thereby loosing any stored positionTrackingTouch
and DirectTouch
information).
Slider
Doesn't account for the "in flight" sliderhead (marked with an X
here). Releasing and then re-tapping on a slider should keep the DirectTouch
.
Notelock
Notelocked circles cause the touch to be a DirectTouch
, but I think that might be unwanted, see the example below:
Over the course of gameplay, the user's green streaming/tapping touches have drifted to into the playfield (you could argue this is user error). They are streaming the blue stream not knowing that a red note will appear under their taps. And then they accidentally tap on the red note (which is notelocked) in the middle of the blue stream, changing the positionTrackingTouch
that was previously following the pink 'cursor touch', breaking expectations and making them miss the blue stream.
If a notelocked circle didn't cause a DirectTouch
, then the user could finish the blue stream, realise they are covering/tapping over the red stream, and readjust accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of keeping this implementation simple I'm against making any proposed changes for now. I should have left a note regarding this in the PR I guess.
I'm not against any improvements and the potential improvements we can add are very obvious, but they aren't happening in this PR.
positionTrackingTouch = trackedTouch; | ||
else | ||
{ | ||
// If not a direct circle touch, consider whether to release the an original direct touch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some copy/paste mistake in this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain which part is unclear? This still reads correct to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "an" in "the an original" doesn't make sense to me. Also isn't this comment a bit misleading: the code below the comment isn't "considering" whether to release the pressed action -- if there is an action to release, it'll unconditionally release it.
Also, due to the positionTrackingTouch?.DirectTouch != true
check above, positionTrackingTouch.DirectTouch
will always be true
and should not be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the code below is doing:
"Release the original direct touch action (if any) to allow empty space stream tapping with two contact points."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read over this multiple times and missed the completely incorrect "an" 😓
I'll reword based on your feedback.
I did consider this, but again I'd prefer to change it as a follow-up if/when required. In all the video footage I watched of people playing on touch devices, finger based tracking was mostly only used for streams as it gets very tiresome to always be dragging. Maybe different when a stylus is involved, but I do want to get more user feedback. I'd generally be against ALL touches resulting in a dragging touch as you have it coded. I'd probably base it on a minimum drag time and/or have a larger area lenience around hit circle receptors which are also considered direct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked at some gameplay videos of people playing on phones, and it looks like this input method (as implemented) will work well for those.
I'll copy some of my unaddressed feedback to a separate issue.
This is the one that mobile users have been waiting for.
I've tried to keep the implementation as simple as possible. The general rules are:
If a touch occurs on a hit circle's receptor
If a touch occurs outside all hit circle receptors
So in short, the caveat here is that the game will give the user one extra input if they tap on a hit circle directly then "stream" using two other fingers. In other words, you can hit stacks of three objects by pressing three fingers in quick succession (one on the circle, two anywhere else) without releasing any, which is not possible with keyboard + mouse.
The only thing I'm not sure about is whether we should always alternate the left/right based on last action to make the key overlay look more balanced, or if it's fine as it is (currently if a user always releases the previous input before tapping again, they will all be
Left
). Maybe for another day. Definitely for another day.Closes #4201.
RPReplay_Final1674464946.mp4