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

Refactor AimAnimation traits #15140

Merged
merged 6 commits into from Jun 3, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented May 14, 2018

Splits WithAimAnimation from WithAttackAnimation and refactors how/when the aim sequence + prefix are updated.

Edit: Now also removes ReloadPrefix from the AimAnimation traits, added a GrantConditionOnArmamentReloading trait instead. Switching out the entire sprite body/turret is more reliable than doing these prefix gymnastics.
Supersedes #15078.

Prerequisite for my upcoming animation priority system that aims to fix/avoid all similar and future animation conflicts.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 14, 2018

Travis failed.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch from 4a276d4 to 67cd2d1 May 14, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 14, 2018

Fixed.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 16, 2018

Updated: Now also removes ReloadPrefix from the *AimAnimation traits, added a GrantConditionOnArmamentReloading trait instead. Switching out the entire sprite body/turret is more reliable than doing these prefix gymnastics.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch from 633b035 to c67c563 May 16, 2018

: base(info)
{
armament = self.TraitsImplementing<Armament>()
.Single(a => a.Info.Name == Info.Armament);

This comment has been minimized.

@GraionDilach

GraionDilach May 17, 2018

Contributor

Do we have an official policy on this one? How should eliteweapons be treated in this case? How are they treated in TS?

This comment has been minimized.

@reaperrr

reaperrr May 17, 2018

Author Contributor

How should eliteweapons be treated in this case?

Elite weapons should have a different name than their non-elite counterpart, then you can use a 2nd GCOAR for the elite weapon.

@pchote said months ago that we should ultimately aim for one attack trait per armament, so each armament having a distinct name will become mandatory anyway, eventually.
I'm trying to write anything somewhat related with that in mind already, to keep the number of places that need to be adapted later as low as possible.

How are they treated in TS?

Probably still using "primary" for both, but iirc there aren't any actors in TS that have both an elite weapon and one of the affected traits, so at this point it shouldn't matter.

This comment has been minimized.

@GraionDilach

GraionDilach May 17, 2018

Contributor

Pffft, that proposal will be a PITA to work with on IFVs/units using multiple weapon instances as burst cases. Whatever, I'll rip my hair off when that break comes in.

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

@pchote said months ago that we should ultimately aim for one attack trait per armament, so each armament having a distinct name will become mandatory anyway, eventually.

Hmm, I don't remember this.

This comment has been minimized.

@reaperrr

reaperrr May 22, 2018

Author Contributor

Ok... anyway, I think the approach of this PR is much more robust and less issue-prone than attempting to control every Armament via a single condition trait would be.

However, it just occured to me that maybe making the Armament itself grant a ReloadingCondition (instead of adding GrantConditionOnArmamentReloading) would be even better. What do you think?

}

