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

Add WithTurretAimAnimation and remove WithSpriteTurret.AimSequence #14846

Merged
merged 3 commits into from Mar 9, 2018

Conversation

Projects
None yet
2 participants
@reaperrr
Copy link
Contributor

reaperrr commented Feb 23, 2018

AimSequence and AttackSequence on WithTurretAttackAnimation actually conflicted with each other when used at the same time, so splitting them is the sensible thing to do.

Additionally, with this the AimSequence on WithSpriteTurret becomes redundant, allowing us to remove it and simplify WithSpriteTurret's code.

Testcase: Check that TD MLRS/Rocket Launcher still correctly raises turret before firing.

Split from #14036.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Feb 23, 2018

OpenRA.Mods.Cnc/Traits/Render/WithReloadingSpriteTurret.cs(60,27): error CS0115: 'OpenRA.Mods.Cnc.Traits.Render.WithReloadingSpriteTurret.Tick(OpenRA.Actor)' is marked as an override but no suitable method found to override

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Feb 23, 2018

Meh, seems it depends on #14845 afterall.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 8, 2018

Rebased and updated.

var atkSequence = turAttackAnim.Value.Nodes.FirstOrDefault(n => n.Key == "AttackSequence");
var aimSequence = turAttackAnim.Value.Nodes.FirstOrDefault(n => n.Key == "AimSequence");

// If only AimSequence is null, just rename AttackSequence to Sequence (ReloadPrefix is very unlikely to be defined in that case).

This comment has been minimized.

@abcdefg30

abcdefg30 Mar 9, 2018

Member

I'm confused now. Why would want to rename AttackSequence? You didn't change that on WithTurretAttackAnimation?

This comment has been minimized.

@reaperrr

reaperrr Mar 9, 2018

Author Contributor

You didn't change that on WithTurretAttackAnimation?

But I wanted to, thanks for pointing this out.

@reaperrr reaperrr force-pushed the reaperrr:turret-AimAnim branch from 8194622 to fafa008 Mar 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 9, 2018

Updated.

Split *AimAnimation from WithTurretAttackAnimation
These two didn't interact and actually even conflicted when used at the same time, so splitting them is the sensible thing to do.

@reaperrr reaperrr force-pushed the reaperrr:turret-AimAnim branch from fafa008 to 42689f1 Mar 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 9, 2018

Rebased.

reaperrr added some commits Sep 16, 2017

Remove WithSpriteTurret.AimSequence
We can now use WithTurretAimAnimation instead.

@reaperrr reaperrr force-pushed the reaperrr:turret-AimAnim branch from 42689f1 to 45b1e22 Mar 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Mar 9, 2018

Fixed MLRS crash.

@abcdefg30 abcdefg30 merged commit 02b1530 into OpenRA:bleed Mar 9, 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 Mar 9, 2018

@Valkirie Valkirie referenced this pull request May 1, 2018

Closed

Added GI sandbag deployment #394

1 of 3 tasks complete

@reaperrr reaperrr deleted the reaperrr:turret-AimAnim branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.