godot
godot copied to clipboard
godotengine
Prevent `PhysicsDirectSpaceState3D::instersect_ray` when mouse is over non-ignorable gui
Overview
This PR is intended to fix https://github.com/godotengine/godot/issues/87322, where a non-ignorable gui element was not blocking a CollisionShape3D's _mouse_enter from being emitted.
Issue
The cause of this bug is that _drop_physics_mouseover (in viewport.cpp) would get called when the mouse would move over gui, and that would cause a _mouse_exit to occur, and it would also result in the following if statement condition to be true: new_collider != physics_object_over because the physics_object_over would be null. That shouldn't be an issue, except that it is possible for _process_picking to be invoked on the same exact frame as the previous _drop_physics_mouseover.
I'm not 100% certain about the last sentence above, but that was my best understanding based on my debugging. I look forward to hearing from others that understand the engine better than I do.
Fix
I think the fix is to only check for 3D objects in process_picking if !gui.mouse_over. This does not break the Stop, Ignore, and Pass settings (as far as I can tell), but I have only tested on the minimum example project attached to this PR.
Minimum Fix Project
minimal_mouse_hove_example.zip
Screen Recording of Fix
https://github.com/godotengine/godot/assets/8028662/1376f503-4ac5-4221-86e1-830cd44fbc28
When the mouse hovers over the 3D shape (it looks 2D because there is no lighting), the shape scales up use _mouse_enter. When the mouse hovers off the 3D shape it scales down to its original scale. I can explain the logs further if people have questions, but I want to keep this description short so I will hold off on that.
@Sauermann I am moving this out of draft mode. My latest commit fixes the bug I reported, and the issue you found. Please let me know what you think.
Latest Fix
2c6ba092265c9a1b0f5d7adac9dcaa575e33aed7 Fixes the issues by blocking PhysicsDirectSpaceState3D::instersect_ray when the mouse is over a Control with mouse filter set to MOUSE_FILTER_STOP.
This commit also prevents an unneeded _mouse_exit from occurring when initially entering a Control with mouse a filter set to MOUSE_FILTER_PASS (by only allowing _drop_physics_mouseover to be called when gui.mouse_over's filter is MOUSE_FILTER_STOP). The unneeded _mouse_exit was triggering an extra _mouse_enter -- similar to the original bug.
Latest MRP
minimal_mouse_hover_example_02.zip (no .godot folder)
Screen Recording
https://github.com/godotengine/godot/assets/8028662/3224c5e2-3b6e-4de5-ab2a-c8a2e7203df2
The screen recording shows that there are no extra calls to the 3D collision object's mouse enter/exit, and the PASS-Control does not prevent print statements from being executed when mouse is moved over it.
Question
I noticed that the print statement for mouse movement still get printed even over the section of the PASS-Control that overlaps the STOP-Control (~9s into the screen recording). That might be another bug, but if it is, I think it is separate from this PR, and might be less of a big deal than the current bug since devs can make easier workarounds for it.
My question is: does the engine keep track of every Control hierarchy that it is over, or does it only keep track of the top-most Control hierarchy? I couldn't find anything like "gui.mouse_over_all_hierarchies" so I am assuming it only keeps track of the top-most.
related:
- https://github.com/godotengine/godot/issues/54529
There is an issue when there are multiple Controls in the mouse over hierarchy and the top one is STOP. In this Gif:
- ta0 is the red one and has mouse filter STOP
- ta1 is the child of ta0 and has mouse filter PASS
When entering the PASS Control from the Area2D, it keeps mouse over and does not drop it when the mouse is no longer overlapping it. When entering the Area2D from the PASS Control, it does not receive mouse over at all. Area2D and Area3D are the same here.
does the engine keep track of every Control hierarchy that it is over, or does it only keep track of the top-most Control hierarchy?
Yes, it only keeps track of the the top most since it is intended that the mouse can only be over one chain of Control nodes at a time.
The mouse filter MOUSE_FILTER_PASS actually sends events up to its parent with event bubbling, it is not intended to let events behind it. See these issues for more about it:
- https://github.com/godotengine/godot-proposals/issues/3613
- https://github.com/godotengine/godot/issues/55432
- https://github.com/godotengine/godot-proposals/issues/7167
- https://github.com/godotengine/godot-proposals/issues/1775
Since PASS sends events up and not behind, I feel that PASS and STOP should work the same in this case and not let mouse events through to the physics picking at all, but others have different opinions about it (#54529).
@kitbdev I'm not sure I completely understand your comment. I see the issue, but are you suggesting that this PR is the cause of the issue you linked? Is there something I should change in my PR or are you just pointing out that there is a separate, but possibly related, issue?
The linked issues are only related.
The issue in the Gif is a result of this PR, since you changed the behavior (_drop_physics_mouseover() affects 2d and 3d)
Not that the original behavior was completely functional, since the original issue you fixed also exists in 2D as well.
@kitbdev can you share the MRP of the gif you shared? I see the issue in your gif, but I'd like to be sure I test against the same setup.
I think I know the issue -- my changes don't take into account the gui.mouse_over_hierarchy. I will take another look.
@kitbdev https://github.com/godotengine/godot/pull/87362/commits/d6efa634ee4fbf17bd149f8ad479120369a5c2c9 fixes the issue exposed by your MRP, and fixes the issue exposed in my MRP.
I did not expect it, but on master a PASS-Control that is a child of a STOP-Control must result in an invocation of _drop_physics_mouseover even if the portion of the PASS-Control that the mouse is over is not overlapping the STOP-Control. This commit keeps that behavior by checking if any ancestor of the PASS-Control is a STOP-Control.
Hi @Sauermann, this is my first PR for Godot and just want to know if I need to do anything on my end to make sure this gets reviewed in time for the the milestone deadline. Is there anything else I need to do in particular to get the reviewed?
A large part of the reviews are done by volunteers in their spare time. So it depends on their availability.
This PR is not a trivial change and will require some effort to review and test it in depth, because PRs that touch a core-class can easily introduces unintended consequences.
