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

Refactor WithAttackAnimation to support multiple sprite bodies #13350

Merged
merged 4 commits into from Jun 13, 2017

Conversation

Projects
None yet
5 participants
@reaperrr
Contributor

reaperrr commented May 24, 2017

Same approach as with WithMoveAnimation.

Additionally, reduced string.NullOrEmpty look-ups and made sure the aim/reload sequences don't override a playing attack anim.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 24, 2017

Contributor

Updated.

Contributor

reaperrr commented May 24, 2017

Updated.

@pchote

Looks good otherwise, and works as advertised. I'll hold my 👍 until we come to a decision about how to handle multiple bodies.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 26, 2017

Contributor

Updated. Travis doesn't seem to trigger on force-pushes anymore, but it passed make all and make check locally.

Contributor

reaperrr commented May 26, 2017

Updated. Travis doesn't seem to trigger on force-pushes anymore, but it passed make all and make check locally.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote May 26, 2017

Member

This seems like a more reasonable approach, but IMO we should go further: require all WithAttackAnimation traits (and then as a second step all With*Animation) to define the WithSpriteBody that they act on, and then throw a YamlException from RulesetLoaded if there isn't exactly one matching trait.

Member

pchote commented May 26, 2017

This seems like a more reasonable approach, but IMO we should go further: require all WithAttackAnimation traits (and then as a second step all With*Animation) to define the WithSpriteBody that they act on, and then throw a YamlException from RulesetLoaded if there isn't exactly one matching trait.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 26, 2017

Contributor

require all WithAttackAnimation traits (and then as a second step all With*Animation) to define the WithSpriteBody that they act on, and then throw a YamlException from RulesetLoaded if there isn't exactly one matching trait.

Updated.

Contributor

reaperrr commented May 26, 2017

require all WithAttackAnimation traits (and then as a second step all With*Animation) to define the WithSpriteBody that they act on, and then throw a YamlException from RulesetLoaded if there isn't exactly one matching trait.

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 28, 2017

Contributor

Updated.

Contributor

reaperrr commented May 28, 2017

Updated.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 28, 2017

Contributor

Updated, replaced .Single with .First in the armament look-up.

Even ignoring performance, it is entirely possible that mods might want to 'share' an attack anim between two armaments (for example, dual-barreled units might need two armaments to fire from both barrels at the same time, imagine TS Wolverine firing visible projectiles), so being forced to give one of the armaments a different name just to avoid a crash is a bit inconvenient.

Contributor

reaperrr commented May 28, 2017

Updated, replaced .Single with .First in the armament look-up.

Even ignoring performance, it is entirely possible that mods might want to 'share' an attack anim between two armaments (for example, dual-barreled units might need two armaments to fire from both barrels at the same time, imagine TS Wolverine firing visible projectiles), so being forced to give one of the armaments a different name just to avoid a crash is a bit inconvenient.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v May 28, 2017

Contributor

Found issues updated, looks good, but not tested (not clear for me how to test it ingame), enough reviewers, so only small 👍 from me.

Contributor

rob-v commented May 28, 2017

Found issues updated, looks good, but not tested (not clear for me how to test it ingame), enough reviewers, so only small 👍 from me.

reaperrr added some commits May 26, 2017

Refactor WithAttackAnimation
- made trait compatible with actors that have more than one sprite body or enable/disable sprite bodies via conditions
- added check for running attack anim and prevent aim/reload sequences from overriding it
- added caching of whether trait has either aim or reload sequence, to avoid some string.IsNullOrEmpty look-ups every tick
Throw yaml exception if WithAttackAnimation has no assigned sprite body
...or too many assigned bodies.

Also, further simplify WithAttackAnimation code.
Rename WithAttackAnimation.BodyName to just .Body
Shorter and more consistent with Armament.Turret, WithTurretedAttackAnimation.Turret, WithSpriteBarrel.Armament etc.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 7, 2017

Contributor

Rebased and updated. Dropped the Single->First commit again (see discussion on #13378).

@rob-v If you add

WithFacingSpriteBody@2:
	Name: body2

to RA V2RL or TS SMECH, they'll crash on bleed. With this PR, they'll show both bodies, but only the default one will be modified by the attack anim trait.

Additionally, you can test for regressions just by checking if V2RL aim/reload anim and TS Wolverine attack anim still work.

The conditional part could by tested by making WithAttackAnimation on one of the above actors require rank-veteran > 1, then the anim would only play when the unit ranks up by at least 1 level.

Contributor

reaperrr commented Jun 7, 2017

Rebased and updated. Dropped the Single->First commit again (see discussion on #13378).

@rob-v If you add

WithFacingSpriteBody@2:
	Name: body2

to RA V2RL or TS SMECH, they'll crash on bleed. With this PR, they'll show both bodies, but only the default one will be modified by the attack anim trait.

Additionally, you can test for regressions just by checking if V2RL aim/reload anim and TS Wolverine attack anim still work.

The conditional part could by tested by making WithAttackAnimation on one of the above actors require rank-veteran > 1, then the anim would only play when the unit ranks up by at least 1 level.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 12, 2017

Contributor

This is probably not related to this PR, when I add WithFacingSpriteBody to smech as you proposed, it fails with this error: 'Unit smech does not have a sequence named idle'

Contributor

rob-v commented Jun 12, 2017

This is probably not related to this PR, when I add WithFacingSpriteBody to smech as you proposed, it fails with this error: 'Unit smech does not have a sequence named idle'

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jun 12, 2017

Contributor

This is probably not related to this PR, when I add WithFacingSpriteBody to smech as you proposed, it fails with this error: 'Unit smech does not have a sequence named idle'

Ah yes, the default value for Sequence in sprite bodies is idle, which smech doesn't have. So for my example to work, you'd have to set Sequence: stand on that additional sprite body.

Contributor

reaperrr commented Jun 12, 2017

This is probably not related to this PR, when I add WithFacingSpriteBody to smech as you proposed, it fails with this error: 'Unit smech does not have a sequence named idle'

Ah yes, the default value for Sequence in sprite bodies is idle, which smech doesn't have. So for my example to work, you'd have to set Sequence: stand on that additional sprite body.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Jun 12, 2017

Contributor

note - this throw new YamlException("WithAttackAnimation needs exactly one sprite body with matching name."); and other exceptions to be useful depends on this bug fix: #13372, otherwise these exceptions are not logged.

Contributor

rob-v commented Jun 12, 2017

note - this throw new YamlException("WithAttackAnimation needs exactly one sprite body with matching name."); and other exceptions to be useful depends on this bug fix: #13372, otherwise these exceptions are not logged.

@rob-v

rob-v approved these changes Jun 12, 2017

👍

@atlimit8 atlimit8 merged commit d52313a into OpenRA:bleed Jun 13, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@atlimit8
Member

atlimit8 commented Jun 13, 2017

@reaperrr reaperrr deleted the reaperrr:WithAttackAnimRefactor branch Jul 23, 2017

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