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

Make WithSpriteBody always use Animation.PlayRepeating for looped playback #15600

Merged
merged 1 commit into from Sep 24, 2018

Conversation

Projects
None yet
2 participants
@reaperrr
Copy link
Contributor

reaperrr commented Sep 10, 2018

PlayCustomAnimationRepeating looping back into itself via Action instead of simply using Animation.PlayRepeating is weird enough by itself.
It also can cause the animation to slowly go out of sync with animations using Animation.PlayRepeating under certain circumstances (happens in at least my own downstream mod).

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 10, 2018

Do you have a specific test case (in mind)?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 22, 2018

Can't really come up with a test-case on bleed, I tried using the cooling towers of the cnc powerplants but there wasn't any difference (i.e. they played at exact same speed with both approaches).

All I can say at this point is that in my MechCommander mod I use WithMovingSpriteTurret/-Barrel traits for torso and arms, respectively, which use Animation.PlayRepeating.
With bleed's WSB/WithMoveAnimation, the leg walk anim goes slowly out of sync with the above anims when the mech is moving.
When using Animation.PlayRepeating like on this PR, they stay perfectly in sync.

I'd also like to mention that CancelCustomAnimation as well as the default idle sequence started from ctor (unless WSB has a StartSequence, but I'm about to add a fix for that, too) already use Animation.PlayRepeating without issues.

This might actually be an issue exclusive to my mod, since I'm using stand-to-move transitions there.

In any case, this at least makes the code more consistent and in the unlikely case there's anything wrong with Animation.PlayRepeating (which I doubt, but you never know), we should rather fix that than continue to let WSB do its own thing for looping custom anims.

@reaperrr reaperrr force-pushed the reaperrr:fix-wsb-custom-repeat branch from 4955917 to f3c4f8a Sep 22, 2018

@reaperrr reaperrr changed the title Fix WithSpriteBody looped custom animation playback Make WithSpriteBody always use Animation.PlayRepeating for looped playback Sep 22, 2018

@@ -104,7 +104,7 @@ public void PlayCustomAnimation(Actor self, string name, Action after = null)
public void PlayCustomAnimationRepeating(Actor self, string name)
{
var sequence = NormalizeSequence(self, name);
DefaultAnimation.PlayThen(sequence, () => PlayCustomAnimationRepeating(self, sequence));
DefaultAnimation.PlayRepeating(NormalizeSequence(self, sequence));

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 22, 2018

Member

Looks like you are normalizing the sequence twice.

This comment has been minimized.

@reaperrr

reaperrr Sep 22, 2018

Contributor

Ugh, good catch.

@@ -70,7 +70,7 @@ protected WithSpriteBody(ActorInitializer init, WithSpriteBodyInfo info, Func<in

if (info.StartSequence != null)
PlayCustomAnimation(init.Self, info.StartSequence,
() => PlayCustomAnimationRepeating(init.Self, info.Sequence));
() => DefaultAnimation.PlayRepeating(NormalizeSequence(init.Self, info.Sequence)));

This comment has been minimized.

@abcdefg30

abcdefg30 Sep 22, 2018

Member

What is wrong with not changing this? (It should work already after you changed PlayCustomAnimationRepeating. Not that this is a blocker, just curious.)

This comment has been minimized.

@reaperrr

reaperrr Sep 22, 2018

Contributor

Mostly for consistency with the 'else' below, and semantics (the default sequence is not a custom animation, afterall).

@reaperrr reaperrr force-pushed the reaperrr:fix-wsb-custom-repeat branch from f3c4f8a to 1b58da5 Sep 22, 2018

Make WithSpriteBody always use Animation.PlayRepeating
... for looped sequences.

PlayCustomAnimationRepeating looping back into itself via Action instead of simply using Animation.PlayRepeating is weird, and in fact causes a slight 'desync' in animation speed with Animation.PlayRepeating in at least one downstream mod.

@reaperrr reaperrr force-pushed the reaperrr:fix-wsb-custom-repeat branch from 1b58da5 to 6fa1dd8 Sep 22, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Sep 22, 2018

Updated.

@abcdefg30 abcdefg30 merged commit 1098804 into OpenRA:bleed Sep 24, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Sep 24, 2018

@reaperrr reaperrr deleted the reaperrr:fix-wsb-custom-repeat branch Oct 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment