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

Fix input_event signal on Android #89914

Closed

Conversation

Alex2782
Copy link
Member

Fixes: #89683

Test: Area2DInputTestAndroid-4.3-dev5.zip

Screen_recording_20240326_173058.mp4

@Alex2782 Alex2782 requested a review from a team as a code owner March 26, 2024 16:34
@AThousandShips AThousandShips added this to the 4.3 milestone Mar 26, 2024
@Sauermann
Copy link
Contributor

Sauermann commented Mar 26, 2024

I believe, that the PR in its current form will introduce for focused Windows the following problem:

Windows could now receive delayed input events from the OS after the mouse has left the window, leading to mouse_in_window = true mouse_in_viewport = true even though the mouse is not inside the Window.

Edit: Corrected: it was viewport and not window

@Alex2782
Copy link
Member Author

Everything seems to be ok on MacOS, or how should I test?

mouse_in_window is always false when the mouse leaves the window. (video below, print_line outputs)

void Window::_update_mouse_over(Vector2 p_pos) {

	print_line("void Window::_update_mouse_over: mouse_in_window:", mouse_in_window, "focused:", focused);

	if (!mouse_in_window) {
		if (is_embedded()) {
			mouse_in_window = true;
			_propagate_window_notification(this, NOTIFICATION_WM_MOUSE_ENTER);
		} else if (!focused) {
			// Prevent update based on delayed InputEvents from DisplayServer.
			// Also check whether the window is not focused.
			// Android and iOS do not send 'WINDOW_EVENT_MOUSE_*' events, 'mouse_in_window' is always false.
			return;
		}
	}
Screen-2024-03-26-184939.mp4

@Sauermann
Copy link
Contributor

I have tested the PR (rebased on 7d151c8) on Linux X11 and can confirm my previously mentioned problematic behavior.

MRP: area2d-oob.zip

Move the mouse from inside the Window to outside the window. Afterwards (not always, but in about 25% of my tests) one of the labels tells "in vp", indicating, that the mouse is over the viewport, contradicting the fact, that the mouse is outside of the window.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

The PR needs to address the above mentioned problem.

@Alex2782 Alex2782 force-pushed the fix_viewport_mouse_over branch from 91daa37 to af42fcc Compare March 26, 2024 23:06
@Alex2782 Alex2782 requested a review from a team as a code owner March 26, 2024 23:06
@Alex2782
Copy link
Member Author

Alex2782 commented Mar 26, 2024

I could not reproduce delayed InputEvents from DisplayServer on MacOs and I do not currently have Linux X11.

The new solution now forces WINDOW_EVENT_MOUSE events on the Android side. Another solution would be to check if it is an Android device, I always try to avoid such solutions.


I think if multi-window-support is introduced at some point, the events will have to be revised again.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

While I'm not familiar with Android loop callbacks, the usage of WINDOW_EVENT_MOUSE_* looks correct: It takes care of setting mouse-over states.
Haven't tested the PR though.

I agree, that revisiting these events will become necessary, if multi window support gets implemented.

For reference: #89920 approaches the problem from a different side, that is more aligned with the initial implementation of this PR.

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.

I think if multi-window-support is introduced at some point, the events will have to be revised again.

Multi-window support is on the roadmap (Godot 4.4), and we are already experimenting with it for the Godot XR Editor, so this PR will need to be revised to account for it.

If I'm reading right, in a multi-window world each window will fire the WINDOW_EVENT_MOUSE_ENTER event causing input focus to randomly switch back and forth between each window; is that correct?

@m4gr3d
Copy link
Contributor

m4gr3d commented Mar 28, 2024

@Sauermann @Alex2782 It seems the regression on Android is caused by mouse_in_viewport defaulting to false from #88992 instead of true; can't we set that value to default to true on Android platforms?

Also, how is that change affecting iOS platforms? I'd imagine they would run into a similar issue. cc @bruvzg

@Sauermann
Copy link
Contributor

@m4gr3d While setting mouse_in_viewport = true for android (and introducing an inconsistent state) would solve this problem, I believe that it would be better to fix the underlying problem.
And that underlying problem is that the order of input events and window events from the DisplayServer can get mixed up. #89920 is my attempt to fix this underlying problem for all platforms.

@Alex2782
Copy link
Member Author

This PR is no longer necessary if return is removed again.

https://github.com/godotengine/godot/pull/89920/files
image

@Alex2782 Alex2782 closed this Mar 28, 2024
@AThousandShips AThousandShips removed this from the 4.3 milestone Mar 28, 2024
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.

Area2D's input_event signal doesn't work on Android in v4.3.dev5.official [89f70e98d] and master [fe01776]
4 participants