Make mouse enter/exit notifications match mouse events #84547

kitbdev · 2023-11-06T20:15:33Z

I created this based on #83276 to fix some logic and edgecases and to add some tests, since this is important for 4.2.

based on and supersedes Consider children Control nodes for mouse-enter/exit notifications #83276
resolve MouseEnter/Exit events not correctly passed on from children to parent script since Godot 4.2 dev3 #81909
resolve Problems with new mouse_entered and mouse_exited signals #82530
regression from Refactor mouse_entered and mouse_exited signals #67791
supersedes Document mouse signal propagation for MOUSE_FILTER_PASS #82182

Changes:
Mouse Enter/Exit notifications go through non-Control CanvasItems to match mouse event behavior in gui_input.
Handles Mouse Filters.
Handles changing Mouse Filters while hovering a descendant.
Handles changing Top Level in CanvasItems while hovering a descendant.
Handles removing or hiding a Control when hovering a descendant.
Simplified some logic.
Added tests.
Clarified some documentation.

Updated test case "[Viewport][GuiInputEvent] Mouse Motion changes the Control that it is over." to check both mouse enter and mouse enter self notifications.
Added test cases:

"[Viewport][GuiInputEvent] Mouse Enter/Exit notification propagation."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification propagation when moving into child."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification propagation with top level."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification propagation with mouse filter stop."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification propagation with mouse filter ignore."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification when changing top level."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification when changing the mouse filter to stop."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification when removing the hovered Control."
"[Viewport][GuiInputEvent] Mouse Enter/Exit notification when hiding the hovered Control."

The notification propagation test doesn't check for notification order, I'm not sure if it's possible to check that in a test or if it really needs to be tested.

These tests don't cover all edgecases, so I could add more if it is wanted. Such as changing mouse filter ignore when hovering on and off, moving mouse between siblings, or moving the mouse up and down the node hierarchy when mouse filter is stop or is set to top level.

Sauermann

Thanks for your great effort! The changes are looking good so far.

The notification propagation test doesn't check for notification order, I'm not sure if it's possible to check that in a test or if it really needs to be tested.

It is possible to check the notification order. You can see a method for how to do that in the updated #83276.
However I wouldn't consider testing the notification order strictly necessary for this PR.

These tests don't cover all edgecases [...]

I believe, that the implemented set of unit-tests covers the most important cases. I will try to test the implementation before Thursday.

tests/scene/test_viewport.h

scene/main/viewport.h

scene/gui/control.cpp

Sauermann · 2023-11-07T21:58:51Z

I have created a testing project for mouse notifications: HierarchicalMouseNotifications.zip

In most cases this PR works as expected. Only when switching from/to MOUSE_FILTER_IGNORE and when hiding Controls, I found, that there could be some stale mouse-over states.

In the following video I show these.

MouseNotifications.mp4

The first one (top right corner):
0. start with the middle Control set to pass

move mouse cursor over bottom Control
press SPACE to set the middle (right) Control from pass to ignore
Now the middle Control has as state, that the mouse is over the Control, which is not expected.

The second one (bottom left corner):
0. start with the middle Control visible

move mouse cursor over bottom Control
press SPACE to hide the middle (right) Control
now the topmost Control has as state, that the mouse is over the Control, which is not expected

The third one (bottom right corner) A:
0. start with the middle Control set to stop

move mouse cursor over bottom Control
press SPACE to set the middle (right) Control from pass to ignore
Now the middle Control has as state, that the mouse is over the Control, which is not expected.

The third one (bottom right corner) B (not in the video):
0. start with the middle Control set to ignore

move mouse cursor over bottom Control
press SPACE to set the middle (right) Control from ignore to stop
Now the middle Control doesn't receive a mouse-enter notification, but that would be expected.
move the mouse from the bottom Control to the background
Now the middle Control receives a mouse-exit notification (currently without having received a mouse-enter notification before)

I noticed that in contrast to #83276, in this PR a Control with MOUSE_FILTER_IGNORE doesn't break the notification chain. Is there a specific reason for choosing this interpretation?

kitbdev · 2023-11-07T23:03:26Z

a Control with MOUSE_FILTER_IGNORE doesn't break the notification chain. Is there a specific reason for choosing this interpretation?

Yes, since this is how it works with mouse events in _gui_input.
From the mouse filter ignore documentation: This will not block other controls from receiving these events or firing the signals

For changing mouse filter to/from ignore while the mouse is over it or a descendant (your 1st, 3rd, and 4th cases):

I consider this intended, since setting the mouse filter to ignore means it should not receive any notifications or signals.
If the user is tracking the mouse over state and changing the mouse filter to/from ignore, they should expect this behavior or manually set their over state to false when changing to ignore.
Also from the mouse filter ignore documentation: The control will not receive mouse movement input events... The control will also not receive the mouse_entered nor mouse_exited signals.
So if the user changes the filter to ignore, we can't immediately send a mouse signal.

kitbdev · 2023-11-08T00:21:23Z

For your 2nd case, when a Control is hidden (or removed) and the mouse is over it or a descendant:
Currently, the mouse exit notification is sent the next time the mouse over is updated and not null, so the next time a control is hovered.
I was trying to keep it the same as when a Control is added, moved, or shown. In those cases, the mouse over only updates when a mouse event occurs, such as a mouse movement.
However I don't handle when it's null properly, so when the mouse is not over another Control when the one it was over is hidden, then the situation you showed happens.

If we want, I could update the mouse over when a hovered Control is hidden or removed.
~~Alternatively, I could make the Control above the one hidden/removed receive mouse enter self, even if the mouse isn't over it if we want to update only on mouse event.~~

If do we update on hide/remove then some test cases will need to be updated (Mouse Enter/Exit notification when hiding the hovered Control and Mouse Enter/Exit notification when removing the hovered Control) where I check that no Controls received mouse enter self after hiding/removing.

kitbdev · 2023-11-08T01:58:58Z

Updated to handle hiding/removing Controls when the mouse was over a descendant not updating until the mouse was over a Control. This fixes the 2nd case in the testing project (bottom left corner).

The Controls hidden/removed get mouse exit immediately, and the rest of the mouse over updates the next time a mouse event occurs.
If we do want to update the mouse over immediately when a Control is hidden or removed it should probably also update when it is shown, added, and maybe when it is moved. This could be done in a separate PR.

Sauermann · 2023-11-08T20:04:29Z

Yes, since this is how it works with mouse events in _gui_input.

That is good. Also it is consistent with previous behavior.

For changing mouse filter to/from ignore while the mouse is over it or a descendant (your 1st, 3rd, and 4th cases):

I consider this intended, since setting the mouse filter to ignore means it should not receive any notifications or signals. If the user is tracking the mouse over state and changing the mouse filter to/from ignore, they should expect this behavior or manually set their over state to false when changing to ignore.

I see the following problems with this approach:

It requires the user to take care of the mouse-over-state, while it could be done automatically by listening to the signals from the engine. That makes it less user-friendly.
With the PR in its current form, the following behavior is introduced: It is possible (4th case) that a Control node receives a mouse-exited signal, without having received a mouse-entered signal previously. I would consider it a bug, that a mouse-exited signal can happen before a mouse-entered signal.

Also from the mouse filter ignore documentation: The control will not receive mouse movement input events... The control will also not receive the mouse_entered nor mouse_exited signals.
So if the user changes the filter to ignore, we can't immediately send a mouse signal.

In my opinion, the description leaves room for interpretation. The description can be interpreted as: "after the successful change of the mouse-filter to ignore, the control will not receive entered/exited signals." But during the process of changing the filter from pass to ignore, the signals can be received.

I was trying to keep it the same as when a Control is added, moved, or shown. In those cases, the mouse over only updates when a mouse event occurs, such as a mouse movement.

