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 rotate methods #90454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Muller-Castro
Copy link
Contributor

Updates the description of rotate, rotate_x, rotate_y, and rotate_z to clarify that the rotation is relative to the axes of the parent node.

@Muller-Castro Muller-Castro requested a review from a team as a code owner April 9, 2024 22:55
@Muller-Castro Muller-Castro changed the title Clarify rotate methods Clarify rotate methods Apr 9, 2024
@AThousandShips
Copy link
Member

This is confusing in my opinion, as it implies that a parent must exist, which is false, it's also only technically true, it doesn't care about the parent axes, it only operates on the local transform, not caring at all about the parent

@Muller-Castro
Copy link
Contributor Author

These lines

https://github.com/godotengine/godot-docs/blob/master/tutorials/3d/using_transforms.rst?plain=1#L89
https://github.com/godotengine/godot-docs/blob/master/tutorials/3d/using_transforms.rst?plain=1#L189

of the doc says that it's relative to the parent node.

rotate_object_local works the way you described, ignoring the parent transform.

@AThousandShips
Copy link
Member

AThousandShips commented Apr 10, 2024

But those are actively talking about scenes, the nodes can be rotated without a parent, this implies that a parent is needed by mentioning it, and the "local" part is meaning this

Also neither of those specify the axes of the parent, which is additionally confusing here

And no the object local means the operation happens based on the transform itself, i.e as if the axis was transformed first, not what I'm talking about

All these do is literally: transform = get_transform().rotate(...)

@Mickeon
Copy link
Contributor

Mickeon commented Apr 10, 2024

See also #87440 . Although it's still a draft, I changed those lines to something similar to:

Rotates this node's local [member transform] around the [param axis] by the given [param angle], in radians. The operation happens in local space (relative to the parent).

"Relative to the parent" should also be more understandable given the rest of the rewritten descriptions (at least that's the goal).

@AThousandShips
Copy link
Member

Your wording puts much less emphasis on the parent which to me is much better

And that PR is more comprehensive than this so should be merged instead IMO (beyond just taking precedence)

@Mickeon
Copy link
Contributor

Mickeon commented Apr 10, 2024

I think it's good that this PR has been made still. It highlights there's an actual issue with the wording and it's not just embellishing the description for the sake of it.

@AThousandShips
Copy link
Member

I'd say that clarifying what "local" means has value, but the only missing part this covers is to explain what that means, which depends on what other materials you've read before (like the tutorials)

@Muller-Castro
Copy link
Contributor Author

perhaps saying that it's relative to the object's origin may be better to describe without needing to highlight the word "parent" as explained in this tutorial: https://learnopengl.com/Getting-started/Coordinate-Systems

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.

3 participants