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.

Sign up for GitHub

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 crash on hiding grandparent Control on mouse exit #85313

Merged
merged 1 commit into from
Dec 8, 2023
Merged

Fix crash on hiding grandparent Control on mouse exit #85313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion scene/main/viewport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2451,6 +2451,14 @@ void Viewport::_gui_update_mouse_over() {
return;
}

if (gui.sending_mouse_enter_exit_notifications) {
// If notifications are already being sent, delay call to next frame.
if (get_tree() && !get_tree()->is_connected(SNAME("process_frame"), callable_mp(this, &Viewport::_gui_update_mouse_over))) {
get_tree()->connect(SNAME("process_frame"), callable_mp(this, &Viewport::_gui_update_mouse_over), CONNECT_ONE_SHOT);
}
return;
}

// Rebuild the mouse over hierarchy.
LocalVector<Control *> new_mouse_over_hierarchy;
LocalVector<Control *> needs_enter;
Expand Down Expand Up @@ -2507,6 +2515,8 @@ void Viewport::_gui_update_mouse_over() {
return;
}

gui.sending_mouse_enter_exit_notifications = true;
Copy link
Contributor

@Sauermann Sauermann Dec 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In the implementation you keep track in every Viewport separately, if sending_mouse_enter_exit_notifications is active or not.
Have you considered the idea to keep track of the state in a global manner (for example only in the root-Viewport (even when the entry-point is not the root-Viewport))? I'm not entirely sure, if there can be edge cases, where these notifications in different Viewports could interfere with each other.

However, for now it looks like a reasonable approach.


// Send Mouse Exit Self notification.
if (gui.mouse_over && !needs_exit.is_empty() && needs_exit[0] == (int)gui.mouse_over_hierarchy.size() - 1) {
gui.mouse_over->notification(Control::NOTIFICATION_MOUSE_EXIT_SELF);
Expand All @@ -2528,6 +2538,8 @@ void Viewport::_gui_update_mouse_over() {
for (int i = needs_enter.size() - 1; i >= 0; i--) {
needs_enter[i]->notification(Control::NOTIFICATION_MOUSE_ENTER);
}

gui.sending_mouse_enter_exit_notifications = false;
}

Window *Viewport::get_base_window() const {
Expand Down Expand Up @@ -3200,17 +3212,21 @@ void Viewport::_update_mouse_over(Vector2 p_pos) {
gui.mouse_over = over;
gui.mouse_over_hierarchy.reserve(gui.mouse_over_hierarchy.size() + over_ancestors.size());

gui.sending_mouse_enter_exit_notifications = true;

// Send Mouse Enter notifications to parents first.
for (int i = over_ancestors.size() - 1; i >= 0; i--) {
over_ancestors[i]->notification(Control::NOTIFICATION_MOUSE_ENTER);
gui.mouse_over_hierarchy.push_back(over_ancestors[i]);
over_ancestors[i]->notification(Control::NOTIFICATION_MOUSE_ENTER);
}

// Send Mouse Enter Self notification.
if (gui.mouse_over) {
gui.mouse_over->notification(Control::NOTIFICATION_MOUSE_ENTER_SELF);
}

gui.sending_mouse_enter_exit_notifications = false;

notify_embedded_viewports = true;
}
}
Expand Down Expand Up @@ -3252,6 +3268,12 @@ void Viewport::_mouse_leave_viewport() {
}

void Viewport::_drop_mouse_over(Control *p_until_control) {
if (gui.sending_mouse_enter_exit_notifications) {
// If notifications are already being sent, defer call.
callable_mp(this, &Viewport::_drop_mouse_over).call_deferred(p_until_control);
return;
}
Comment on lines +3271 to +3275
Copy link
Member

Choose a reason for hiding this comment

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

I'm not familiar with this code, but slightly worried at the possibility of a deferred infinite recursion here. Are we absolutely sure that this won't end up deferring itself over and over again without ever reaching a condition that would make gui.sending_mouse_enter_exit_notifications false?

Copy link
Contributor Author

@kitbdev kitbdev Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, since every time gui.sending_mouse_enter_exit_notifications is set to true, it is immediately set to false in the same scope after it is done sending a fixed amount of notifications. So it will never be true by the time deferred calls are made.
I'm not sure if there is a way to tell if this call is being called from the deferred queue, but I could add a check just in case something changes and it is set to true?

Also this method only removes Controls from a list and cannot add any, so it cannot keep calling itself infinitely from user code, like was previously possible in _gui_update_mouse_over.


_gui_cancel_tooltip();
SubViewportContainer *c = Object::cast_to<SubViewportContainer>(gui.mouse_over);
if (c) {
Expand All @@ -3263,6 +3285,8 @@ void Viewport::_drop_mouse_over(Control *p_until_control) {
v->_mouse_leave_viewport();
}
}

gui.sending_mouse_enter_exit_notifications = true;
if (gui.mouse_over && gui.mouse_over->is_inside_tree()) {
gui.mouse_over->notification(Control::NOTIFICATION_MOUSE_EXIT_SELF);
}
Expand All @@ -3276,6 +3300,7 @@ void Viewport::_drop_mouse_over(Control *p_until_control) {
}
}
gui.mouse_over_hierarchy.resize(notification_until);
gui.sending_mouse_enter_exit_notifications = false;
}

void Viewport::push_input(const Ref<InputEvent> &p_event, bool p_local_coords) {
Expand Down
1 change: 1 addition & 0 deletions scene/main/viewport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,7 @@ class Viewport : public Node {
Control *key_focus = nullptr;
Control *mouse_over = nullptr;
LocalVector<Control *> mouse_over_hierarchy;
bool sending_mouse_enter_exit_notifications = false;
YuriSizov marked this conversation as resolved.
Show resolved Hide resolved
Window *subwindow_over = nullptr; // mouse_over and subwindow_over are mutually exclusive. At all times at least one of them is nullptr.
Window *windowmanager_window_over = nullptr; // Only used in root Viewport.
Control *drag_mouse_over = nullptr;
Expand Down
Loading