godot
godot copied to clipboard
Fix SubViewportContainer processing Events before other Control-Nodes
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.
- 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.
- 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
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:
With this approach the theoretical order becomes
I think a natural order should be
as if the viewport does not exist at all. Firing an input
event from a gui_input
callback sounds really weird.
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.
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:
@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.
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.
For comparison, here is a graphic of the previous event processing order:
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.
Thanks!