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 unrestricted mouse-event propagation to SubViewports for Physics-Picking #57894

Merged
merged 1 commit into from
May 9, 2023

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Feb 10, 2022

resolve #17326
resolve #58902
resolve #75015
resolve #75019

previous description

Currently, Events get processed by Nodes in order of the green arrow in the following graphic, when aSubViewportContainer is within the Root Viewport:

CurrentSubViewportHandlingOld

The red "X" denotes the place where set_input_as_handled() is called within Viewport::_gui_input_event, when an InputEventMouseMotion happens over the SubViewportContainer. Here is the according location in the source code:

godot/scene/main/viewport.cpp

Lines 1789 to 1796 in 84f2c68

bool stopped = false;
if (over && over->can_process()) {
stopped = _gui_call_input(over, mm);
}
if (stopped) {
set_input_as_handled();
}

If the SubViewportContainer marks the event as stopped, _unhandled_input is never called.
If the SubViewportContainer ignores the event, it is processed in _unhandled_input, even though ignoring should be a synonym for not sending the event to the SubViewport of SubViewportContainer.

This pull request makes sure, that the event reaches _unhandled_input inside the SubViewport before the "X":

CurrentSubViewportHandlingNew

This is done by moving the push_unhandled_input call from Window::_window_input to Viewport::push_input.

This change makes SubViewportContainer::unhandled_input redundant, since it would distribute the event to _unhandled_input inside the SubViewport a second time.

Shortcut Events now need to be distributed via push_input, in order for them to be able to reach SubViewports.

MRP for all linked issues: SubViewportEventBugs.zip

BugsSVC

Currently InputEvents get sent unconditionally to SubViewports via SubViewportContainer::unhandled_input, even if the event-position is outside of the area of the SubViewportContainer. This leads to problems like #58902, where the mouse enters CollisionObjects2D, even if they are not visible. (in the MRP, clicking in the red area causes a mouse-click on the non-visible Logo within the SubViewport).

Also hovering CollisionObjects2D within SubViewports currently is reversed:

  • If the SubViewportContainer is set to MOUSE_FILTER_IGNORE, the CollisionObject2D does receive mouse events.
  • If the SubViewportContainer is set to MOUSE_FILTER_STOP, the CollisionObject2D doesn't receive mouse events.

A third bug is that a Panel with MOUSE_FILTER_STOP, parent of a SubViewportContainer, prohibits Physics-picking in the SubViewport.

The fourth bug, that gets fixed is that TouchScreenButtons don't receive some mouse events within _unhandled_input. (TouchScreenButton doesnt send event via _unhandled_input)

This PR solves all problems by moving the call of push_unhandled_input from Window::_window_input to Viewport::push_input.
This makes sure, that only events with a position within the area of the SubViewportContainer are considered for physics picking.
This change makes SubViewportContainer::unhandled_input redundant, since it would distribute the event to _unhandled_input inside the SubViewport a second time.

Updated 2022-09-26: fix merge conflict, made sure that this PR is still relevant after #61088 and adjusted description.
Updated 2022-10-14: adjust shortcut event handling
Updated 2022-11-06: update documentation of push_input.
Updated 2022-11-23: fix merge conflict
Updated 2023-01-27: fix merge conflict, update description based on the recently merged #58334
Updated 2023-03-17: fix merge conflict, updated MRP, linked a third bug, that this PR fixes.
Updated 2023-05-08: linked a fourth bug, that this PR fixes and updated description and MRP.

@Sauermann Sauermann requested review from a team as code owners February 10, 2022 01:17
@akien-mga akien-mga added this to the 4.0 milestone Feb 10, 2022
@Sauermann Sauermann force-pushed the fix-subviewport-1 branch 2 times, most recently from 7bc5bd8 to c42a242 Compare March 31, 2022 09:54
@Sauermann Sauermann requested a review from a team as a code owner November 6, 2022 19:59
@Sauermann Sauermann changed the title Fix that unhandled_input events do not get sent to SubViewports Fix unrestricted mouse-event propagation to SubViewports for Physics-Picking Jan 27, 2023
@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 20, 2023
This solves the problem, that mouse events get sent to SubViewports
even if they are outside of the visible area of the SubViewport.

This changes makes SubViewportContainer::unhandled_input redundand.
Shortcut Events now need to be distributed via push_input, in order for
them to be able to reach SubViewports.
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Tested the attached project + some random stuff and it appears to be working correctly.

@RandomShaper
Copy link
Member

I think it's a good time to merge this.

@akien-mga akien-mga merged commit 72323a5 into godotengine:master May 9, 2023
@akien-mga
Copy link
Member

Thanks!

@WolfgangSenff
Copy link
Contributor

I commented already in #17326, but I think some or much of this PR may be missing from the latest beta, 4.2.beta5. At least, I looked at the code and it seemed like some of it was missing - could be it was updated in the future and I missed that.

@Sauermann
Copy link
Contributor Author

@WolfgangSenff The only thing, that changed, was the renaming of _push_unhandled_input to _push_unhandled_input_internal, but the important parts of this PR are still in the code.

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