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

AnimatedSprite: Remove play()'s backwards, add methods to compensate #65973

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Sep 17, 2022

Closes godotengine/godot-proposals#5286 (see comments).
Closes godotengine/godot-proposals#4506.
Supersedes #41821.

A bit of a backstory:

#65148 added the ability to set speed_scale to a negative value and if it is, the animation plays in reverse. This is good. However, the original intention of the proposal was to have play()'s second parameter, "backwards", implicitly switch speed_scale's sign if necessary. Doing this would have closed the proposals above. This was not agreed upon. Instead, what ended up happening was that... basically, setting "backwards" to true and speed_scale to -1 has the animation moving forward. This is not just confusing, but defeats the original purpose of that PR, because now there is one "backwards" that is not exposed to the user.


This PR outright removes play()'s second parameter from AnimatedSprite2D and AnimatedSprite3D. This makes speed_scale > 0.0 the one consistent way to verify if an animation is playing in reverse.

To compensate, adds play_forward() and play_backwards() as shorthand methods. Both of these methods change the sign of speed_scale only when necessary, then call play(). This is like #65148 's original implementation, but split in two separate, way more explicit methods.

I should follow this up with a Proposal.

Removes `play()`'s second parameter, `backwards`. This makes `speed_scale > 0.0` the one consistent way to verify if an animation is playing in reverse.

To compensate, adds `play_forward()` and `play_backwards()` as shorthand methods. Both of these methods change the sign of `speed_scale` only when necessary, then call `play()`.
@TokageItLab
Copy link
Member

This breaks consistency with AnimationPlayer. It is theoretically correct that reverse playback in negative scale is forward playback, and I don't see this as a problem.

If you don't need the reverse playback feature, just don't use it, and this is not a reason to remove it. You can always just guarantee it as false in your project.

Moreover, at least you can get the direction of playback of the animation correct by just storing the flags on the script during playback and inverse speed_scale as needed.

@kleonc
Copy link
Member

kleonc commented Sep 17, 2022

Closes godotengine/godot-proposals#5286 (see comments).
Closes godotengine/godot-proposals#4506.

I think, given the changes/discussion from #65148, to solve these proposals the backwards property should be exposed instead (as suggested in #65148 (comment)). Whether play() should still have backwards parameter would be up to debate, for me it would be no longer needed. I don't think separate play_forward() and play_backwards() are needed.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 17, 2022

This breaks consistency with AnimationPlayer. It is theoretically correct that reverse playback in negative scale is forward playback, and I don't see this as a problem.

If we want to bring up consistency with AnimationPlayer, play_backwards() should still be added, in that case.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 17, 2022

I think, given the changes/discussion from #65148, to solve these proposals the backwards property should be exposed instead.

I wouldn't expose it as a property at this point. It seems like is_playing_backwards() may still be the solution to go for, and a very simple one, at that.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 17, 2022

If we want to bring up consistency with AnimationPlayer, play_backwards() should still be added, in that case.

This can be supported. Perhaps just implement it as a wrapper function of play() with backward = true, but it is good for consistency.

However, I would say that the problem of confusability that this PR can be easily avoided by as I said above: user restriction such as backward is always false, or with a few lines of code on GDScript. So I think that it is not worth changing the core.

@kleonc
Copy link
Member

kleonc commented Sep 17, 2022

I wouldn't expose it as a property at this point. It seems like is_playing_backwards() may still be the solution to go for, and a very simple one, at that.

I was thinking about the easiness of checking/unchecking backwards from the inspector contrary to typing in the negated speed_scale (for use cases like the one from godotengine/godot-proposals#5286). But if already allowed negative speed_scale is considered good enough for it then just adding is_playing_backwards() would be sufficient indeed.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 17, 2022

But if already allowed negative speed_scale is considered good enough for it then just adding is_playing_backwards() would be sufficient indeed.

Yeah, it was as simple as exposing the cached variable with setters and getters. This time however, unlike 2 years ago, it's more justifiable because I wouldn't want users to write (speed_scale < 0.0 !=stored_flag_i_have_created_by_overridding_play) for the sake of consistency.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 17, 2022

I wouldn't want users to write (speed_scale < 0.0 !=stored_flag_i_have_created_by_overridding_play) for the sake of consistency

The user already should know in which mode is playing. Therefore, there is no need to implement the function is_playing_backwards() in the core for a one-line branch that can be confusing, since the documentation can explain it.

e.g.)
To know if the frames of the animation are playing in the order in which they are set, the following code can be used:

var speed_scale: float = -1.0
var played_backward: bool = true

AnimatedSprite.set_speed_scale(speed_scale)
AnimatedSprite.play("AnimationName", played_backward)

if speed_scale >= 0.0 != played_backward:
    # Playing forward
else:
   # Playing backward

@TokageItLab TokageItLab modified the milestones: 4.0, 4.x Sep 17, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 1, 2022

Closing, as this makes things more complicated than they actually are. It goes against the simplicity and ease of use of AnimatedSprite.

@Mickeon Mickeon closed this Nov 1, 2022
@aaronfranke aaronfranke removed this from the 4.x milestone Feb 1, 2023
@Mickeon Mickeon deleted the animated-sprite-play-forwards-backwards branch February 11, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants