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 and -Turret always use CancelCustomAnimation() after custom anim #13745

Merged
merged 3 commits into from Aug 23, 2017

Conversation

Projects
None yet
4 participants
@reaperrr
Contributor

reaperrr commented Jul 28, 2017

On bleed, custom animations that don't queue CancelCustomAnimation() will break the default sequence loop.

Closes #13742.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Jul 28, 2017

Member

What uses PlayCustomAnimation right now? Is there any reason to not make this work intuitively by default now?

Member

pchote commented Jul 28, 2017

What uses PlayCustomAnimation right now? Is there any reason to not make this work intuitively by default now?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 28, 2017

Contributor

What uses PlayCustomAnimation right now? Is there any reason to not make this work intuitively by default now?

Turns out the only case that doesn't CancelCustomAnimation() is (surprise, surprise...) WithHarvestAnimation, probably to avoid the default sequence overriding the fullness-dependant sequence.

That should be fixable, though, if that's even an issue in practice.

Edit: Isn't even an issue in practice, RA harvester fullness still works fine even when using CancelCustomAnimation().

Contributor

reaperrr commented Jul 28, 2017

What uses PlayCustomAnimation right now? Is there any reason to not make this work intuitively by default now?

Turns out the only case that doesn't CancelCustomAnimation() is (surprise, surprise...) WithHarvestAnimation, probably to avoid the default sequence overriding the fullness-dependant sequence.

That should be fixable, though, if that's even an issue in practice.

Edit: Isn't even an issue in practice, RA harvester fullness still works fine even when using CancelCustomAnimation().

reaperrr added some commits Jul 28, 2017

Resume looping default WithSpriteTurret.Sequence after custom anim
It doesn't really make sense not to, and the only trait using PlayCustomAnimation previously did this manually anyway.
WithHarvestAnimation style fixes
Make interface implementations explicit.

@reaperrr reaperrr changed the title from Fix WithMakeAnimation to Make WithSpriteBody and -Turret always use CancelCustomAnimation() after custom anim Jul 28, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 28, 2017

Contributor

Updated.

Requesting @forcecore to test this with #13737 (without the yaml work-around).

Contributor

reaperrr commented Jul 28, 2017

Updated.

Requesting @forcecore to test this with #13737 (without the yaml work-around).

@forcecore

This comment has been minimized.

Show comment
Hide comment
@forcecore

forcecore Jul 28, 2017

Contributor

Thumper fix works without the walk around on this branch and idle animations loops OK.

Contributor

forcecore commented Jul 28, 2017

Thumper fix works without the walk around on this branch and idle animations loops OK.

@reaperrr reaperrr added this to the Next + 1 milestone Aug 17, 2017

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 17, 2017

Member

Turns out the only case that doesn't CancelCustomAnimation() is (surprise, surprise...) WithHarvestAnimation, probably to avoid the default sequence overriding the fullness-dependant sequence.

Are you sure about this? I would have assumed it was to let the harvesting animation loop if the animation length is less than the harvest cycle length. If that's the case then this would be a good time/place to introduce a PlayCustomAnimationRepeating method.

Member

pchote commented Aug 17, 2017

Turns out the only case that doesn't CancelCustomAnimation() is (surprise, surprise...) WithHarvestAnimation, probably to avoid the default sequence overriding the fullness-dependant sequence.

Are you sure about this? I would have assumed it was to let the harvesting animation loop if the animation length is less than the harvest cycle length. If that's the case then this would be a good time/place to introduce a PlayCustomAnimationRepeating method.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 17, 2017

Member

I would have assumed it was to let the harvesting animation loop if the animation length is less than the harvest cycle length.

Either my assumption was wrong, or this was already broken on release/playtest/bleed.
This can be easily confirmed by adding BaleLoadDelay: 100 to the Harvester trait.

Member

pchote commented Aug 17, 2017

I would have assumed it was to let the harvesting animation loop if the animation length is less than the harvest cycle length.

Either my assumption was wrong, or this was already broken on release/playtest/bleed.
This can be easily confirmed by adding BaleLoadDelay: 100 to the Harvester trait.

@pchote

pchote approved these changes Aug 17, 2017

Code changes look good to me, and no regressions spotted ingame.

@pchote pchote added the PR: Needs +2 label Aug 17, 2017

@abcdefg30 abcdefg30 merged commit 120d7f9 into OpenRA:bleed Aug 23, 2017

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.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Aug 23, 2017

@Mailaender Mailaender referenced this pull request Aug 24, 2017

Open

Added GI sandbag deployment #394

1 of 3 tasks complete

@reaperrr reaperrr deleted the reaperrr:fix-makeanim branch Mar 9, 2018

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