Streamline turret render traits - part 2 #14036

reaperrr · 2017-09-16T21:01:35Z

My idea for making #13619 more flexible is to use integer priority values instead of a single boolean, but even independently from that, our current turret render traits are a bit of a mess, especially when it comes to anything dealing with "aiming" at a target.
This needs to be sorted out first to avoid problems and work-arounds in #13619.

This PR straightens out that mess, so that both #13619 as well as any future additional WithTurret*Animation traits should have an easier time to do what they're intended to do without implicit conflicts or compatibility issues.

pchote · 2017-10-15T14:33:34Z

OpenRA.Mods.Common/Traits/Render/WithSpriteTurret.cs

-			var sequence = Attack.IsAttacking ? Info.AimSequence : Info.Sequence;
-			DefaultAnimation.ReplaceAnim(sequence);
-		}
+		protected virtual void Tick(Actor self) { }


Better to remove ITick completely, and if a subclass wants to tick then they can implement the interface themselves.

pchote · 2017-10-15T14:38:20Z

OpenRA.Mods.Common/Traits/Render/WithTurretAttackAnimation.cs

 		}

 		void INotifyAttack.Attacking(Actor self, Target target, Armament a, Barrel barrel)
 		{
-			if (info.DelayRelativeTo == AttackDelayType.Attack)


This appears to be almost completely duplicated with INotifyAttack.PreparingAttack.
Could you split out a void NotifyAttack(Actor self, AttackDelayType type) method that is called from each?

pchote · 2017-10-15T14:39:33Z

OpenRA.Mods.Common/Traits/Render/WithTurretAttackAnimation.cs

-				PlayAttackAnimation(self);
-
-			if (string.IsNullOrEmpty(info.AimSequence) && string.IsNullOrEmpty(info.ReloadPrefix))
+			if (IsTraitDisabled)


Not much point having this as an early return. Just add !IsTraitDisabled to the if statement below.

pchote · 2017-10-15T14:40:24Z

mods/cnc/cursors.yaml

@@ -235,3 +235,10 @@ Cursors:
 			Length: 8
 		attackoutsiderange-minimap:
 			Start: 8
+	mouse8.shp: cursor


Rebase error?

pchote · 2017-10-15T14:41:59Z

OpenRA.Mods.Cnc/Traits/Render/WithTurretAmmoReloadAnimation.cs

+
+		public override void RulesetLoaded(Ruleset rules, ActorInfo ai)
+		{
+			var matches = ai.TraitInfos<WithTurretAimAnimationInfo>().Count(t => t.Turret == Turret);


Style nit: .Any(t => t.Turret == Turret) instead of Count != 0.

pchote · 2017-10-15T14:45:02Z

OpenRA.Mods.Common/Traits/Render/WithTurretAimAnimation.cs

+
+		public override void RulesetLoaded(Ruleset rules, ActorInfo ai)
+		{
+			var turretAttackAnim = ai.TraitInfos<WithTurretAttackAnimationInfo>().Any();


Shouldn't this be checking the turret the same way as WithTurretAmmoReloadAnimationInfo?

reaperrr · 2017-10-27T20:01:19Z

Basically depends on #13538 due to
#13538 (review)

reaperrr · 2017-11-12T23:19:15Z

Updated and rebased.

Note: Added a testcase commit that needs to be removed before merging (makes cnc mobile sam able to attack ground, for easier testing).

reaperrr · 2017-11-15T18:21:37Z

Rebased and updated.

Moved testcase here (TD Nod Mobile SAM):
https://github.com/reaperrr/OpenRA/tree/refact-tur-render2-testcase

penev92 · 2017-11-27T13:07:58Z

Has merge conflicts again with god-knows-what.

GraionDilach · 2017-11-27T16:33:57Z

With #14202.

reaperrr · 2017-11-28T19:00:13Z

Rebased.

With #14202.

And #14425.

reaperrr · 2017-12-28T07:04:44Z

Rebased.

abcdefg30 · 2017-12-28T18:24:12Z

And needs another rebase.

reaperrr · 2018-01-04T07:41:33Z

I guess there's no point in rebasing this until the upgrade rule refactor is merged.

Arular101 · 2018-01-13T17:31:50Z

Please don't forget to update the copyright notice year.

WithReloadingSpriteTurret was bound to run into conflicts with any WithTurret*Animation traits due to overriding the turret sequence constantly via ITick. Using (stacked) conditions instead avoids that.

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

We can now use WithTurretAimAnimation instead.

reaperrr · 2018-02-23T03:17:23Z

Rebased and updated.

reaperrr · 2018-03-06T22:35:10Z

Superseded by #14845, #14846 and a future third PR that will contain the rest of the changes from this one.

reaperrr added the PR: Is Dependency label Sep 16, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch from d108d33 to 31a748a Compare September 16, 2017 21:07

reaperrr mentioned this pull request Sep 16, 2017

Add uninterruptability support to sprite turret anims and fix trait namings #13619

Closed

reaperrr added the PR: Rebase me! label Sep 28, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch from 31a748a to 4966002 Compare September 28, 2017 12:43

reaperrr removed the PR: Rebase me! label Sep 28, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch 2 times, most recently from 02426aa to 52494f3 Compare October 14, 2017 22:07

pchote reviewed Oct 15, 2017

View reviewed changes

pchote mentioned this pull request Oct 16, 2017

Add ReloadAmmoPool trait #13538

Merged

reaperrr force-pushed the refactor-turret-rendertraits2 branch from 52494f3 to ed98b2f Compare October 22, 2017 17:42

reaperrr added the PR: Has dependencies label Oct 27, 2017

reaperrr added the PR: Rebase me! label Nov 9, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch 2 times, most recently from d350d6d to 24a53eb Compare November 12, 2017 23:11

reaperrr removed PR: Has dependencies PR: Rebase me! labels Nov 12, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch 2 times, most recently from 8e8dda8 to 40ef9c6 Compare November 15, 2017 18:20

penev92 added the PR: Rebase me! label Nov 27, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch from 40ef9c6 to 18a1196 Compare November 28, 2017 18:52

reaperrr removed the PR: Rebase me! label Nov 28, 2017

reaperrr added the PR: Rebase me! label Dec 11, 2017

reaperrr force-pushed the refactor-turret-rendertraits2 branch from 18a1196 to c0c2e06 Compare December 28, 2017 04:13

reaperrr removed the PR: Rebase me! label Dec 28, 2017

abcdefg30 added the PR: Rebase me! label Dec 28, 2017

reaperrr added the PR: Has dependencies label Jan 4, 2018

Replace WithReloadingSpriteTurret with conditions

eff0ca4

WithReloadingSpriteTurret was bound to run into conflicts with any WithTurret*Animation traits due to overriding the turret sequence constantly via ITick. Using (stacked) conditions instead avoids that.

reaperrr force-pushed the refactor-turret-rendertraits2 branch from c0c2e06 to 045b186 Compare February 23, 2018 03:10

reaperrr added 5 commits February 23, 2018 04:13

Split *AimAnimation from WithTurretAttackAnimation

25e5c55

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

Remove WithSpriteTurret.AimSequence

e619723

We can now use WithTurretAimAnimation instead.

Make WithTurretAttackAnimation conditional

653bd17

Make WithTurretAimAnimation conditional

0c78d49

De-duplicate some WithTurretAttackAnimation code

04fb88e

reaperrr force-pushed the refactor-turret-rendertraits2 branch from 045b186 to 04fb88e Compare February 23, 2018 03:14

reaperrr removed PR: Has dependencies PR: Is Dependency PR: Rebase me! labels Feb 23, 2018

This was referenced Feb 23, 2018

Replace WithReloadingSpriteTurret with conditions #14845

Merged

Add WithTurretAimAnimation and remove WithSpriteTurret.AimSequence #14846

Merged

reaperrr closed this Mar 6, 2018

reaperrr deleted the refactor-turret-rendertraits2 branch March 9, 2018 01:22

reaperrr mentioned this pull request Mar 9, 2018

Make turret Attack/Aim anim traits conditional #14895

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streamline turret render traits - part 2 #14036

Streamline turret render traits - part 2 #14036

reaperrr commented Sep 16, 2017 •

edited

Loading

pchote Oct 15, 2017

pchote Oct 15, 2017

reaperrr Nov 28, 2017

pchote Oct 15, 2017

pchote Oct 15, 2017

reaperrr Oct 15, 2017

pchote Oct 15, 2017

pchote Oct 15, 2017

reaperrr commented Oct 27, 2017

reaperrr commented Nov 12, 2017

reaperrr commented Nov 15, 2017

penev92 commented Nov 27, 2017

GraionDilach commented Nov 27, 2017

reaperrr commented Nov 28, 2017

reaperrr commented Dec 28, 2017

abcdefg30 commented Dec 28, 2017

reaperrr commented Jan 4, 2018

Arular101 commented Jan 13, 2018

reaperrr commented Feb 23, 2018

reaperrr commented Mar 6, 2018

Streamline turret render traits - part 2 #14036

Streamline turret render traits - part 2 #14036

Conversation

reaperrr commented Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperrr commented Oct 27, 2017

reaperrr commented Nov 12, 2017

reaperrr commented Nov 15, 2017

penev92 commented Nov 27, 2017

GraionDilach commented Nov 27, 2017

reaperrr commented Nov 28, 2017

reaperrr commented Dec 28, 2017

abcdefg30 commented Dec 28, 2017

reaperrr commented Jan 4, 2018

Arular101 commented Jan 13, 2018

reaperrr commented Feb 23, 2018

reaperrr commented Mar 6, 2018

reaperrr commented Sep 16, 2017 •

edited

Loading