Remove abbreviations in Tween methods and enums

BenjaminNavarro commented 2 years ago

Describe the project you are working on

Not relevant

Describe the problem or limitation you are having in your project

Godot 4 introduced quite a lot a renaming in order to be clearer and more consistent but I feel that the tween area could benefit a renaming as well.

When looking at the tween API everything seems fine to me except for set_trans and the TransitionType enumerators which are almost all abbreviated.

I don't see why set_trans is abbreviated since Tween also has longer and more descriptive function names such as get_total_elapsed_time or interpolate_value.

As for the TransitionType enumerators, the full name of some (e.g TRANS_QUINT) would be the same length as others (e.g TRANS_ELASTIC) so I don't really see an argument for them to be abbreviated.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I suggest the following renamings:

Tween::set_trans -> Tween::set_transition
TransitionType::TRANS_QUINT -> TransitionType::TRANS_QUINTIC
TransitionType::TRANS_QUART -> TransitionType::TRANS_QUARTIC
TransitionType::TRANS_QUAD -> TransitionType::TRANS_QUADRATIC
TransitionType::TRANS_EXPO -> TransitionType::TRANS_EXPONENTIAL
TransitionType::TRANS_CIRC -> TransitionType::TRANS_CIRCULAR

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Apply the renaming to scene/animation/tween.h and scene/animation/tween.cpp and propagate the changes to the rest of the code base (if necessary) and update the docs.

I'm willing to submit a PR if this gets accepted.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No

Is there a reason why this should be core and not an add-on in the asset library?

This changes the core Tween class definition

YuriSizov commented 2 years ago

Shouldn't it then be TRANSITION_QUINTIC etc?

Mickeon commented 2 years ago

Shouldn't it then be TRANSITION_QUINTIC etc?

While I do agree with the sentiment, as I've always been while contributing to the renaming-spree, I'm not sure about that one. It'd be one major annoyance writing it out and seeing it in code, when the inferred meaning is already obvious by the time 5 letters have been written. A longer name would only mean autocompletion takes much longer to kick in appropriately.

Mickeon commented 2 years ago

The shortened names for these functions and enum constants have probably have been done this way to mitigate repetition: the full meaning of .set_trans(Tween.TRANS_EXPO) can be easily inferred by context. Imagine, for how frequently Tweens may or may not be used, writing and reading .set_transition(Tween.TRANSITION_EXPONENTIAL). And that is not to mention the method concatenation the new Tween object encourages, which would further cause more headaches than it attempts to solve.

BenjaminNavarro commented 2 years ago

Shouldn't it then be TRANSITION_QUINTIC etc?

I thought about it as well but I don't know what's best. If it was my own code it would be, in C++, Tween::Transition::Quintic but I'm not familiar enough with the Godot code base, guidelines and bindings generation to know if it's something that could be done. TweenTransition::Quintic could be an alternative that I'm ok with.

To me prefixes in enumerators are a relic of C but with C++11 enum class there's no point in that anymore. It could even be faked in C++98.

EDIT: and also if it's a change that should be done I'm pretty sure that's something that should be done on the whole code base, not just for Tweens

Imagine, for how frequently Tweens may or may not be used, writing and reading .set_transition(TRANSITION_EXPONENTIAL)

I would love to write and read tween.set_transition(TweenTransition.Exponential), I don't want to infer meanings based on context as it's error prone and can be confusing.

godotengine / godot-proposals