The current behavior is not very robust, and considered a bug (see #40012). See also #78017, which fixed this behavior for Node2D physics nodes. The Control node case is more complex (as seen in #66625) and needs some additional changes before that can be implemented.

Updated to handle hiding/removing Controls when the mouse was over a descendant not updating until the mouse was over a Control. This fixes the 2nd case in the testing project (bottom left corner).

I can confirm this improvement. Now, just like in the Control-movement case, an additional mouse-move is necessary to cause the mouse-exited signal.

If do we update on hide/remove then some test cases will need to be updated (Mouse Enter/Exit notification when hiding the hovered Control and Mouse Enter/Exit notification when removing the hovered Control) where I check that no Controls received mouse enter self after hiding/removing.

Thanks for the info, I will keep it in mind for this situation.

If we do want to update the mouse over immediately when a Control is hidden or removed it should probably also update when it is shown, added, and maybe when it is moved. This could be done in a separate PR.

That change should be postponed until after #40012 gets fixed, so the current handling of hiding nodes is fine.

kitbdev · 2023-11-08T21:43:21Z

I was thinking MOUSE_FILTER_IGNORE could be used as a way to have more control over mouse enter/exit notifications, but I'm not sure there is any use case for having exit called before enter or getting enter called multiple times in a row. In the editor, set_mouse_filter is only ever called when a Control is created and mouse enter/exit is mostly used for keeping track of mouse over state.

I noticed Mouse Enter/Exit Self handles MOUSE_FILTER_IGNORE like you are suggesting and keeping track of the mouse over state seems more important, so I will update to handle mouse filter ignore differently.

I will also add a note to clarify on the Control MOUSE_FILTER_IGNORE docs:
[b]Note:[/b] If the Control has received [signal mouse_enter] but not [signal mouse_exit], changing the [member mouse_filter] to [constant MOUSE_FILTER_IGNORE] will cause [signal mouse_exit] to be emitted.

kitbdev · 2023-11-09T02:35:39Z

Changed to send MOUSE_EXIT when changing a Control to MOUSE_FILTER_IGNORE, and send MOUSE_ENTER when changing a Control to not be MOUSE_FILTER_IGNORE while the mouse is over it or a descendant.
This fixes the remaining cases (1,3,4) from the testing project.

Added test case "[Viewport][GuiInputEvent] Mouse Enter/Exit notification when changing the mouse filter to ignore."

Changing the MOUSE_FILTER_IGNORE behavior made handling each possible case individually in _gui_update_mouse_over more complex, so instead I revamped it to just rebuild the mouse over hierarchy. This should be easier to maintain than having separate cases, and can be used if anything else needs to change the mouse_over_hierarchy structure.

When hovering a control and changing it to MOUSE_FILTER_IGNORE, it will receive MOUSE_EXIT_SELF immediately. Its parent won't receive MOUSE_ENTER_SELF until the next mouse event (similar to hiding). If changing a control from MOUSE_FILTER_IGNORE and the mouse would be directly over it, MOUSE_ENTER_SELF won't be received until the next mouse event. Changing these can also be postponed until after #40012.

Sauermann · 2023-11-09T07:39:08Z

Great work on that, thanks!

I have tested the latest changes and found only one strange case:

MouseNotificationsB.mp4

Testing project: "Switch between Stop and Ignore"

Have the middle Control set to Stop
Move the mouse cursor over the middle Control, so that it isn't over the top-Control
press SPACE to switch from Stop to Ignore
Now the top Control receives a mouse-entered notification, even though the mouse is not over the top-Control. I would expect, that the top-Control doesn't receive a mouse-entered notification.

akien-mga · 2023-11-09T08:17:30Z

CC @mrTag if you could help test that it solves your issues too.

RandomShaper · 2023-11-09T08:42:02Z

scene/main/viewport.cpp

+	LocalVector<Control *> new_mouse_over_hierarchy;
+	LocalVector<Control *> needs_enter;
+	LocalVector<int> needs_exit;


We may want to avoid allocations in such a hot function. @Sauermann has an idea on maybe using just references to nodes at the ends of the chain. Maybe the other vector needs something else. This is just an idea about optimization, not meant as a blocker for this PR in any case.

I'm not sure this function (_gui_mouse_over_hierarchy) is hot. It is only called when set_top_level() or set_mouse_filter() is called, and only executed if the mouse is over something.
If we want I can add a guard before the allocations that is similar to the loop below it but smaller.

Also the mouse_over_hierarchy contains a list of all nodes that have gotten mouse_entered but still need a mouse_exited. If we change this to only store the ends, we wouldn't be able to send the correct signals when the mouse filter ignore is changed, or it would be more complicated.

doc/classes/Control.xml

mrTag · 2023-11-09T10:49:19Z

I just tested this PR with our game and can confirm: mouse_enter/exit works as expected and our tooltips are not broken anymore.
Great work! Thank you!

`NOTIFICATION_MOUSE_ENTER` and `NOTIFICATION_MOUSE_EXIT` now includes the areas of children control nodes if the mouse filters allow it. In order to check if a Control node itself was entered/exited, the newly introduced `NOTIFICATION_MOUSE_ENTER_SELF` and `NOTIFICATION_MOUSE_EXIT_SELF` can be used. Co-authored-by: Markus Sauermann <6299227+Sauermann@users.noreply.github.com>

kitbdev · 2023-11-09T14:13:14Z

I have tested the latest changes and found only one strange case:

Changing from stop to ignore:
This seems to be because gui.mouse_over isn't updated until the next mouse event. This can probably also be postponed until after #40012.
Or if it is important we fix this now, I can look into updating the mouse over immediately.

kitbdev · 2023-11-09T14:16:36Z

Updated to add is_experimental to the *_self notifications, and fix a comma.

Unrelated: The Control documentation seems to use control, Control, [Control], the node, and this node interchangeably.

Sauermann · 2023-11-09T14:20:13Z

Or if it is important we fix this now, I can look into updating the mouse over immediately.

When we discussed it, this seems to be a minor edge case and could be deferred to a followup-PR. However from my analysis, this has nothing to do with updates after the next mouse-move event, because the problem happens long before the next mouse event.

akien-mga

We reviewed this early today with the input team and approved it for merging. It seems to be a pretty thorough solution with extensive unit testing.

We had some doubts on whether the ENTER_SELF and EXIT_SELF notifications are needed, which is why they're now marked as experimental. The engine itself doesn't use them, so we can see if users start making good use of the additional functionality or if they find it too confusing.

akien-mga · 2023-11-09T17:24:39Z

Thanks @kitbdev and @Sauermann!

Amazing work fixing a critical regression so fast during the beta stage :)

