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

Fix Viewport mouse enter+exit notifications #58042

Merged

Conversation

Sauermann
Copy link
Contributor

resolve #57762

Viewports of a Window receive the notifications NOTIFICATION_WM_MOUSE_ENTER, NOTIFICATION_WM_MOUSE_EXIT, and NOTIFICATION_WM_WINDOW_FOCUS_OUT directly from their window:

godot/scene/main/window.cpp

Lines 326 to 327 in aa069a4

void Window::_propagate_window_notification(Node *p_node, int p_notification) {
p_node->notification(p_notification);

Since SubViewports, that are child-nodes of a SubViewportContainer, do not have a Window, they do not receive these events, which leads to the mentioned bug.
This pull request solves this problem by propagating the notifications of SubViewportContainer to their SubViewports.

@Sauermann Sauermann requested a review from a team as a code owner February 13, 2022 08:10
@akien-mga akien-mga added this to the 4.0 milestone Feb 13, 2022
@reduz
Copy link
Member

reduz commented Feb 14, 2022

Looks quite good. @KoBeWi what do you think of it?

@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2022

This fixes the entered signals, but exit signals seem to be the same. Is this expected?

@Sauermann
Copy link
Contributor Author

@KoBeWi yes, this is expected. This is a different bug and it is tracked in #28833.
Regarding exit events, this pull request fixes, that they no longer get dropped.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2022

And what about NOTIFICATION_WM_WINDOW_FOCUS_IN? If out is received, so should in, no?

@Sauermann
Copy link
Contributor Author

Viewport::_notification does not act on NOTIFICATION_WM_WINDOW_FOCUS_IN so I thought, there is no need to forward it.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2022

It doesn't act, but the notification is still received and can be used in scripts etc. So it should be forwarded too.

@Sauermann
Copy link
Contributor Author

@KoBeWi Ok, I will include that notification. Are there any additional notifications, that should be forwarded?

For windows, I found the following notifications:
NOTIFICATION_WM_CLOSE_REQUEST
NOTIFICATION_WM_GO_BACK_REQUEST
NOTIFICATION_WM_DPI_CHANGE
NOTIFICATION_WM_SIZE_CHANGED

NOTIFICATION_RESIZED is already handled in SubViewportContainer, but for the others I believe that there is no fitting notification in SubViewportContainer.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2022

Are there any additional notifications, that should be forwarded? For windows, I found the following notifications:
NOTIFICATION_WM_CLOSE_REQUEST
NOTIFICATION_WM_GO_BACK_REQUEST
NOTIFICATION_WM_DPI_CHANGE

I don't think these need to be forwarded.

But now that I look at this PR, it sends window-related notifications based on different notifications. I wonder if this should be documented, as users might find it unexpected.

@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 14, 2022

it sends window-related notifications based on different notifications

I find it confusing, that NOTIFICATION_WM_MOUSE_ENTER is considered a window-related notification. It doesn't trigger, when the mouse enters the window (which can have a title-bar), but when the mouse enters the viewport of the window. To me it would make more sense, if this notification was considered a viewport-related notification.

I see a problem with the current patch. I believe, that NOTIFICATION_FOCUS_EXIT should not be forwarded as NOTIFICATION_WM_WINDOW_FOCUS_OUT.

NOTIFICATION_WM_WINDOW_FOCUS_OUT means that the window no longer has focus.
NOTIFICATION_FOCUS_EXIT means that a Control no longer has the status of key_focus.
These are two different things and seem to be incompatible.

@Sauermann Sauermann force-pushed the fix-viewport-border-notifications branch from 597a92f to 7d5931f Compare February 14, 2022 16:37
@Sauermann Sauermann requested review from a team as code owners February 14, 2022 16:37
@Sauermann
Copy link
Contributor Author

Sauermann commented Feb 14, 2022

My updated pull request uses different notifications for two concepts:

  • mouse being over the window (global state for all nodes) (just as before)
  • mouse being over a viewport (viewport-local state) (newly introduced notification)

Otherwise the global state would interfere with subviewports.

@Sauermann Sauermann force-pushed the fix-viewport-border-notifications branch from 7d5931f to ad4b043 Compare February 14, 2022 16:43
@Sauermann Sauermann changed the title Fix SubViewportContainer border notifications Fix Viewport mouse enter+exit notifications Feb 14, 2022
@reduz
Copy link
Member

reduz commented Feb 15, 2022

Some notes of concern here that I was thinking, the WM focus in and out notifications are propagated through the whole tree to all nodes. I have my doubts whether this is really useful, maybe they should be signals of SceneTree. The problem though is that here the sub-viewport will receive them now.

The problem here is that the WM notifications will be received twice by the sub-viewport.

@Sauermann
Copy link
Contributor Author

When I noticed, that WM-mouse-enter+exit is distributed to all Nodes, I became aware, that my initial solution would not work. In the most recent patch, I created new notifications, that trigger when the mouse enters a viewport.
For the root-viewport, this notification comes from the Window.
For other sub-viewports, this notification comes from their parent SubViewportContainer.

@Sauermann Sauermann force-pushed the fix-viewport-border-notifications branch from ad4b043 to 415042a Compare February 15, 2022 21:50
@reduz
Copy link
Member

reduz commented Feb 19, 2022

This makes sense, I am still unhappy to have it in Node, but Viewport has no notifications so not much we can do for the time being.

@akien-mga akien-mga merged commit 417698c into godotengine:master Feb 19, 2022
@akien-mga
Copy link
Member

Thanks!

@Sauermann Sauermann deleted the fix-viewport-border-notifications branch February 19, 2022 08:07
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.

Notification problem at SubViewportContainer border
4 participants