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

Handle potential platform-specific Window mouse-enter/exit bugs gracefully #80187

Merged

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Aug 2, 2023

Also replace DEV_ASSERT by ERR_FAIL.

followup to #67791
implement comment #67791 (comment)
based on #80156
fix #80335

@Sauermann
Copy link
Contributor Author

After giving it some thought, I'm no longer sure, if ERR_FAIL is needed, because with this change, Window handles all possible cases.

} break;
case DisplayServer::WINDOW_EVENT_MOUSE_EXIT: {
Window *root = get_tree()->get_root();
DEV_ASSERT(root->gui.windowmanager_window_over); // Exiting a window, while no window is hovered should never happen.
ERR_FAIL_NULL_MSG(root->gui.windowmanager_window_over, "Exiting a window, while no window is hovered should never happen.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ERR_FAIL_NULL_MSG(root->gui.windowmanager_window_over, "Exiting a window, while no window is hovered should never happen.");
ERR_FAIL_NULL_MSG(root->gui.windowmanager_window_over, "Exiting a window while no window is hovered should never happen.");

But "should never happen" is still the kind of message that one would use for a DEV_ASSERT.

If it can be triggered by user code, and thus require an ERR_FAIL for graceful handling, then it does actually happen and the message should clarify what went wrong so that the user can fix it on their side.

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess here the "user" can be a platform port that does things wrong in its window handling? So I guess it makes sense as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the question, that I try to wrap my head around.
If "user" is a engine-developer of platform code, then the error-message helps identifying an engine-problem.
if "user" is a game-developer, then the error-message has no benefit, because the game-developer can not address the problem.

In any case, I have accepted your code-change suggestion.

Copy link
Member

@bruvzg bruvzg left a comment

Choose a reason for hiding this comment

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

After giving it some thought, I'm no longer sure, if ERR_FAIL is needed, because with this change, Window handles all possible cases.

Probably not, I'm sure, if arrival of system move event messages is always guaranteed, and error won't be useful for users. It should be OK to simply ignore duplicate mouse enter/exit events.

@@ -672,16 +672,21 @@ void Window::_event_callback(DisplayServer::WindowEvent p_event) {
case DisplayServer::WINDOW_EVENT_MOUSE_ENTER: {
_propagate_window_notification(this, NOTIFICATION_WM_MOUSE_ENTER);
Window *root = get_tree()->get_root();
DEV_ASSERT(!root->gui.windowmanager_window_over); // Entering a window while a window is hovered should never happen.
Window *previous_over = root->gui.windowmanager_window_over;
Copy link
Member

Choose a reason for hiding this comment

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

If mouse exit was not received, windowmanager_window_over might be an invalid pointer to the deleted Window (the same might happen in Viewport._update_mouse_over()). Maybe ObjectID should be stored instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I haven't thought about stale gui.windowmanager_window_over. Thanks for pointing me to this. I will investigate this and try to check situations, where this happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried multiple ways to cause a crash on X11 based on stale gui.windowmanager_window_over

  • situations, that crashed without this fix
  • explicitly freeing (queue_free and free)

But I wasn't able to cause a crash. However I can't rule out, that it is possible to cause a crash this way.

@Sauermann Sauermann force-pushed the fix-mouseover-error-handling branch from 81d2126 to b35f9de Compare August 3, 2023 21:33
Copy link
Contributor Author

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

Probably not, I'm sure, if arrival of system move event messages is always guaranteed, and error won't be useful for users. It should be OK to simply ignore duplicate mouse enter/exit events.

So for me this boils down to the following question:

  • Should platform-implementations be forced to adhere to the same behavior regarding send mouse-enter window-events or
  • should platform-implementations be allowed to be vague about mouse-enter window-events and catch all possible cases in the Window-class?

Personally I believe that the goal should be to align the behavior of all platform-implementations.

@@ -672,16 +672,21 @@ void Window::_event_callback(DisplayServer::WindowEvent p_event) {
case DisplayServer::WINDOW_EVENT_MOUSE_ENTER: {
_propagate_window_notification(this, NOTIFICATION_WM_MOUSE_ENTER);
Window *root = get_tree()->get_root();
DEV_ASSERT(!root->gui.windowmanager_window_over); // Entering a window while a window is hovered should never happen.
Window *previous_over = root->gui.windowmanager_window_over;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I haven't thought about stale gui.windowmanager_window_over. Thanks for pointing me to this. I will investigate this and try to check situations, where this happens.

} break;
case DisplayServer::WINDOW_EVENT_MOUSE_EXIT: {
Window *root = get_tree()->get_root();
DEV_ASSERT(root->gui.windowmanager_window_over); // Exiting a window, while no window is hovered should never happen.
ERR_FAIL_NULL_MSG(root->gui.windowmanager_window_over, "Exiting a window, while no window is hovered should never happen.");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the question, that I try to wrap my head around.
If "user" is a engine-developer of platform code, then the error-message helps identifying an engine-problem.
if "user" is a game-developer, then the error-message has no benefit, because the game-developer can not address the problem.

In any case, I have accepted your code-change suggestion.

@clayjohn
Copy link
Member

clayjohn commented Aug 8, 2023

Testing this locally. It fixes the crash caused by the dev assert. But every single time I navigate within the 3D viewport (RMB + mouse move for example) I get the "Entering a window while hovering it should never happen." error.

@lawnjelly
Copy link
Member

Maybe a WARN_PRINT_ONCE might be better if it is spamming until the bug causing this is fixed?

@Sauermann
Copy link
Contributor Author

Had the same idea about WARN_PRINT_ONCE. Will update the PR.

…cefully

Also replace `DEV_ASSERT` by `WARN_PRINT_ONCE`.
@Sauermann Sauermann force-pushed the fix-mouseover-error-handling branch from a81a6aa to 2f8673d Compare August 8, 2023 11:06
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems good as a stopgap solution.

@akien-mga akien-mga merged commit af722e2 into godotengine:master Aug 8, 2023
15 checks passed
@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.

Crash when dragging the mouse in the inspector to modify a value
6 participants