Fix subviewports receiving gui input too early #55300

zhehangd · 2021-11-25T05:58:14Z

Issue

Fixes #48401 for 4.0, the wrong order that subviewports handle input events.

Testing project:
viewport_gui_input_fix.zip

Before this fix the order was

Main input (nodes before and including subviewport container)
Subviewport input
Subviewport gui_input
Main input (after subviewport container)
Main gui_input (including subviewport container)
All unhandled_input

One consequence is that no control over a viewport can receive gui_input, as #48401
The root of the problem is that Viewport::push_input handles input and gui_input in succession.
This is alright for the main viewport but not for subviewports.

void Viewport::push_input(const Ref<InputEvent> &p_event, bool p_local_coords) {
  // ...
  if (!is_input_handled()) {
	  get_tree()->_call_input_pause(input_group, SceneTree::CALL_INPUT_TYPE_INPUT, ev, this);
  }
  
  if (!is_input_handled()) {
	  _gui_input_event(ev);
  }
}

Fix

My solution is to split push_input into two functions push_input and push_gui_input (as there has been a push_unhandled_input). They are almost identical to the original push_input except that one only calls _call_input_pause and the other one calls _gui_input_event.

The main viewport enters this function from Window::_window_input

void Window::_window_input(const Ref<InputEvent> &p_ev) {
	// ...
	push_input(p_ev);
	if (!is_input_handled()) {
		push_unhandled_input(p_ev);
	}
}

I added push_gui_input(p_ev); after push_input(p_ev); so the logic remains the same.

Subviewports enter this function from SubViewportContainer::input

void SubViewportContainer::input(const Ref<InputEvent> &p_event) {
	ERR_FAIL_COND(p_event.is_null());

	if (Engine::get_singleton()->is_editor_hint()) {
		return;
	}

	Transform2D xform = get_global_transform();

	if (stretch) {
		Transform2D scale_xf;
		scale_xf.scale(Vector2(shrink, shrink));
		xform *= scale_xf;
	}

	Ref<InputEvent> ev = p_event->xformed_by(xform.affine_inverse());

	for (int i = 0; i < get_child_count(); i++) {
		SubViewport *c = Object::cast_to<SubViewport>(get_child(i));
		if (!c || c->is_input_disabled()) {
			continue;
		}

		c->push_input(ev);
	}
}

I overrode void SubViewportContainer::gui_input which copied the content of SubViewportContainer::input except that

It calls c->push_gui_input(ev) instead at the end.
No p_event->xformed_by(xform.affine_inverse()) as the gui event has been transformed.

After the fix the event order becomes

Main input (before and including viewport container)
Subviewport input
Main input (after)
Main gui_input (before and including viewport container)
Subviewport gui_input
Main gui_input (after)
All unhandled_input

Other concerns

Viewport::push_input has an event_count++ at the end.
I removed it from Viewport::push_gui_input to avoid an event being counted twice.
So only Viewport::push_input counts the events, Viewport::push_gui_input does not (same as Viewport::push_unhandled_input).

I saw TouchScreenButton in scene/2d/touch_screen_button.cpp calls push_input as well.
I have no clue why a button needs to call viewport directly.
I simply added a push_gui_input call after every push_input so the logic should remain the same as before, which means it may still suffer from this bug.
I don't have the environment to test it though.

The doc Using InputEvent is wrong about the viewport order, as it says viewports process events one after another. Hopefully someone will update it before 4.0.

Bugsquad edit:

Fixes Click Events passing through Buttons if there is a Viewport underneath #39666

Fixes godotengine#48401

zhehangd · 2021-11-25T09:14:51Z

Fixed doc generation and not registering the Viewport::push_gui_input method.
Fixed unit test macros SEND_GUI_MOUSE_EVENT and SEND_GUI_DOUBLE_CLICK which used Viewport::push_input.
These should fix the CI errors

zhehangd · 2021-11-26T03:34:13Z

Backported the fix to 3.x in #55339.

doc/classes/Viewport.xml

KoBeWi · 2021-12-25T15:07:11Z

I tested the PR and can confirm that it fixes a bug without introducing obvious regressions. Not sure about the code, needs a review from core team.

reduz · 2022-02-05T11:17:40Z

Looks good, up to @akien-mga to ensure its mergeable.

Sauermann · 2022-02-05T13:35:58Z

Unfortunately this pull request introduces a regression.

