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

Expose viewport gui_find_control to scripting #86343

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdoise-vbg
Copy link

@cdoise-vbg cdoise-vbg commented Dec 19, 2023

Resubmitting relevant changes from this pull request to expose gui_find_control to GDScript. Another pull request had already been merged in that covered getting the mouse_over control
#84909

Usage:

var control_under_point = get_viewport().gui_find_control(get_global_mouse_position())
print(control_under_point)

@cdoise-vbg cdoise-vbg requested review from a team as code owners December 19, 2023 21:57
@AThousandShips AThousandShips added this to the 4.x milestone Dec 19, 2023
@AThousandShips AThousandShips changed the title Expose viewport gui_find_control to GDScript Expose viewport gui_find_control Dec 19, 2023
@AThousandShips AThousandShips changed the title Expose viewport gui_find_control Expose viewport gui_find_control to scripting Dec 19, 2023
@AThousandShips
Copy link
Member

AThousandShips commented Dec 19, 2023

The bind methods aren't just exposed to GDScript, they're also available to C# and GDExtension, please update your commit message accordingly

@Sauermann
Copy link
Contributor

Sauermann commented Dec 20, 2023

var control_under_point = get_viewport().gui_find_control(get_global_mouse_position())

This example will not work when the Canvas layer has a non-default transform, since you are mixing different coordinate systems:

  • CanvasItem.get_global_mouse_position() returns the position in canvas layer coordinates
  • Viewport.gui_find_control() accepts the position in viewport coordinates.

Please change your description to:

var control_under_point = get_viewport().gui_find_control(get_viewport().get_mouse_position())

@cdoise-vbg
Copy link
Author

cdoise-vbg commented Dec 20, 2023 via email

@cdoise-vbg
Copy link
Author

cdoise-vbg commented Dec 20, 2023

I updated the description to be more specific about which coordinates are expected based on comments above. Should I include the specific code example too in that particular comment location? (meaning: "var control_under_point = get_viewport().gui_find_control(get_viewport().get_mouse_position())").

I also need to go back and fix up the 2 commits so that they appear as 1.

UPDATE:
Commits have been squashed so they appear as 1 commit.

… called from places that can make use of the bound methods (such as GDScript, C#, and GDExtension).
<param index="0" name="point" type="Vector2" />
<description>
Returns the [Control] that would be under the mouse cursor if the mouse cursor were located at the
viewpoint coordinates provided by the point parameter. If no [Control] is at that point, returns null.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
viewpoint coordinates provided by the point parameter. If no [Control] is at that point, returns null.
viewport coordinates provided by the point parameter. If no [Control] is at that point, returns null.

Copy link
Member

Choose a reason for hiding this comment

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

This should also be a single line, the current text is two separate paragraphs

Updating documentation wording for gui_find_control to be clearer and to remove extra whitespace.

Co-authored-by: A Thousand Ships <[email protected]>
@cdoise-vbg
Copy link
Author

Thank you @AThousandShips for the rewording of the documentation. I've committed that from the button to accept suggested changes in the comments above. Should I now go back and squash those two commits back down to one?

@name212
Copy link

name212 commented May 23, 2024

@AThousandShips will this pull reqest be merged?

@AThousandShips
Copy link
Member

It hasn't been approved yet, only once approved will it be considered for merger, we're currently in feature freeze for 4.3 so this can't be merged until after 4.3 has been released, but again after approval

@cdoise-vbg
Copy link
Author

Do I need to do anything in particular to request approval or further comments for the latest check-in from December 31?

@AThousandShips
Copy link
Member

Not that I know, but once 4.3 has been released you can write in the developer chat and see if anyone can take a look, feel free to join already to keep up to date with development

@badsectoracula
Copy link
Contributor

I fixed the conflict and did a quick test with an editor script and this seemed to work fine and as expected.

You may want to squash the commits again to 1 and fix the conflict.

@Sauermann
Copy link
Contributor

Please note, that Viewport::gui_find_control doesn't take embedded windows into account.
So the caller of this function would need to make sure, that there is no embedded window at the specified position.
Otherwise the function could return a Control-node, that is hidden behind an embedded window.

So I'm not sure, if exposing this function as it is, is a good idea, because people could get unexpected results, if they are unaware of these details. This implementation detail also caused problems inside the engine, which was recently discovered: see #99849

Would the following idea be better?
Expose a function Node *Viewport::gui_find_node_at_position(const Point2 p_position), which returns

  • the embedded window at p_position or otherwise
  • the Control node at p_position or
  • nullptr, if neither of the above conditions fit.

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.

5 participants