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 SubViewportContainer processing Events before other Control-Nodes #58334

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Feb 20, 2022

Currently, SubViewportContainer propagates all its events in _input
1 before all other Control-Nodes (fix #39666)
2 and where rect_position and rect_size of a Control-Node do not matter (fix #28833)

fix one of the reasons for #58902
alternative to #55300

MRP for all related Bugs: SubViewportEventBugs.zip

This patch changes SubViewportContainer, so that it propagates positional events (events that have a position property) no longer in _input, but in _gui_input to its SubVieworts via push_input.

Event coordinates

Since _input and _gui_input expect different event-position-coordinate-systems, they now have to be adjusted only with the shrink factor in _gui_input.

Changes to Events

Here is a description of how this patch affects events.

  1. Events, that contain a position

The position of the event (like a mouse-click) specifies the Control-Node, they are intended for. Just like in master, these events get delivered to the Control-Node, that is located at the position. They are no longer delivered automatically to SubViewports of other SubViewportContainer, that are away from the position. The following graphic shows how event propagation of positional events (green line) changes.

image

  1. Events, that don't contain a position

Those events get sent to the same nodes in the same order as in the current master.

Additional thoughts

Update 2022-09-29: Evaluate positional Events explicitely
Update 2022-10-02: Fix coordinate transform in _gui_input

@zhehangd
Copy link
Contributor

My main concern is that this may create a very confusing event order.
Say you have three ColorRect: R,G,B where G is in a subviewport, like below:
Screenshot_20220223_183203

With this approach the theoretical order becomes
Screenshot_20220223_183950

I think a natural order should be
Screenshot_20220223_183956

as if the viewport does not exist at all. Firing an input event from a gui_input callback sounds really weird.

@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 24, 2022

Thanks for your review. The following perspective may give a different view on the perceived weirdness:
A Viewport is a different world and as its own entity, it should know, how to process an incoming event (via push_input) all on its own, without the outer world telling it, in which order to process events. This makes push_input the single point of contact between the two worlds and as such simplifies the Viewport interface. So it is not about "Firing an input event from a gui_input callback", but about pushing the event to the subviewport, so that it handles it on its own and gui_input is the natural place for SubViewportContainer as a Control-Node to handle their input.

In your suggested order, the combination "R(gui) -> ... -> G(unhandled)" is the reason for issue #17326, because right after "SubViewportContainer(gui)" the event gets set to handled. This makes the sequence "G(gui) -> G(unhandled) -> ... -> R(gui)" necessary.

Change SubViewportContainer, so that it processes events with a position property no longer in 'input', but in 'gui_input'.
This fixes the issue, that Nodes within its SubViewport receive MouseButton events before other Control-Nodes.
@Sauermann
Copy link
Contributor Author

Sauermann commented Nov 2, 2022

Together with #57894, this PR fixes several outstanding problems about SubViewport-InputEvent-propagation. Since these problems are reported in different Bug reports with multiple MRPs, I created a single MRP, that demonstrates all of the problems:

SubViewportEventBugs.zip

image

@KoBeWi some time ago you asked for a project file for these problems, but I only had a very complicated one available. I believe, that this should make the review-process simpler.

@Sauermann
Copy link
Contributor Author

Sauermann commented Jan 22, 2023

With the goal of a better understanding of the changes from my PRs I have created a documentation graphic for event processing including changes from #57894.

InputEventsFlowchart drawio

For comparison, here is a graphic of the previous event processing order:

InputEventsFlowchartOld drawio

@reduz
Copy link
Member

reduz commented Jan 27, 2023

This took a while to understand properly because there is just so much to consider, but at this point I agree it's the best possible way to do it.

@akien-mga akien-mga merged commit 3e1925f into godotengine:master Jan 27, 2023
@akien-mga
Copy link
Member

Thanks!

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.

Click Events passing through Buttons if there is a Viewport underneath Viewport not clip control node input
4 participants