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

Implement OS agnostic InputEventGesture with Pan, Pinch, and Twist #39055

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

Conversation

jeremyz
Copy link
Contributor

@jeremyz jeremyz commented May 25, 2020

this is shamelessy inspired by Federico-Ciuffardi's job on #36953

implementation of thresholds to limit the amount of
emitted events will come later.

this is related to #13139 (and #37754 for 3.2 branch)

Bugsquad edit: Fixes #13139.

@jeremyz jeremyz force-pushed the input_gesture branch 2 times, most recently from e09c858 to 5a4e1e0 Compare May 26, 2020 04:59
@akien-mga akien-mga added this to the 4.0 milestone May 27, 2020
@jeremyz jeremyz force-pushed the input_gesture branch 2 times, most recently from 56a2c03 to e2a2e75 Compare May 28, 2020 15:02
@jeremyz jeremyz requested a review from a team as a code owner June 15, 2020 09:50
@jeremyz
Copy link
Contributor Author

jeremyz commented Jun 15, 2020

This has been tested on X11 and android on 3.2 branch, see #37754 (test scene included).

@jeremyz jeremyz requested review from a team as code owners November 2, 2021 15:01
@jeremyz jeremyz force-pushed the input_gesture branch 2 times, most recently from f8fae5d to 42589f0 Compare November 2, 2021 15:57
@akien-mga akien-mga changed the title implements EventGesturePan,Pinch,Twist - OS agnostic Implement OS agnostic InputEventGesture with Pan, Pinch, and Twist Nov 10, 2021
core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.cpp Outdated Show resolved Hide resolved
core/input/input_event.h Show resolved Hide resolved
doc/classes/InputEventGesturePan.xml Outdated Show resolved Hide resolved
doc/classes/InputEventGesturePinch.xml Outdated Show resolved Hide resolved
doc/classes/InputEventGesturePinch.xml Outdated Show resolved Hide resolved
doc/classes/InputEventGestureTwist.xml Outdated Show resolved Hide resolved
doc/translations/classes.pot Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Sorry for the delay reviewing this, and thanks for keeping at it! It's looking good and I agree that such an OS agnostic approach is the best to support these events on all platforms that support touch input.

BTW, what does it imply for the current only platform-specific implementation for macOS?

Would be good to have some review/feedback from @godotengine/input @RandomShaper, maybe also potential users of the feature like @HEAVYPOLY?

@jeremyz
Copy link
Contributor Author

jeremyz commented Nov 11, 2021

No worries about the delay, there's a lot of PR, you do what you can.
BTW, good PR review, you've nailed all my mistakes ! they should be fixed now.
About the native Gesture events, (Android/OSX) I'm processing the issue right now, because yes, there is one.
I don't have OSX hardware to test, if someone has some valuable input I'll appreciate it.
I'm on it, will come back with something this week-end.

@madmiraal
Copy link
Contributor

I'm not convinced this is the right way forward.

I agree that we should detect gestures in the Godot input system instead of in each individual platform i.e. this PR is better than #36953. And I agree that gestures should be named after their action not how they’re interpreted i.e. "Pinch" is better than "Magnify". However, I'm not convinced these are the gestures that we should be implementing. My concerns include, but are not limited to:

  • Why does “Pan” (which is an interpretation not an action) require two fingers? Why not just use InputEventScreenDrag as a single finger gesture (since all drags require a touch). It already includes a relative and a velocity parameter?
  • Why include “Twist” (which is also an interpretation not an action). Why not simply include the change in angle between the two fingers as another parameter of “Pinch”?

At the very least, I think there should be an agreed proposal to support which gestures, if any, we should implement.

@madmiraal
Copy link
Contributor

I've created godotengine/godot-proposals#4340 to facilitate the discussion.

@akien-mga akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023
@stevenctl
Copy link

I think you're spot on in your opinion about naming, but at the same time these are supported on mac and work great. I don't see why that should block allowing that on other platforms.

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.

Implement InputEventGesture (incl. Pan and Magnify) for all platforms
4 participants