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

Use float instead of integers for durations in Tween documentation #102523

Merged
merged 1 commit into from
Mar 11, 2025

Conversation

timkrief
Copy link
Contributor

@timkrief timkrief commented Feb 7, 2025

I think it could be cleaner to use floats instead of integers, since duration is supposed to be floating numbers. In this case it doesn't make a big difference, but if you use integers instead of floats as target value in tweens, it can create unexpected results, so I think it could be cleaner to use floats when floats are expected, at least in the documentation itself.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@timkrief timkrief requested a review from a team as a code owner February 7, 2025 10:04
@AThousandShips AThousandShips requested a review from a team February 7, 2025 10:07
@AThousandShips AThousandShips added this to the 4.x milestone Feb 7, 2025
@AThousandShips AThousandShips changed the title Use float instead of integers for durations Use float instead of integers for durations in Twee documentation Feb 7, 2025
@fire fire changed the title Use float instead of integers for durations in Twee documentation Use float instead of integers for durations in Tween documentation Feb 7, 2025
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me.

In general, I prefer docs (and sample code) having explicit float literals whenever the value being set is a float.

@akien-mga akien-mga modified the milestones: 4.x, 4.5 Feb 11, 2025
@akien-mga
Copy link
Member

As this will invalidate translations, let's merge for 4.5-dev1, so translators have enough time to handle the fuzzy strings before 4.5-stable.

@@ -402,7 +402,7 @@
[codeblocks]
[gdscript]
var tween = create_tween()
tween.tween_method(look_at.bind(Vector3.UP), Vector3(-1, 0, -1), Vector3(1, 0, -1), 1) # The look_at() method takes up vector as second argument.
tween.tween_method(look_at.bind(Vector3.UP), Vector3(-1, 0, -1), Vector3(1, 0, -1), 1.0) # The look_at() method takes up vector as second argument.
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
tween.tween_method(look_at.bind(Vector3.UP), Vector3(-1, 0, -1), Vector3(1, 0, -1), 1.0) # The look_at() method takes up vector as second argument.
tween.tween_method(look_at.bind(Vector3.UP), Vector3(-1.0, 0.0, -1.0), Vector3(1.0, 0.0, -1.0), 1.0) # The look_at() method takes up vector as second argument.

If we are trying to use explicit float literals, why are we not using them for the Vector2/Vector3 constructors too, for consistency?

I don't necessarily think we should do so. But if we choose to keep the inconsistency we should do so intentionally and have some reason even if it's just "it's longer and doesn't increase clarity"

Copy link
Contributor

Choose a reason for hiding this comment

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

We can handle this in a separate PR/discussion, no need to keep this one on ice in the meantime

@Repiteo Repiteo merged commit 66dc909 into godotengine:master Mar 11, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Mar 11, 2025

Thanks! Congratulations on your first merged contribution! 🎉

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.

None yet

6 participants