Sauermann · 2023-11-09T22:20:52Z

Thanks @kitbdev for putting so much effort into this!

dobin · 2023-12-22T09:07:20Z

Is this already in 4.2, or later?

KoBeWi · 2023-12-22T10:42:00Z

It's in 4.2.

kitbdev requested review from a team as code owners November 6, 2023 20:15

akien-mga added bug enhancement needs testing topic:input regression topic:gui labels Nov 6, 2023

akien-mga added this to the 4.2 milestone Nov 6, 2023

kitbdev force-pushed the mouse-notif-3 branch from cbe6c02 to 5adf98c Compare November 6, 2023 20:48

Sauermann reviewed Nov 6, 2023

View reviewed changes

tests/scene/test_viewport.h Show resolved Hide resolved

scene/main/viewport.h Show resolved Hide resolved

scene/gui/control.cpp Show resolved Hide resolved

kitbdev force-pushed the mouse-notif-3 branch from 5adf98c to fd08ed7 Compare November 7, 2023 18:26

kitbdev force-pushed the mouse-notif-3 branch from fd08ed7 to add52a1 Compare November 8, 2023 01:52

kitbdev force-pushed the mouse-notif-3 branch from add52a1 to 3aeb26a Compare November 9, 2023 02:33

RandomShaper reviewed Nov 9, 2023

View reviewed changes

akien-mga reviewed Nov 9, 2023

View reviewed changes

doc/classes/Control.xml Outdated Show resolved Hide resolved

akien-mga mentioned this pull request Nov 9, 2023

MouseEnter/Exit events not correctly passed on from children to parent script since Godot 4.2 dev3 #81909

Closed

kitbdev force-pushed the mouse-notif-3 branch from 3aeb26a to d24d73b Compare November 9, 2023 14:13

akien-mga approved these changes Nov 9, 2023

View reviewed changes

akien-mga mentioned this pull request Nov 9, 2023

Consider children Control nodes for mouse-enter/exit notifications #83276

Closed

akien-mga merged commit d36cc73 into godotengine:master Nov 9, 2023
15 checks passed

kitbdev deleted the mouse-notif-3 branch November 9, 2023 22:53

Sauermann mentioned this pull request Nov 9, 2023