Here is a MRP:
SubViewportClick.zip

Load and start the MRP.
Click with the left mouse button on the "Sub B" Label (this sets input-focus to the SubViewportContainer and to the Label B)
Enter a key on the keyboard

Current behavior on master branch is, that the Label B receives the gui-input-event of the key.
With this patch the new behavior is, that Label B does not receive the gui-input-event of the key.

I would expect, that gui-input-events still reach all of the Nodes, as they did before this patch.

zhehangd · 2022-02-07T05:07:17Z

Unfortunately this pull request introduces a regression.

Here is a MRP: SubViewportClick.zip

Load and start the MRP.

Click with the left mouse button on the "Sub B" Label (this sets input-focus to the SubViewportContainer and to the Label B)

Enter a key on the keyboard

Current behavior on master branch is, that the Label B receives the gui-input-event of the key. With this patch the new behavior is, that Label B does not receive the gui-input-event of the key.

I would expect, that gui-input-events still reach all of the Nodes, as they did before this patch.

Thanks for the feedback! I investigated this issue and found it is due to the focus mechanism.
A viewport only knows the focused control that belongs to it, stored in key_focus.
And within a window only one control can be focused under one viewport.

When a control in the subviewport grabs the focus, the main viewport nullifies its key_focus.
So when main viewport recieves a KeyEvent, because it thinks nothing is expecting the event the viewport simply ignores it instead of forwarding it to the subviewport.

So to solve this, it must collect all viewports that are its children and find one with the focus (no more than one) and pass this event to the focused node.
The above commit implements this fix.

However, I also found that this PR does not work properly with subwindows.
Right now it causes an event being fired multiple times.
Working on a patch now.

Sauermann · 2022-02-07T07:33:55Z

Thanks for the feedback! I investigated this issue and found it is due to the focus mechanism.

@zhehangd You are welcome. Currently I also try to solve the same problem with a different approach in #57687. May I ask you for feedback on my implementation concept?

zhehangd · 2022-02-23T03:29:38Z

Finally got a chance to follow up on this PR and get everything fixed.

At the time I started the PR I didn't aware that 4.0 has a dedicated Window class for subwindows, so I accidently copied the window event forwarding function to push_gui_input from push_input. It is deleted now otherwise an event might be forwarded twice.

Originally in this PR push_input is split into two functions: push_input and push_gui_input, with the existing push_unhandled_input alongside. However, clearly push_input is very special compared to the other two:
It needs to handle window event forwarding, count the events, then does the "_input" job. The other two functions only need to care about the callbacks.

Using four functions sounds more reasonable: push_input, push_local_input, push_local_gui_input, and push_local_unhandled_input. push_input does window event forwarding, counts the event, and then call the other three functions in sequence to handle callbacks within the viewport. Meanwhile, subviewport containers can simply call the respective local functions directly in its event callbacks.

The above two changes make up the above commit.

Here is a small project for testing.
input_event_test.tar.gz

It consists of four buttons, each in a different viewport.

Button 1 in a subviewport in a container
Button 2 in the main viewport, on top of the subviewport
Button 3 in a window
Button 4 in another window, the window is under the subviewport in 2.
There is also an isolated viewport not attaching to any container or window, it has no effect at all, as expected.

Before 67bf3d3 Button 2 was unclickable because the subviewport consumed it.
Before 62070f0 Button 1 couldn't receive key event, found by Sauermann, because the main viewport didn't not know who had the focus.
Before the recent 930428b an event was fired twice if it belonged to a subwindow.

Here is a comparison with and without (Alpha3) this PR when I click a button then press a key for each button

Press Button 1: (both the same)

PR
MOUSE INPUT:  2 Main Button
MOUSE INPUT:  1 SubViewport Button
MOUSE GUI_INPUT:  1 SubViewport Button

Alpha3
MOUSE INPUT:  2 Main Button
MOUSE INPUT:  1 SubViewport Button
MOUSE GUI_INPUT:  1 SubViewport Button

Press a key when Button 1 is focused (both the same)

PR
KEY INPUT:  2 Main Button
KEY INPUT:  1 SubViewport Button
KEY GUI_INPUT:  1 SubViewport Button 
KEY UNHANDLED_INPUT:  2 Main Button
KEY UNHANDLED_INPUT:  1 SubViewport Button

