-
Notifications
You must be signed in to change notification settings - Fork 451
Revert pen input flow and map pen to touch on mobile platforms #6523
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
Conversation
|
Is this a direct revert? Are you intending to make a PR to re-apply the changes for future consideration (so they aren't lost to the void)? |
|
I can make a PR but the best state it could be in is a draft and eventually become outdated structure. The pen input implementation has shown to be not working very well with touch input, and I believe a major refactor in how it works is required rather than reviving it back as is. |
|
@Susko3 are you on board with this direction? |
|
Just to keep this part clear, reverting is mandatory as |
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 also a partial fix, as a hovering pen + touch for buttons play style should not work. For that to work, changing OsuTouchInputMapper is required, and at that point, we might as well go for ppy/osu#31704.
If this was a hack that gets everything in working order in an ugly way, then I would let it pass. But this is a hack that makes things half-working on half the platforms.
| if (penDown && device_type == TabletPenDeviceType.Direct) | ||
| enqueueInput(new TouchInput(new Input.Touch(TouchSource.PenTouch, position), true)); | ||
| else | ||
| enqueueInput(new MousePositionAbsoluteInput { Position = position }); |
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 don't like this. Mapping pen to touch only on mobile will fix the issue only on mobile.
Having pen input sometimes be touch and sometimes be mouse is hard to reason about, debug and develop for.
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 don't like this. Mapping pen to touch only on mobile will fix the issue only on mobile.
That is the intention, fixing a regression on mobile. Desktop platforms are off topic.
Having pen input sometimes be touch and sometimes be mouse is hard to reason about, debug and develop for.
This is how the game has always been working on iOS. This change is not introducing new concepts, it is bringing back how things have been working before.
This is not a partial fix, this is a complete fix for a regression that was caused by SDL. Hovering play-style has never been working before, it is never a reason to block fixes like this as it is irrelevant to the entire regression in the first place. |
|
Sure, I can get behind this. But I don't see the need to pull out the pen handling changes. If the API is flaky, just make |
|
If it's not serving any purpose I'm not sure why it should exist, but sure, I can bring it back. |
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'm fine with mapping pen to touch in the interim, fixes a regression as you mentioned.
My end goal would be proper pen input (ppy/osu#31892 (comment)).
| private bool penDown; | ||
| private Vector2 currentPosition; |
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.
Doesn't need to be stored here, makes the logic harder to follow.
- SDL_PenMotionEvent has SDL_PenInputFlags for
penDown - SDL_PenTouchEvent has
float x, y
Maybe cleaner to make both PenMove and PenTouch report both Vector2 position and bool pressed.
| /// <summary> | ||
| /// A touch source that represents a pen/stylus. | ||
| /// </summary> | ||
| PenTouch, |
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 eleventh finger on a touchscreen would report as a PenTouch. assignNextAvailableTouchSource() / activeTouches (both SDL2 and SDL3) needs to be updated to only consider the first ten.
I'd say add internal const int MAX_NATIVE_TOUCH_COUNT = 10 in TouchState.
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.
Code looks good.
Hover play-style, from what I've seen has worked in android devices (see this video i sent on discord, there's 2 cursors on my galaxy tab, red is hover, and blue is ingame cursor), though you've said previously (and a lot of times) that it never worked for iPadOS. I'm just a player, so I probably had no say on this, but reverting back to touch has some issues:
If this PR ends up getting merged, I will certainly hope there's gonna be actual pen support in the future for mobile devices, instead of depending on |
|
Hover will still work with this PR. You'll only get TD applied if you touch the pen on the screen. |
I thought this was a direct revert? Before SDL3 updated to support pen input on mobile, hover was never working, however, post SDL3 support (which is what caused all this mess to begin with) makes it so that hover IS supported.
|
I think there's a misunderstanding here. By hovering playstyle, I'm specifically referring to a pen + touch playstyle where you hover with the pen and tap on the corners. If your concern is on the pen + keyboard playstyle, that has always been working on iPadOS and will continue to be working even with this PR. It seems that it didn't work in Android until the SDL3 bump since previously SDL did not have support for pen input on Android, and the pen was not translated as mouse or otherwise. Regardless, even with this PR, the SDL3 bump will not be reverted and Android pen support will remain working and you can still play with a pen + keyboard playstyle. The concern has always been about pen + touch playstyle. |
Ah, got it. Was thinking for months that "hovering playstyle" was referring to hover playstyle WITH keyboard (along with TOUCH as well). Thanks for the clear up. |
After multiple attempts to try and fix ppy/osu#31570, this is one which brings back osu! to how it has been working before.
Now since direct/onscreen pen input is mapped to touch input, the entire pen input /
ISourcedFromPenAPI becomes flaky and inconsistent, as not all pen input are sent as anISourcedFromPenmouse event. For simplicity purposes, I have chosen to revert it completely until later time.When returning back to resurrect this flow, special attention needs to be given into the following intertwined two problems:
PassThroughInputManager.syncReleasedInputs()should respect mouse input sourced from pen (currently sends release input without such context)PassThroughInputManager.syncReleasedInputs()should be aware whether mouse button pressed from parent comes from mouse and not other input sources (main reason why it's impossible to make this flow work withOsuTouchInputMapper)