Control node "Pass" mousefilter is not passing input #55432

Open

This was referenced Nov 13, 2023

Refactor mouse_entered and mouse_exited signals #67791

Merged

Control's mouse_exit() is emitted when the mouse is moved to another control inside that control #16854

Closed

akien-mga changed the title ~~Make Mouse Enter/Exit notifications match Mouse Events~~ Make mouse enter/exit notifications match mouse events Nov 13, 2023

lostminds mentioned this pull request Nov 14, 2023

Inconsistent partial mouse event handling for unfocusable windows #84700

Open

SeremTitus mentioned this pull request Nov 14, 2023

actual hover using mouse_entered and mouse_exited control signals MewPurPur/GodSVG#211

Merged

akien-mga mentioned this pull request Nov 23, 2023

Crash when hiding a panel from a NOTIFICATION_MOUSE_ENTER notification #85277

Closed

Benardus mentioned this pull request Nov 28, 2023

MouseEntered not detected on area2D with child CollisionShape2D #85469

Closed

YuriSizov removed the needs testing label Dec 22, 2023

dobin mentioned this pull request Dec 22, 2023

Mouse Filter Stop on parent swallows on_mouse_exited() #86445

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make mouse enter/exit notifications match mouse events #84547

Make mouse enter/exit notifications match mouse events #84547

kitbdev commented Nov 6, 2023 •

edited

Loading

Sauermann left a comment

Sauermann commented Nov 7, 2023

kitbdev commented Nov 7, 2023 •

edited

Loading

kitbdev commented Nov 8, 2023 •

edited

Loading

kitbdev commented Nov 8, 2023 •

edited

Loading

Sauermann commented Nov 8, 2023

kitbdev commented Nov 8, 2023

kitbdev commented Nov 9, 2023 •

edited

Loading

Sauermann commented Nov 9, 2023 •

edited

Loading

akien-mga commented Nov 9, 2023

RandomShaper Nov 9, 2023

kitbdev Nov 9, 2023 •

edited

Loading

kitbdev Nov 9, 2023 •

edited

Loading

mrTag commented Nov 9, 2023

kitbdev commented Nov 9, 2023

kitbdev commented Nov 9, 2023 •

edited

Loading

Sauermann commented Nov 9, 2023

akien-mga left a comment

akien-mga commented Nov 9, 2023

Sauermann commented Nov 9, 2023

dobin commented Dec 22, 2023

KoBeWi commented Dec 22, 2023

Make mouse enter/exit notifications match mouse events #84547

Make mouse enter/exit notifications match mouse events #84547

Conversation

kitbdev commented Nov 6, 2023 • edited Loading

Sauermann left a comment

Choose a reason for hiding this comment

Sauermann commented Nov 7, 2023

kitbdev commented Nov 7, 2023 • edited Loading

kitbdev commented Nov 8, 2023 • edited Loading

kitbdev commented Nov 8, 2023 • edited Loading

Sauermann commented Nov 8, 2023

kitbdev commented Nov 8, 2023

kitbdev commented Nov 9, 2023 • edited Loading

Sauermann commented Nov 9, 2023 • edited Loading

akien-mga commented Nov 9, 2023

RandomShaper Nov 9, 2023

Choose a reason for hiding this comment

kitbdev Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

kitbdev Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

mrTag commented Nov 9, 2023

kitbdev commented Nov 9, 2023

kitbdev commented Nov 9, 2023 • edited Loading

Sauermann commented Nov 9, 2023

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented Nov 9, 2023

Sauermann commented Nov 9, 2023

dobin commented Dec 22, 2023

KoBeWi commented Dec 22, 2023

kitbdev commented Nov 6, 2023 •

edited

Loading

kitbdev commented Nov 7, 2023 •

edited

Loading

kitbdev commented Nov 8, 2023 •

edited

Loading

kitbdev commented Nov 8, 2023 •

edited

Loading

kitbdev commented Nov 9, 2023 •

edited

Loading

Sauermann commented Nov 9, 2023 •

edited

Loading

kitbdev Nov 9, 2023 •

edited

Loading

kitbdev Nov 9, 2023 •

edited

Loading

kitbdev commented Nov 9, 2023 •

edited

Loading