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

Allow negative speed_scale in AnimatedSprite2D & 3D #65148

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 31, 2022

This PR unlocks AnimatedSprite2D and AnimatedSprite3D's speed_scale, allowing it to go below 0.
If the speed_scale is set to a negative value, the animation plays backwards.
The second parameter of play(), backwards, works as before. If set to true, and speed_scale is negative, the animation plays forward. As such, this is barely if not a compatibility-breaking change, unless users were relying on the previous clamping.

Showcase

It also slightly tweaks the internal code, as a follow-up from #64155.
I may do something more drastic later on, because the internals of this class look somewhat archaic, and there actually are some bugs and odd inconsistencies only some users like me may notice.

May need some testing regarding signal emissions.

Note: Having a method returning (speed_scale < 0.0) may still be beneficial for less math-leaning users and for readability purposes, but I'm not sure. Perhaps too situational.

The second parameter of play(), backwards, now reverses the current speed_scale when necessary.
This is no longer the case. The speed_scale Because of it, the true "direction" still cannot be known.
Closes godotengine/godot-proposals#5286 (see comments).
Closes godotengine/godot-proposals#4506.
Supersedes #41821.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 7, 2022

@Calinou Upon reevaluation, I have to unfortunately say that this is at least slightly compatibility breaking, even if welcome. Thinking of situations where the scale of an animation's speed changes dynamically (e.g. running animation dependent on speed), where it is constantly set to a certain positive value, which means that unlike the previous behaviour, if the animation is by chance also reversed, it would snap back to not being so upon setting the property.

Edit: accidentally closed.

@Mickeon Mickeon closed this Sep 7, 2022
@Mickeon Mickeon reopened this Sep 7, 2022
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I don't know if it should finish when animation reach first frame with speed_scale is negative in the case where there is no loop and backward == false.
FYI, in AnimationTree it will not be finish, but in AnimationPlayer it probably will be finish.

scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
@dalexeev
Copy link
Member

@Mickeon Sorry, I inadvertently implemented the same changes as you in a larger PR. I haven't looked at your PRs. For example, I fixed the processing logic differently (and more compactly).

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 10, 2022

It doesn't necessarily supersede it, but because both this and the mentioned PR are contested changes, it would be better to implement one thing at the time, then rebase if necessary.

@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch 2 times, most recently from d55afd5 to 9918baf Compare September 12, 2022 12:33
@Mickeon Mickeon requested a review from a team as a code owner September 12, 2022 12:33
@Mickeon Mickeon changed the title Allow negative speed_scale in AnimatedSprite2D & refactoring Allow negative speed_scale in AnimatedSprite2D & 3D Sep 12, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 12, 2022

This PR has been rebased and it now applies to AnimatedSprite3D as well, now that it can, since #64155.

@kleonc
Copy link
Member

kleonc commented Sep 12, 2022

I don't know if it should finish when animation reach first frame with speed_scale is negative in the case where there is no loop and backward == false.

@TokageItLab I think this implementation doesn't allow such situation, speed_scale < 0 if and only if backwards == true. It's ensured in the set_speed_scale/play methods.

@dalexeev
Copy link
Member

Sorry if I sound rude or arrogant, but in #65609 these issues are already resolved, these PRs overlap quite a lot. Things that are in this PR but not in #65609 are general edits like !frames.is_valid() -> frames.is_null(). I'm not opposed to this PR being merged, it just doesn't seem to make much sense to me. My patch size won't decrease much even if this PR gets merged.

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.

Checked the code, LGTM (and I do like the feature).

scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
@kleonc
Copy link
Member

kleonc commented Sep 12, 2022

Sorry if I sound rude or arrogant, but in #65609 these issues are already resolved, these PRs overlap quite a lot. Things that are in this PR but not in #65609 are general edits like !frames.is_valid() -> frames.is_null(). I'm not opposed to this PR being merged, it just doesn't seem to make much sense to me. My patch size won't decrease much even if this PR gets merged.

@dalexeev Personally I prefer few separate small PRs over a single all-in-one PR. It's just simpler to review such changes / harder to miss something. It's not like the features in your PR rely on each other, they can be introduced separately. I'm not suggesting it just for the sake of separating things. One feature might be welcomed but the other might be unwanted for whatever reason. Separating things makes it easier to discuss/approve specific parts (in some cases it might actually fasten the whole process).
And of course part of your PR is already made in here so it's kinda a partially duplicate. But I do like the "Add support for individual frame duration to SpriteFrames." part, just haven't looked at it properly yet.

@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 12, 2022

At this point do you think it would be a bad idea to remove the second parameter of play(), add play_backwards() and have both do and document the sign inversion on speed_scale when necessary?

I mean, see if something else is off in this PR, then it can be merged and I can see later on. A Proposal would be very appropriate for this discussion.

scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from 5de67e7 to baa3e61 Compare September 12, 2022 17:48
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from baa3e61 to e98240e Compare September 12, 2022 18:36
scene/3d/sprite_3d.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from e98240e to 34f6561 Compare September 13, 2022 08:41
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.

Mainly typos.

doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite3D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite3D.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from 34f6561 to 0e11850 Compare September 13, 2022 09:44
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from 0e11850 to 4dd34df Compare September 13, 2022 10:20
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 13, 2022

Removed every "frame check" early return during iteration because frame is always clamped appropriately to at least be above 0.

scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from 4dd34df to 58ee128 Compare September 13, 2022 10:36
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 13, 2022

I know I am the only person on Earth interested on this Node, and I do really really love to improve on it, but it'd be a good idea to merge this one and seeing how to go from there. I implied it before, but I would like to make a bunch of PRs about it

If the `speed_scale` is set to a negative value, the animation plays in reverse.
The second parameter of `play()` still reverses as before. if `speed_scale` and the second parameter of `play()` is true, the animation plays forward.

Also updates the documentation to better describe the pausing and playing behaviour.
@Mickeon Mickeon force-pushed the animated-sprite-negative-speed-scale branch from 58ee128 to 8142bc4 Compare September 13, 2022 10:41
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for nice job!

@akien-mga akien-mga merged commit 16d4439 into godotengine:master Sep 16, 2022
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants