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

Remove broken scroll gesture on Android #62289

Merged
merged 1 commit into from
Jun 26, 2022
Merged

Conversation

madmiraal
Copy link
Contributor

@madmiraal madmiraal commented Jun 21, 2022

Detecting an Android scroll gesture and using it to create a InputEventPanGesture was added as part of #33630, which aimed to address #8151, i.e. it had nothing to do with scrolling. Unfortunately, the implementation of the creation of the InputEventPanGesture was deeply flawed and broke everything that used the InputEventPanGesture on Android; most notably ScrollContainers, but also GraphEdit and possibly others.

This PR removes the flawed implementation of InputEventPanGesture on Android.

Fixes #36312
Fixes #45402
Fixes #53619
Fixes #60600
Fixes #61816

@m4gr3d
Copy link
Contributor

m4gr3d commented Jun 22, 2022

Unfortunately, the implementation of the creation of the InputEventPanGesture was deeply flawed and broke everything that used the InputEventPanGesture on Android; most notably ScrollContainers, but also GraphEdit and possibly others.

Can you provide more context? Do you know what in the implementation caused the breakage?

This PR removes the flawed implementation of InputEventPanGesture on Android.

Are scroll gestures still properly handled after the removal of this logic?

@madmiraal
Copy link
Contributor Author

Can you provide more context? Do you know what in the implementation caused the breakage?

The only data extracted from the GestureDetector.SimpleOnGestureListener.onScroll(MotionEvent e1, MotionEvent e2, float distanceX, float distanceY) call are the distanceX and distanceY values:

public boolean onScroll(MotionEvent e1, MotionEvent e2, float distanceX, float distanceY) {
//Log.i("GodotGesture", "onScroll");
final int x = Math.round(distanceX);
final int y = Math.round(distanceY);
GodotLib.scroll(x, y);
return true;
}

Therefore, only the amount moved since the last MotionEvent is being used.

Furthermore, these movement values are then used as position values, from which a new movement values is calculated:

void AndroidInputHandler::process_scroll(Point2 p_pos) {
Ref<InputEventPanGesture> ev;
ev.instantiate();
_set_key_modifier_state(ev);
ev->set_position(p_pos);
ev->set_delta(p_pos - scroll_prev_pos);
Input::get_singleton()->parse_input_event(ev);
scroll_prev_pos = p_pos;
}

The result is an InputEventPanGesture with a position that is always around (0, 0) (the amount moved since the last MotionEvent) and a nonsense delta that is the difference between the last two amounts moved!

Since the position is always around (0, 0), these nonsense events are only picked up by Control elements that use InputEventPanGestures that are located in the top left corner of the screen (or full screen elements) as seen in all the bugs.

Are scroll gestures still properly handled after the removal of this logic?

Yes, the functional scrolling that uses the emulate_mouse_from_touch is unaffected by this removal.

Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

Thanks for providing additional context and catching that implementation error!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment