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

Clarify the coordinate space for the bounding rectangle returned by Label.get_character_bounds #96919

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

lpenguin
Copy link
Contributor

@lpenguin lpenguin commented Sep 12, 2024

This PR intends to clarify the documentation for `Label.get_character_bounds': that the coordinate space for the result bounding rectangle of the method is local.

Closes #95088

@lpenguin lpenguin requested a review from a team as a code owner September 12, 2024 15:53
@AThousandShips AThousandShips added enhancement documentation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release and removed cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Sep 12, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 12, 2024
@@ -14,7 +14,7 @@
<return type="Rect2" />
<param index="0" name="pos" type="int" />
<description>
Returns the bounding rectangle of the character at position [param pos]. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.
Returns the bounding rectangle of the character at position [param pos]. The bounding rectangle is interpreted in local 2D coordinates. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.
Copy link
Member

Choose a reason for hiding this comment

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

Personally I dislike how "interpreted" is being used here. We know what exactly it returns, so AFAICT there's nothing to be interpreted here.

It's kinda the same wording as for the Line2D.points though (mentioned by me in #95088 (comment)). But there I consider "interpreted" acceptable as Line2D.points is a property with a setter, so it makes sense to say how the input (assigned array) will be "interpreted". In case of output/return value I think "interpreted" does not fit (not sure if it's just me though 🙃).

So I'd rather opt for something like:

Suggested change
Returns the bounding rectangle of the character at position [param pos]. The bounding rectangle is interpreted in local 2D coordinates. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.
Returns the local bounding rectangle of the character at position [param pos]. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.

or (better):

Suggested change
Returns the bounding rectangle of the character at position [param pos]. The bounding rectangle is interpreted in local 2D coordinates. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.
Returns the bounding rectangle of the character at position [param pos], in the Label's local coordinate system. If the character is a non-visual character or [param pos] is outside the valid range, an empty [Rect2] is returned. If the character is a part of a composite grapheme, the bounding rectangle of the whole grapheme is returned.

AFAICT it's better to be explicit by saying e.g. "Label's local coordinate system" instead of just "local coordinate system" etc. I think doing so should, in the long run, spread the intuition that whatever "local" is always local to some specific object. Which, from my observations, is often not so obvious among the users having problems with differentiating between coordinate systems.

But maybe it would be currently inconsistent with the rest of the docs, not sure. So cc @Mickeon for how exactly we want to word that.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say that "interpreted" is appropriate here as it's not so much about ambiguity as it is about what way to treat it, but regardless the wording is clunky to me as well and I'd say "in the ... local coordinate system" is a good wording and AFAIK one that we use elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kleonc thanks for suggestions! I would go with "Returns the bounding rectangle of the character at position [param pos] in the Label's local coordinate system." (I removed the comma).

@AThousandShips I went crazy and did quick grepping of returns.*local and looks like apart "in the ... local coordinate system" sometimes its "using the local coordinate system"

Returns the mouse's position in this [CanvasItem] using the local coordinate system of this [CanvasItem].

or "in the ... local space"

Returns position of the joint in the local space of body a of the joint.

or "the local ... position "

Returns the local mouse position adjusted for the text direction.

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

LGTM!

@akien-mga akien-mga changed the title Clarify the coordinate space for the bounding rectangle returned by Label.get_character_bounds Clarify the coordinate space for the bounding rectangle returned by Label.get_character_bounds Sep 14, 2024
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 14, 2024
doc/classes/Label.xml Outdated Show resolved Hide resolved
@akien-mga akien-mga merged commit 36496c5 into godotengine:master Sep 17, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Godot 4.3-rc2 new function Label.get_character_bounds(character pos) not working as expected
5 participants