Alpha3
KEY INPUT:  2 Main Button
KEY INPUT:  1 SubViewport Button
KEY GUI_INPUT:  1 SubViewport Button
KEY UNHANDLED_INPUT:  2 Main Button
KEY UNHANDLED_INPUT:  1 SubViewport Button

Press Button 2 (without the fix the event can't go to Button 2 and stops at the subviewport background):

PR
MOUSE INPUT:  2 Main Button
MOUSE INPUT:  1 SubViewport Button
MOUSE GUI_INPUT:  2 Main Button

Alpha3
MOUSE INPUT:  2 Main Button
MOUSE INPUT:  1 SubViewport Button

Press a key when Button 2 is focused (without the fix Button 2 is not clickable so Button 1 still had the focus):

PR
KEY INPUT:  2 Main Button
KEY INPUT:  1 SubViewport Button
KEY GUI_INPUT:  2 Main Button
KEY UNHANDLED_INPUT:  2 Main Button
KEY UNHANDLED_INPUT:  1 SubViewport Button

Alpha3
KEY INPUT:  2 Main Button
KEY INPUT:  1 SubViewport Button
KEY GUI_INPUT:  1 SubViewport Button
KEY UNHANDLED_INPUT:  2 Main Button
KEY UNHANDLED_INPUT:  1 SubViewport Button

For window buttons the events are forwarded to the corrsponding window, so other controls won't recieve any event, as expected. This PR does not change any behavior.

Press Button 3:

PR
MOUSE INPUT:  3 Window Button
MOUSE GUI_INPUT:  3 Window Button

Alpha3
MOUSE INPUT:  3 Window Button
MOUSE GUI_INPUT:  3 Window Button

Press a key when Button 3 is focused

PR
KEY INPUT:  3 Window Button
KEY GUI_INPUT:  3 Window Button
KEY UNHANDLED_INPUT:  3 Window Button

Alpha3
KEY INPUT:  3 Window Button
KEY GUI_INPUT:  3 Window Button
KEY UNHANDLED_INPUT:  3 Window Button

Press Button 4:

PR
MOUSE INPUT:  4 Subviewport Window Button
MOUSE GUI_INPUT:  4 Subviewport Window Button

Alpha3
MOUSE INPUT:  4 Subviewport Window Button
MOUSE GUI_INPUT:  4 Subviewport Window Button

Press a key when Button 4 is focused

PR
KEY INPUT:  4 Subviewport Window Button
KEY GUI_INPUT:  4 Subviewport Window Button
KEY UNHANDLED_INPUT:  4 Subviewport Window Button

Alpha3
KEY INPUT:  4 Subviewport Window Button
KEY GUI_INPUT:  4 Subviewport Window Button
KEY UNHANDLED_INPUT:  4 Subviewport Window Button

* Rearrange push_input/push_unhandled_input as push_input/push_local_input/ push_local_gui_input/push_local_unhandled_input * Fix extra window event forwarding in push_gui_input * Fix code format of the last commit.

Sauermann · 2022-02-23T08:19:06Z

Fix SubViewportContainer processing Events before other Control-Nodes #58334
solves the same problem using a different method. I think, it would be worthwhile, to compare the two approaches.

Sauermann · 2022-02-24T18:49:36Z

I tested the current version bece838 and unfortunately had difficulties with the following things:

Moving one window (in your input_event_test.tar.gz) works without problems, but moving the second window afterwards shifts the window to the position of the first window
Using <tab> to switch between two Control-Nodes, that are in different Viewports, is no longer possible (Load and run TabSwitch.zip, mouse-click on one of the buttons, then use <tab> to switch between them)

zhehangd · 2022-02-24T21:21:04Z

I tested the current version bece838 and unfortunately had difficulties with the following things:

Moving one window (in your input_event_test.tar.gz) works without problems, but moving the second window afterwards shifts the window to the position of the first window

Using <tab> to switch between two Control-Nodes, that are in different Viewports, is no longer possible (Load and run TabSwitch.zip, mouse-click on one of the buttons, then use <tab> to switch between them)

I believe these two issues are from somewhere else. The issues disappear after I merge the PR with the latest master.
Another issue I can see is that the right edge of 1 subviewport button is not clickable likely because of viewport scaling. This is also beyond the scope of this PR.

zhehangd · 2022-02-24T21:29:02Z

I tested the current version bece838 and unfortunately had difficulties with the following things:

Moving one window (in your input_event_test.tar.gz) works without problems, but moving the second window afterwards shifts the window to the position of the first window

Using <tab> to switch between two Control-Nodes, that are in different Viewports, is no longer possible (Load and run TabSwitch.zip, mouse-click on one of the buttons, then use <tab> to switch between them)

Btw may I ask how do you manage to come up with these test cases? Honestly I don't really think so much of them. XD

Sauermann · 2022-02-25T07:49:07Z

The issues disappear after I merge the PR with the latest master.

I am glad to hear that!

Btw may I ask how do you manage to come up with these test cases? Honestly I don't really think so much of them. XD

Usually by just playing around and more importantly creating a Project-file, in which I gather all of them, which I did for my implementation of the patch.

Sauermann · 2022-03-12T10:02:13Z

This PR and PR #58334 both solve #39666 for positional events (events that have a position parameter).
I wanted to provide an overview of the two approaches within the topic of event processing.

Context

Since #39666 is not the only bug related to event processing, we need to consider both approaches in context of solving all event processing related issues. Here is a list of all issues I found, that put constraints on positional event processing:

#39666: _gui_input in parent viewport must happen before _gui_input in child viewport.
#17326: _unhandled_input and physics-picking in child viewport must happen before exit of SubViewportContainer._gui_input in parent viewport.
#28833 _gui_input in child viewport may happen only, if event position is within parent SubViewportContainer.
#58902: _unhandled_input and physics-picking in child viewport may happen only, if event position is within parent SubViewportContainer.
without Bugreport: _input within child viewport may happen only, if event position is within parent SubViewportContainer (otherwise the same problems as in #28833 and #58902 happen for _input when using TouchScreenButton).

The PR #57894 also needs to be taken into account, because it solves #17326.

The following graphic shows the order of positional event processing when considering the three PR. (diagrams.net Source-File)

Events without position parameter

Events without position parameter (like InputEventKey) are not affected by the issues of positional events.

If non-positional events were treated in the same way as positional events, the following problem appears for both combined solutions #58334 + #57894 and #55300 + #57894:
Non-positional events do not get distributed by all SubViewportContainers to their SubViewports _gui_input, which differs from current working behavior.

This argument makes it necessary to treat positional and non-positional events differently in SubViewportContainer.

Additional necessary changes for solving all mentioned issues

The combination #58334 + #57894 solves all mentioned issues.

The combination #55300 + #57894 would require the following additional changes:

SubViewportContainer needs to check in input, if a positional event is within its rect, before sending it to its SubViewport.
A method for distributing non-positional events by all SubViewportContainer to their SubViewport needs to be added.

Architectural comparison

Viewport Interface:
#58334 + #57894: There is a single interface Viewport::push_input for Windows and SubViewportContainers.
#55300 + #57894: Windows use Viewport::push_input, SubViewportContainers use Viewport::push_local_input and Viewport::push_local_gui_input.

Finding key_focus:
#58334 + #57894: Use the established method.
#55300 + #57894: Implements a new method, duplicating aspects of the established method.

SubViewportContainer test for event propagation based on position:
#58334 + #57894: Viewport::_gui_input_event already tests, if event position is within SubViewportContainer.
#55300 + #57894: Besides Viewport::_gui_input_event for _gui_input, an additional test in _input is necessary.

ViewportSource Code Changes:
#58334 + #57894: Makes single change in Viewport.
#55300 + #57894: Makes additions and changes in Viewport.

KoBeWi · 2022-03-12T12:57:18Z

@Sauermann So you basically say that this PR should be closed and the other 2 merged, as they all fix the same issues? Do you maybe have a test project where all these changes can be easily observed?

Sauermann · 2022-03-12T14:12:24Z

@KoBeWi Both approaches are valid solutions and I am not in a position to decide which is better. I just tried to give an overview of the scope of the changes.

The MRP of the linked issues contain use cases for each of the problems. Also I have a test project, that contains use cases for all of them, but it is rather complex: SubViewportClick.zip
Since #58334 and #57894 conflict in headers, I created temporarily a branch with both changes.

akien-mga · 2023-01-27T10:10:17Z

Superseded by #58334. Thanks for the contribution and peer review!

Fix subviewports receiving gui input too early

6d197f3

Fixes godotengine#48401

zhehangd requested review from a team as code owners November 25, 2021 05:58

Chaosus added bug topic:gui topic:input labels Nov 25, 2021

Chaosus added this to the 4.0 milestone Nov 25, 2021

zhehangd added 2 commits November 25, 2021 00:06

Register push_gui_input method and regenerate doc

67bf3d3

Unit test macros use push_gui_input

92ca7db

zhehangd requested a review from a team as a code owner November 25, 2021 09:08

Fix code style

c10cbbb

zhehangd mentioned this pull request Nov 26, 2021

Fix viewport gui input order for 3.x #55339

Closed

KoBeWi reviewed Dec 25, 2021

View reviewed changes

doc/classes/Viewport.xml Show resolved Hide resolved

Add docs for push_*_input

b62a295

akien-mga added the for pr meeting label Jan 5, 2022

akien-mga requested a review from a team January 6, 2022 15:39

reduz approved these changes Feb 5, 2022

View reviewed changes

Sauermann mentioned this pull request Feb 6, 2022

Proposal for SubViewportContainer input handling #57687

Closed

Allows action-type events send through subviewports

62070f0

zhehangd force-pushed the fix_viewport_gui_input_order branch from 2546a07 to 62070f0 Compare February 7, 2022 01:46

zhehangd requested review from reduz and removed request for a team February 23, 2022 03:30

clayjohn requested a review from KoBeWi February 23, 2022 04:02

zhehangd force-pushed the fix_viewport_gui_input_order branch from 930428b to bddae99 Compare February 23, 2022 04:09

Fix window event forwarding

bece838

* Rearrange push_input/push_unhandled_input as push_input/push_local_input/ push_local_gui_input/push_local_unhandled_input * Fix extra window event forwarding in push_gui_input * Fix code format of the last commit.

zhehangd force-pushed the fix_viewport_gui_input_order branch from bddae99 to bece838 Compare February 23, 2022 04:11

aaronfranke mentioned this pull request Mar 22, 2022

Streamline InputEvent class hierarchy to define position property only once #59144

Closed

Sauermann mentioned this pull request Oct 29, 2022

Fix SubViewportContainer processing Events before other Control-Nodes #58334

Merged

akien-mga closed this Jan 27, 2023

akien-mga added the archived label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix subviewports receiving gui input too early #55300

Fix subviewports receiving gui input too early #55300

zhehangd commented Nov 25, 2021 •

edited by akien-mga

Loading

zhehangd commented Nov 25, 2021

zhehangd commented Nov 26, 2021

KoBeWi commented Dec 25, 2021

reduz commented Feb 5, 2022

Sauermann commented Feb 5, 2022 •

edited

Loading

zhehangd commented Feb 7, 2022

Sauermann commented Feb 7, 2022

zhehangd commented Feb 23, 2022

Sauermann commented Feb 23, 2022

Sauermann commented Feb 24, 2022

zhehangd commented Feb 24, 2022

zhehangd commented Feb 24, 2022

Sauermann commented Feb 25, 2022

Sauermann commented Mar 12, 2022 •

edited

Loading

KoBeWi commented Mar 12, 2022

Sauermann commented Mar 12, 2022

akien-mga commented Jan 27, 2023

Fix subviewports receiving gui input too early #55300

Fix subviewports receiving gui input too early #55300

Conversation

zhehangd commented Nov 25, 2021 • edited by akien-mga Loading

Issue

Fix

Other concerns

zhehangd commented Nov 25, 2021

zhehangd commented Nov 26, 2021

KoBeWi commented Dec 25, 2021

reduz commented Feb 5, 2022

Sauermann commented Feb 5, 2022 • edited Loading

zhehangd commented Feb 7, 2022

Sauermann commented Feb 7, 2022

zhehangd commented Feb 23, 2022

Sauermann commented Feb 23, 2022

Sauermann commented Feb 24, 2022

zhehangd commented Feb 24, 2022

zhehangd commented Feb 24, 2022

Sauermann commented Feb 25, 2022

Sauermann commented Mar 12, 2022 • edited Loading

Context

Events without position parameter

Additional necessary changes for solving all mentioned issues

Architectural comparison

KoBeWi commented Mar 12, 2022

Sauermann commented Mar 12, 2022

akien-mga commented Jan 27, 2023

zhehangd commented Nov 25, 2021 •

edited by akien-mga

Loading

Sauermann commented Feb 5, 2022 •

edited

Loading

Sauermann commented Mar 12, 2022 •

edited

Loading