godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix SubViewportContainer processing Events before other Control-Nodes

Open Sauermann opened this issue 3 years ago • 3 comments

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

  • This bugfix solves problems in a different part of Event handling in comparison to #57894

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

Sauermann avatar Feb 20 '22 01:02 Sauermann

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.

zhehangd avatar Feb 24 '22 02:02 zhehangd

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.

Sauermann avatar Feb 24 '22 06:02 Sauermann

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 avatar Nov 02 '22 18:11 Sauermann

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

Sauermann avatar Jan 22 '23 13:01 Sauermann

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.

reduz avatar Jan 27 '23 09:01 reduz

Thanks!

akien-mga avatar Jan 27 '23 10:01 akien-mga