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

Refactor mouse_entered and mouse_exited signals #67791

Merged

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Oct 23, 2022

fix variant of #20881 where Controls are in different Viewports
fix #66293
fix no longer #57349 (that is already fixed for the case of a single Viewport)
prerequisite for #67531
prerequisite for #77781
addresses #48634 after #77933
related to #54565 (maybe supersedes it)
done: don't merge before #67903 (fix display-server differences between Windows and linuxbsd)

The current implementation for signals mouse_entered and mouse_exited have shortcomings that relate to focused windows and pressed mouse buttons. For example a Control can be hovered by mouse, even if it is occluded by an embedded Window.

This patch changes the behavior, so that Control and Viewports send their mouse-enter/exit-notifications based solely on mouse position, visible area and input restrictions and not on which window has focus or which mouse buttons are pressed. This implicitly also changes when the mouse_entered and mouse_exited signals are sent.

Rewriting a core functionality like mouse_entered and mouse_exited should be tested thoroughly. While I tested it in lots of situations and had a look at the locations where it is used in the code base, it can be, that I have missed something.

So far, tests on the following platforms were successful:

  • linuxbsd
  • Windows
  • Web (Firefox, Chromium, Edge)

Other platforms:

  • macOS: not yet tested
  • Android / iOS: Not sure, if the concept of mouse_entered and mouse_exited makes sense in this context.
  • Android: Signals do not trigger with touch and drag

The concept for this implementation is:

  • When a mouse-event is sent from the DisplayServer to a Window, it first recursively determines mouse-over states for all Viewports and sends mouse-enter/exit notifications and signals.
  • Afterwards the event handling happens just as before, without the enter/exit-notifications.
  • Additional adjustments to how mouse-over-state is dropped were necessary.

MRP: BugVisualMouseNotifications.zip (updated 2022-10-26)

This functionality can not be implemented as a part of Viewport::_gui_input_event, because of its interplay with Windows and because Viewport::_gui_input_event is based on input and not on visibility.

BugVisualMouseNotification.mp4

Updated 2022-11-13: Fix location of sw->_update_mouse_over call.
Updated 2022-11-15: Fix merge conflict with #68019
Updated 2022-11-19: Fix location of sw->_update_mouse_over call.
Updated 2023-01-06: Fix merge conflict with #69972
Updated 2023-01-27: Fix merge conflict with #66692
Updated 2023-02-18: Fix merge conflict with #72764
Updated 2023-06-07: Fix merge conflict with #77933
Updated 2023-06-29: Made necessary adjustments for Gui in 3D Demo
Updated 2023-07-07: Fix merge conflict with #78078
Updated 2023-07-27: Fix merge conflict with #79248

@lmurray
Copy link

lmurray commented Oct 23, 2022

I disagree with adding new signals that differ from the existing ones as a single mouse cursor can only be in one location at a time so it cannot be both inside and outside of a region at the same time which is what is implied by having multiple signals. No other UI toolkit that I know has two separate entered/exited signals that have different rules. Instead I think the existing signals are incorrectly implemented and need to be modified to correctly detect when the cursor enters and exits a region under all circumstances.

@Sauermann
Copy link
Contributor Author

@lmurray I also would prefer to replace the old signals. However i have not yet tested the places, where the old signals are used within the code-base. So I provided new signals in this initial version of this PR, so that they can be tested and compared.

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.

Adding a new signals/notifications is probably unnecessary, it should replace existing viewport signals/methods NOTIFICATION_VP_MOUSE_* (for native windows there's NOTIFICATION_WM_MOUSE_*).

@Sauermann
Copy link
Contributor Author

Sauermann commented Oct 25, 2022

@bruvzg thanks for your insight.
I have updated the PR, so that now Viewports emit NOTIFICATION_VP_MOUSE_* notifications based on the new method.
The behavior changes, but I didn't experience any problematic effects on linuxbsd.

I left NOTIFICATION_WM_MOUSE_* alone, because this works already in the expected way.

My next step will be investigating, if replacing the Control-notifications by the new method would introduce unwanted behavior.

@Sauermann Sauermann force-pushed the fix-visual-mouse-notifications branch from d4bd57c to 49e00e2 Compare October 26, 2022 00:46
@Sauermann Sauermann changed the title Add Notifications to indicate over which Control and Viewport the Mouse visually is Refactor mouse_entered and mouse_exited signals Oct 26, 2022
@Sauermann Sauermann force-pushed the fix-visual-mouse-notifications branch from 49e00e2 to 1277eee Compare October 26, 2022 01:56
@Sauermann
Copy link
Contributor Author

Sauermann commented Oct 26, 2022

With the latest update, I have implemented a replacement for the Control-signals mouse_enter/exit. Feedback about how this works on other platforms than Linux would be appreciated.

Edit: Testing on Windows platform is currently not possible because of #67898 (fixed)

@Sauermann
Copy link
Contributor Author

Since the Windows and linuxbsd Display server behave quite differently, it makes sense to merge this PR only after the differences are solved. The PR #67903 makes this happen.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 16, 2023

I gave it a test and found a bug very quickly:
mycha_yfOC70IBoB
The cursor shape misbehaves when going from connection dialog to script editor (it appears as arrow instead of text cursor). Without this PR it consistently uses the text shape.

@Sauermann Sauermann force-pushed the fix-visual-mouse-notifications branch from 1ed7b0a to 4526a14 Compare January 17, 2023 00:58
@1xch
Copy link

1xch commented Sep 6, 2023

I just updated to v4.2.dev3.official [013e8e3] from v4.1.1 and I'm having a problem with what I suspect is this in particular.
Untitled
With the image above, the left is the window, the right is a snapshot of the node structure. G_Window & G_Pane have widgets in them for handling capture, that previously just listened to mouse enter & exit. For example:

func _on_ready():
	_home.mouse_entered.connect(func(): LXN_FOCUSED.emit(true))
	_home.mouse_exited.connect(func(): LXN_FOCUSED.emit(false))

Now they do not work at all. Can I get a quick glance at this and maybe someone say why it needs to be this way when it did work. I'd like to know if this might be a bug, and I need to provide more info or documentation or if I need to rewrite how I'm handling capture on this. I'd really rather not as it was more a pain to poll mouse events before I just settled on using mouse enter and exit as the main part of measuring capture.

@Sauermann
Copy link
Contributor Author

@1xch from your short description, it is difficult to guess, what the problem might be.
Also I'm not entirely sure, what you mean by "capture". In your code you emit a signal called "LXN_FOCUSED". Is the problem with mouse-capture or with focus?
If you could provide a minimal reproduction project of the problem, that you are experiencing, I could have a deeper look at it.

I just noticed, that you use a SubViewport, that is not a child of a SubViewportContainer. Have you implemented a way for sending InputEvents to that SubViewport?

@1xch
Copy link

1xch commented Sep 6, 2023

@Sauermann,

  1. What I am saying is that the above worked. I've had it working for a while, with continual tinkering through most 4.0 versions, the above has been working for most of this year, with recent refactors this summer.
  2. The problem is with mouse_entered & mouse_exited signals is what I've traced: they no longer happen. I would mostly ignore my terminology, I posted the above after discovery. 'Capture' here means acquiring mouse or other activity indicating the Control needs focused.
  3. Yes, I've implemented a way for InputEvents to go to the SubViewport. That is not the issue.

I think a clearer statement is that G_Window & G_Pane in the above are controls that previously responded with mouse_entered & mouse_exited signals and now they do not. Both nodes still receive InputEvents, my core issue being the signals and not knowing why they aren't triggering when they were and should to my understanding.

edit:
minimal project:

no_mouse_enter_exit.zip

OK, so the signals work in this minimal replication, but in an unexpected manner from previous expectations. Going into G_Pane from G_Window triggers exit from G_Window and enter of G_Pane, and vice versa where previous there was an overlapping. This ruins my workflow and I do not like it. That being said my project still does not have mouse enter/exit signals as above at all and I cannot figure out why, so I probably need to I don't know yet.

@Sauermann
Copy link
Contributor Author

Sauermann commented Sep 6, 2023

@1xch Thanks for the MRP. I have tested it in v4.0.stable.official [92bee43], v4.1.1.stable.official [bd6af8e] and in v4.2.dev.custom_build [fa3428f] on Linux.

Your MRP behaved identical in my tests: when moving the mouse from G_Window to G_Pane, G_Window did receive a mouse-exited signal. So I can't replicate the overlapping behavior, that you describe. Please verify, if the MRP works for you as you would expect in v4.1.1.

I have tested the behavior in 3.5.1 and also there the parent Control receives a mouse-exited signal, when the mouse moves to a child-Control.

I would also kindly ask you to make future MRP less complicated: You use multiple instantiate() calls and different source-code files, which makes the project difficult and time-consuming to understand.

@mournguard
Copy link

Has there been a discussion about this somewhere? I see the work started back in 2022. I personally don't understand why the mouse_entered and mouse_exited signals behavior was changed. Also, this seems to be the source of many issues now, as well as effectively making MOUSE_FILTER_PASS largely unusable.

Props to @Sauermann for all the work and now adding PRs to notify users in the docs, but this seems to me like it's just letting users know that events will not behave correctly. I think more discussion should definitely happen in #82530 , which sums up the situation quite well.

@emirljuca
Copy link

Yeah I gotta say, this broke my game too, I was relying on mouse pass to pass the mouse signal down while still being able to use the mouse entered and mouse exited signal. I have a control node that is further down the node stack that uses the mouse signal for a tooltip that will no longer work. Why make these changes? Is there any plans to add mouse pass work?

@kitbdev
Copy link
Contributor

kitbdev commented Nov 13, 2023

@emirljuca That issue should already be resolved in master as of #84547.

@emirljuca
Copy link

@emirljuca That issue should already be resolved in master as of #84547.

That is great to hear, testing on newest build seems to show this to be the case, 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.

Embedded window doesn't emit mouse_exited / mouse_entered.