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 Tree Mouse hover position #102842

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Feb 14, 2025

This PR should fixes the hovered, tooltip and click position for trees button.

The PR includes:

  • A simplification of the _determine_hovered_item to use the _find_button_at_pos method which tried to do exactly the same thing, aka finding the button at a position.
  • The space used by the scrollbar was not correctly calculated. At first, I tried to simply add the missing scrollbar_h_separation but there's some calculations in _get_content_rect that manage a situation where the scrollbar is inside the panel margin (when theme scrollbar_margin_right is >= 0). I simply used _get_content_rect directly like a lot of other places in the tree code. This fix was added in _range_click_timeout, gui_input, and _find_button_at_pos.
  • The condition to check if the mouse position was hover a button was always off 2 pixels to the right because of a missing >= and a -1 needed because is a comparaison between a zero based position and a size. This fix was added in _find_button_at_pos and propagate_mouse_event.
  • The margin area of the scrollbar was considered the last column and button.
  • In RTL mode, the panel offset was calculated before the adjustment to pos.x in _find_button_at_pos causing an offset of a couple of pixels in RTL mode.
  • Finally, I added -1 when adjusting the position in RTL mode in _find_button_at_pos to have the exact position. Ex: If the position is 99,0 and the size is 100,0 the actual position to use is 0,0. If the position is 0,0, the actual position to use is 99,0. I changed every place in the Tree code where these adjustments were present.

I tested in Window with the MRP from the issue and with different settings. I'm not familiar with the Tree control so I strongly suggest more test are done.

Note: There still some duplicate code between _find_button_at_pos and propagate_mouse_event but everything seems to work fine as is, I'm really not confident I can refactor this without any side effect, especially just before a new release.

godot.windows.editor.dev.x86_64_3rOzMy9UvI.mp4

@Hilderin Hilderin requested a review from a team as a code owner February 14, 2025 04:32
@Hilderin Hilderin added this to the 4.4 milestone Feb 14, 2025
@Hilderin Hilderin force-pushed the fix-tree-mouse-hover branch 2 times, most recently from a638f3e to 73051b0 Compare February 14, 2025 04:45
@akien-mga akien-mga requested review from Calinou and KoBeWi February 14, 2025 07:01
@Hilderin
Copy link
Contributor Author

It seems I missed little detail, in the editor Scene Tree, it's still off a couple pixels... I'll debug that...

@Hilderin Hilderin force-pushed the fix-tree-mouse-hover branch 2 times, most recently from 198b143 to 5cce311 Compare February 14, 2025 15:18
@Hilderin
Copy link
Contributor Author

Hilderin commented Feb 14, 2025

I fixed the issue by reusing the _get_content_rect instead of trying to recalculate the correct scrollbar size.

But, I found out an other issue that the last button is considered hovered and is clicable from the margin around the scrollbar:

godot.windows.editor.dev.x86_64_r9luWwhb0f.mp4

I tested in Godot 4.3 stable and turns out that this behaviour existed in the version (minus the hover which did not exist in 4.3):

Godot_v4.3-stable_win64_HidwuJOb22.mp4

I fixed it anyway by adding a check for the position vs the content area (x_limit) in _find_button_at_pos and propagate_mouse_event.

Now:

godot.windows.editor.dev.x86_64_ZEPYRrybhU.mp4

@KoBeWi
Copy link
Member

KoBeWi commented Feb 14, 2025

The original bug is still there:

godot.windows.editor.dev.x86_64_9tO5cv0WFE.mp4

Different buttons are hovered and clicked.

@Hilderin Hilderin force-pushed the fix-tree-mouse-hover branch 2 times, most recently from 40b77dd to ecc4af4 Compare February 15, 2025 22:15
@Hilderin
Copy link
Contributor Author

Indeed, there's was still some difference between propagate_mouse_event and _find_button_at_pos when the button were under the scrollbar or when the tree was scrolled to the right.

To prevent any difference between the two, I moved the commun parts in a new method called _find_column_and_button_at_pos. This method returns a new struct FindColumnButtonResult which contains all the info needed in propagate_mouse_event to work as before.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Works correctly now.

@Hilderin Hilderin force-pushed the fix-tree-mouse-hover branch from ecc4af4 to 26cbaca Compare February 16, 2025 13:56
@akien-mga akien-mga merged commit 93d2706 into godotengine:master Feb 17, 2025
20 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.

Tree button click detection is off
3 participants