void PlayAttackAnimation(Actor self)
{
if (!IsTraitDisabled && !wsb.IsTraitDisabled && !string.IsNullOrEmpty(Info.AttackSequence))
if (!IsTraitDisabled && !wsb.IsTraitDisabled && !string.IsNullOrEmpty(Info.Sequence))
{

This comment has been minimized.

@abcdefg30

abcdefg30 May 24, 2018

Member

Braces around a single-line statement.

{
get
{
return "ReloadPrefix has been removed from WithAimAnimation and WithTurretAimAnimation.\n"

This comment has been minimized.

@abcdefg30

abcdefg30 May 24, 2018

Member

Am I mixing something up here or did you just add WithAimAnimation in this PR together with an update rule refactoring the new trait?

This comment has been minimized.

@reaperrr

reaperrr May 24, 2018

Author Contributor

Hm yeah, now that you mention it... guess I should put all the WithAimAnimation stuff in one update rule and let this here only update WithTurretAimAnimation.

This comment has been minimized.

@pchote

pchote May 27, 2018

Member

The simpler the better. What is the strategy forward here?

@pchote pchote added this to the Next release milestone May 27, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 27, 2018

This is a dependency for #15008 and ties in with the various update rule messyness, so adding to the milestone.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch 2 times, most recently from 83d6464 to 667b57e May 28, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 28, 2018

Updated:

  • merged #15139 back into this
  • merged all update rules into one
  • reorganised and restructured commits to make the whole thing easier to review (and update, if necessary)

The only point I haven't adressed yet, since I wanted to get some feedback first:

However, it just occured to me that maybe making the Armament itself grant a ReloadingCondition (instead of adding GrantConditionOnArmamentReloading) would be even better. What do you think?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 28, 2018

Allowing the armament to grant the condition seems like a good approach.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch 2 times, most recently from 2c476c8 to 27a5540 May 28, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 28, 2018

Updated.

@pchote
Copy link
Member

pchote left a comment

The code changes look good to me, but I haven't tested this yet. In the meantime, I do have one minor nit/opinion for discussion:

@@ -61,6 +61,10 @@ public class ArmamentInfo : PausableConditionalTraitInfo, Requires<AttackBaseInf
[Desc("Use multiple muzzle images if non-zero")]
public readonly int MuzzleSplitFacings = 0;

[GrantedConditionReference]
[Desc("Condition to grant while reloading.")]
public readonly string OnReloadingCondition = null;

This comment has been minimized.

@pchote

pchote May 29, 2018

Member

IMO this should be ReloadingCondition for consistency. We don't do e.g. OnAirborneCondition or OnCruisingCondition in Aircraft.

This comment has been minimized.

@reaperrr

reaperrr May 29, 2018

Author Contributor

I was a bit unsure about this, because - purely from a wording perspective - some modders might misinterpret ReloadingCondition as "this condition is needed for reloading".
On the other hand, that's what the Desc is for, and modders should know from other traits that the property name would include "Requires" if that were its purpose.

Add INotifyAiming interface
And trigger notifications from Attack* traits.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch from 27a5540 to 9f8ab6f May 30, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented May 30, 2018

Updated.

@pchote
Copy link
Member

pchote left a comment

LGTM 👍. Just a few minor nits and one major issue that has an easy fix.


public override void RulesetLoaded(Ruleset rules, ActorInfo ai)
{
var matches = ai.TraitInfos<WithSpriteBodyInfo>().Count(w => w.Name == Body);

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

Style nit: .Count requires walking the entire enumeration, which is wasteful in cases where we don't need to know what the actual count is. When we want to know if there is exactly one it is conceptually better to use SingleOrDefault and then check for null. In practice this will usually walk the entire enumeration too, but it can return early as soon as it finds a second element that matches the condition.


void INotifyAiming.StartedAiming(Actor self, AttackBase ab)
{
wsb.DefaultAnimation.ReplaceAnim(Sequence);

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

IMO the code would be clearer if we could do:

protected void UpdateSequence()
{
    var seq = !IsTraitDisabled && attack.IsAiming ? Info.Sequence : wsb.Info.Sequence;
    wsb.DefaultAnimation.ReplaceAnim(seq);
}

void INotifyAiming.StartedAiming(Actor self, AttackBase ab) { UpdateSequence(); }
void INotifyAiming.StoppedAiming(Actor self, AttackBase ab) { UpdateSequence(); }
protected override void TraitEnabled(Actor self) { UpdateSequence(); }
protected override void TraitDisabled(Actor self) { UpdateSequence(); }
}

void INotifyAttack.Attacking(Actor self, Target target, Armament a, Barrel barrel)
{
if (a != armament)

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

This can be merged into the check below: if (a == armament && Info.DelayRelativeTo == AttackDelayType.Attack)

@@ -92,6 +80,9 @@ void INotifyAttack.Attacking(Actor self, Target target, Armament a, Barrel barre

void INotifyAttack.PreparingAttack(Actor self, Target target, Armament a, Barrel barrel)
{
if (a != armament)

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

Likewise here

readonly WithSpriteTurret wst;
string sequence;

protected string Sequence { get { return !IsTraitDisabled && attack.IsAiming ? Info.Sequence : wst.Info.Sequence; } }

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

Can do the same simplification here as in WithAimAnimation.

public override IEnumerable<string> AfterUpdate(ModData modData)
{
var message1 = "AimSequences have been split to With*AimAnimation.\n"
+ "The following locations have been updated and might need manual adjustments:\n"

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

Can you please change "locations" to "actors" here?


var message2 = "ReloadPrefixes have been removed.\n"
+ "Instead, grant a condition on reloading via Armament.ReloadingCondition to enable\n"
+ "an alternate sprite body (with reloading sequences) at the following locations:\n"

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

likewise here "on the following actors"

@@ -169,8 +177,24 @@ protected override void Created(Actor self)
base.Created(self);
}

void UpdateCondition(Actor self)
{
if (conditionManager == null)

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

if (string.IsNullOrEmpty(Info.ReloadingCondition) || conditionManager == null)!!!

Right now we crash when anything except the V2 fires its weapon.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 2, 2018

The ReloadingCondition and INotifyAiming are going to be really useful going forwards. If #15008 goes ahead with a GrantConditionWhileAiming trait then we may be able to reproduce the original C&C rocket launcher behaviour where it stops moving and unlocks its turret while aiming.

Add ReloadingCondition to Armament
Allows to grant condition while reloading.

reaperrr added some commits May 28, 2018

Refactor WithTurretAimAnimation using INotifyAiming
Triggering start and end of the aiming animation via interface, as well as removing ReloadPrefix (in favor of switching sprite bodies via condition when reloading), allows us to drop updating via ITick, which in turn will make it much easier to ultimately make this trait compatible with other animation traits, once the priority system lands.
Split and refactor aim animation from WithAttackAnimation
Splitting it from the attack animation, triggering start and end of the aiming animation via interface, as well as removing ReloadPrefix (in favor of switching sprite bodies via condition when reloading) allows us to drop updating via ITick, which in turn will make it much easier to ultimately make this trait compatible with other animation traits, once the priority system lands.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch from 9f8ab6f to 6076c5a Jun 2, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 2, 2018

Updated.


var message2 = "ReloadPrefixes have been removed.\n"
+ "Instead, grant a condition on reloading via Armament.ReloadingCondition to enable\n"
+ "an alternate sprite body (with reloading sequences) at the following actors:\n"

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Member

at → on.

reaperrr added some commits May 15, 2018

Use Armament.ReloadingCondition on RA V2
The new property makes the AmmoPool work-around obsolete.

@reaperrr reaperrr force-pushed the reaperrr:aim-anim-refactor branch from 6076c5a to bd062ff Jun 2, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 2, 2018

Fixed.

@pchote

pchote approved these changes Jun 3, 2018

@pchote pchote merged commit 1408fb7 into OpenRA:bleed Jun 3, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

pchote commented Jun 11, 2018

#15201 reveals that this PR didn't rename AttackSequenceSequence in TS. Did you forget to update/stage these changes, or was this a failing in the update rule?

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 12, 2018

I just forgot to update TS.

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.