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

[3.x] GUI: Support for multi touch in _gui_input #34383

Closed
wants to merge 1 commit into from

Conversation

NoFr1ends
Copy link
Contributor

Fixes #29525


Sends the released and drag touch events to the expected control.
Drag events now goes to the underlying control or if a touch press before happened to that control.
Released events now goes to the control which received the pressed event before.

Before the released event only reached _gui_input if it was from the first finger.
And the drag events always reached the control with the first touch.

I also attached the small test project with which I tested the pressed & released events.
TestMultiTouch.zip

@maluramichael
Copy link

@akien-mga can we get this fix in 3.2 instead of 4.0?

@Calinou
Copy link
Member

Calinou commented Dec 17, 2019

@maluramichael He said on IRC this is too low-level to be a "safe" bug fix for 3.2, considering how critical Viewport is in Godot. Maybe it could be accepted in a 3.2.x point release, but I make no guarantees about this.

@aaronfranke
Copy link
Member

@NoFr1ends Is this still desired? If so, it needs to be rebased on the latest master branch.

@NoFr1ends
Copy link
Contributor Author

@aaronfranke I would really like to rebase this to the current master branch but as currently nether the iOS nor the Android build works on the master branch I'm unable to test these changes.

@ricochet1k
Copy link

I think this is great, but there seems to be a design oversight in focus locking touch events. For example, lets say I touch a button and then change my mind. Most of the time I can move my finger off of the button and then when I let go, the button will not be triggered, also mentioned in #22583. If we want multi-touch focus locking then this PR seems to do it, but probably not without distinguishing between released inside or outside the control. And maybe also enter/exit events for better visual feedback.

Also, I think it is important to get something like this merged in soon, there are quite a number of bugs this would fix: #16761, #22583, #29525, #31822, #41585 and maybe more.

@NoFr1ends If you want to test it on master, I think you could test this at least partially on desktop with Emulate Touch From Mouse turned on.

@NoFr1ends
Copy link
Contributor Author

@ricochet1k That is a good point I have to test and see what we could do about it.

Testing with Emulate Touch From Mouse didn't worked for me well because I was unable to emulate multi touch with it. Maybe I miss something...

@ricochet1k
Copy link

Yeah, multi-touch won't be testable on master, but my vote is that if it works on 3.2 then chances are good that it will at least be easy to fix when someone gets around to making master work on mobile devices.

Copy link
Contributor

@madmiraal madmiraal left a comment

Choose a reason for hiding this comment

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

This fixes the issue of sending the correct InputEventScreenTouch release to a Control i.e. #29525. However, it's worth pointing out that this does not fix #24589. Only InputEventMouseButton events (which are generated from InputEventScreenTouch events when the "Emulate Mouse From Touch" setting is on -- the default) will trigger BaseButton button_down and button_up events in the button; so with "Emulate Mouse From Touch" on, only the first touch will trigger a BaseButton pressed event, and nothing when it's off.

This PR was created before the Vulkan branch was merged, and it remains trivial to rebase it against 3.x. Therefore, I suggest that this PR is changed to be made against 3.x, and a new forward port of this change is created against master.

Otherwise, aside from the minor suggested changes below, I think this is good to merge.

scene/main/viewport.h Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
@NoFr1ends
Copy link
Contributor Author

NoFr1ends commented May 28, 2021

I'll rebase the PR to 3.x and apply your suggested changes later this day. Thanks for your feedback.

@NoFr1ends NoFr1ends requested review from a team as code owners May 28, 2021 16:47
@aaronfranke aaronfranke removed request for a team May 28, 2021 17:48
@madmiraal madmiraal changed the title GUI: Support for multi touch in _gui_input [3.x] GUI: Support for multi touch in _gui_input May 29, 2021
@madmiraal madmiraal modified the milestones: 4.0, 3.4 May 29, 2021
@madmiraal
Copy link
Contributor

and a new forward port of this change is created against master.

@NoFr1ends, will you be creating a forward port of this PR as a new PR against the current master?

@ATHellboy
Copy link

When this fix would be merged ? It will be part of 3.4 ?

@Calinou
Copy link
Member

Calinou commented Aug 5, 2021

When this fix would be merged ? It will be part of 3.4 ?

It's the same issue as #34383 (comment). This pull request didn't get much testing yet, yet it covers a feature that can break projects on mobile if it regresses. If you want to help get it merged faster, test it locally on one of your projects by compiling this pull request's branch from source and report back 🙂

See also Testing pull requests in the documentation.

@YuriSizov YuriSizov requested review from a team August 24, 2021 22:32
@Chaosus Chaosus modified the milestones: 3.4, 3.5 Nov 8, 2021
@gudinoff
Copy link

gudinoff commented Dec 3, 2021

I've tested this PR on an Android device and the events InputEventScreenTouch for the touch press/release were showing OK and with the correct index.
I tested with Emulate Mouse From Touch enabled and disabled and in both tests the _gui_input function received the correct events.

@Atem1995
Copy link

@akien-mga can we get this fix in 3.2 instead of 4.0?

@maluramichael Have you tested it on version 3.4.x?

@Sauermann
Copy link
Contributor

I believe, that this was superseeded by #68630

@akien-mga akien-mga closed this Dec 4, 2022
@YuriSizov YuriSizov removed this from the 3.x milestone Dec 2, 2023
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.

Control _gui_input inconsistent behavior with InputEventScreenTouch