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

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

Closed

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 13, 2023

NOTIFICATION_MOUSE_ENTER and NOTIFICATION_MOUSE_EXIT and the according signals now includes the areas of children control nodes.

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.

resolve #81909
resolve #82530
regression from #67791
supersedes #82182

@kitbdev
Copy link
Contributor

kitbdev commented Oct 19, 2023

Since its a draft I wasn't sure if its ready to be tested or feedback, but it has the needs testing label, so:

Moving mouse from child to parent sends the mouse enter and mouse enter self notification to the parent, when it should only send the mouse enter self.

Mouse Filters aren't respected.

@Sauermann
Copy link
Contributor Author

Thanks for testing and the valuable feedback, I appreciate it. This is still a draft, because it has some bugs. I will mark it ready for review, when I have addressed the bugs.

scene/main/viewport.cpp Outdated Show resolved Hide resolved
scene/main/viewport.cpp Outdated Show resolved Hide resolved
@Sauermann
Copy link
Contributor Author

I have redone the PR and updated my initial naive approach with a more robust method that stores in addition to mouse_over the interval of the nodes, that currently have received a NOTIFICATION_MOUSE_ENTER without having received a NOTIFICATION_MOUSE_ENTER_SELF.
This was necessary to be able to correctly handle the cleanup when removing hovered nodes, which wasn't possible in the initial version.

I have also tested the MRP in the linked issue and also verified, that all tests pass.
Nonetheless, I believe, that this PR would benefit from thorough testing, since it changes core behavior.
I am aware, that changing top_level of hovered Control nodes is currently not handled correctly.

@Sauermann Sauermann marked this pull request as ready for review November 1, 2023 21:28
@Sauermann Sauermann requested review from a team as code owners November 1, 2023 21:28
@kitbdev
Copy link
Contributor

kitbdev commented Nov 1, 2023

It looks like mouse filters are completely ignored.
And when hiding or removing a node the parent doesn't get any exit notifications.

@kitbdev
Copy link
Contributor

kitbdev commented Nov 1, 2023

I put my take on this here if you want to check it out: kitbdev@df975d8
It's based off of your previous commit.
It currently doesn't properly handle changing the mouse filter or top level while it or it's descendant is hovered. (kitbdev@1eda4ce)
edit: updated commit.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 2, 2023

It looks like mouse filters are completely ignored.

IGNORE seems to work correctly, STOP and PASS do the same though.

And when hiding or removing a node the parent doesn't get any exit notifications.

Can't reproduce, the node correctly receives mouse exit when hidden. If shown under cursor, the entered notification is not received until mouse movement, but I assume this is expected for performance reasons.
EDIT: Nvm, exited notification is indeed broken when hovering child node.

`NOTIFICATION_MOUSE_ENTER` and `NOTIFICATION_MOUSE_EXIT` now includes
the areas of children control nodes.

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.
@WolfgangSenff
Copy link
Contributor

Just checking because I added some comments to various, seemingly-related but already-closed bugs (#17326), but would this solve an issue where I'm trying to drag-and-drop a control into a subviewport on top of another control in said SVP? Seems like it should, but I'm not as familiar with this exact part of the codebase.

@Sauermann
Copy link
Contributor Author

@WolfgangSenff Dragging into different SubViewports is currently not supported. #67531 intends to establish Drag&Drop support for differnet SubViewports. This PR should not have any influence on Drag&Drop.

@akien-mga
Copy link
Member

Superseded by #84547.

@akien-mga akien-mga closed this Nov 9, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Nov 9, 2023
@Sauermann Sauermann deleted the fix-mouse-notifications branch November 9, 2023 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants