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

Attack dog leap improvement #13513

Closed
wants to merge 3 commits into from
Closed

Conversation

forcecore
Copy link
Contributor

@forcecore forcecore commented Jun 15, 2017

Attempt to fix #7506

Based on Mailaender's dog improvement PR #11715. (Sorry about C&P instead of Git Cherry Pick but it was way behind bleed)

Demo:


var leapToken = ConditionManager.InvalidConditionToken;

return ActivityUtils.SequenceActivities(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use child activities and OnFirstRun instead of SequenceActivities. Refer to the Transform activity for an example.

@forcecore
Copy link
Contributor Author

  • Now using parent-child activity
  • Buff on and off now encapsulated in AttackLeap class.
    Leap stuff shouldn't be pre-calculated in OnFirstRun otherwise it will be incorrect, as self can change position.

@forcecore
Copy link
Contributor Author

Forgot to mention, in parent-child activity version, I addressed far-jumping dogs
https://youtu.be/1lGmjOaNBxQ

@forcecore
Copy link
Contributor Author

Fixed shell map crash that wasn't caught before

@reaperrr
Copy link
Contributor

Needs rebase.

@forcecore
Copy link
Contributor Author

Rebased

}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Leave newline here.

conditionManager.GrantCondition(self, info.AttackingCondition, info.AttackingDuration);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@forcecore
Copy link
Contributor Author

EOL fixed, squashed.

Copy link
Member

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TheCallFunc activity is not serialization friendly (game saving). Granted, it may take a while before that can be added; however, it is better to code for its support now than to make more work for later.

{
attack.ApproachBuffOn(self);
QueueChild(new MoveWithinRange(self, target, WDist.Zero, range));
QueueChild(new CallFunc(() => attack.ApproachBuffOff(self)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove:

				QueueChild(new CallFunc(() => attack.ApproachBuffOff(self)));

var range = attack.GetMaximumRange();
if (!target.IsInRange(self.CenterPosition, range))
{
attack.ApproachBuffOn(self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append:

				isApproachBuffOn = true;


if (ChildActivity != null)
{
ActivityUtils.RunActivity(self, ChildActivity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with:

				var act = ActivityUtils.RunActivity(self, ChildActivity);
				if (isApproachBuffOn && act == this))
					attack.ApproachBuffOff(self);

readonly Target target;
readonly AttackLeapInfo info;
readonly Mobile mobile;
readonly AttackLeap attack;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Append:

		bool isApproachBuffOn = false;

@forcecore
Copy link
Contributor Author

forcecore commented Aug 1, 2017

Removed CallFunc. Removed more visual glitch.

AttackLeap:
	Voice: Attack
	LeapCondition: leap
	ApproachCondition: rush
	# Disable another leap while leaping or ripping
	RequiresCondition: !leap && !rip

I also made rip condition to disable AttackLeap to prevent dogs from jumping immediately. The problem is, if I queue attack orders, rip disables the attack trait and the queue gets canceled. I didn't look into detail on the reason but I can't queue anymore with && !rip condition. If I remove && !rip things return to normal. I think AttackBase trait should be a pausable trait as well.

@pchote
Copy link
Member

pchote commented Aug 17, 2017

I think AttackBase trait should be a pausable trait as well.

This sounds like a sensible solution. Would you mind filing a PR for this?

@pchote
Copy link
Member

pchote commented Aug 17, 2017

This reintroduces problems with dogs stacking on top of other infantry.

Repro case:

  • Start a game with two clients
  • Player one builds a cell full of non-combat infantry, or normal infantry in the hold fire stance
  • Player two builds 5 dogs, puts them in the hold fire stance, and orders them to move next to player 1's units.
  • Player two selects all 5 dogs and orders them to attack the center unit from Player one's cell.

The dogs will then leap but not adjust their sub-cells. The outer 4 will be left overlapping with Player one's other infantry:
screen shot 2017-08-17 at 18 03 57

@GraionDilach
Copy link
Contributor

I'd like to see this trait renamed to AttackJoust during this PR, because it no longer involves a leap.

See my reasoning at OpenRA/ra2#328 (comment).

@forcecore
Copy link
Contributor Author

forcecore commented Aug 18, 2017

I let them overlap. Otherwise, when the cell contains 5 enemy infantries, attack dogs can't attack any of those infantries at all. If I don't hard-code one-shot kill into the code, I'd get overlap anyway if target HP > dog damage. The neatest solution without unit stacking would be to implement RA3 dog/bear melee attack.

Now I see why WW hard coded one-shot-kill to RA2 attack dogs.

@forcecore
Copy link
Contributor Author

I think AttackBase trait should be a pausable trait as well.
This sounds like a sensible solution. Would you mind filing a PR for this?

I'll try. That should improve EMP in TS too.

@pchote
Copy link
Member

pchote commented Aug 18, 2017

If I don't hard-code one-shot kill into the code, I'd get overlap anyway if target HP > dog damage.

This is precisely the reason for the one-shot kill in the current implementation.

@forcecore
Copy link
Contributor Author

I inteded to respect Mailaender's removal of one shot kill. I guess I have to bring back it back. On the other hand, if dogs jump at the same time to the same target, they will overlap. I think the proper way to avoid this is to introduce some kind of a coordination to the jumping, something like resource claim layer. What do you think about the doggy jumping coordination layer?

@GraionDilach
Copy link
Contributor

Hardcoding the one-shot-kill will remove the chance of using this implementation for parasite behaviour (RA2 Terror Drone vs vehicles, Firestorm Limpet Drone). Another option would be that the actor would bounce to a free subcell after an attack when the target isn't killed.

@forcecore
Copy link
Contributor Author

Hmm, probably why the attack dogs in RA1 "slides" after ripping up. I'll try that.

@pchote
Copy link
Member

pchote commented Aug 25, 2017

I think the proper way to avoid this is to introduce some kind of a coordination to the jumping, something like resource claim layer. What do you think about the doggy jumping coordination layer?

Yes, we need this, but not as a layer. Implement it as a trait on the actor itself, like we have for external capturing or the various things that lock buildings.

@pchote
Copy link
Member

pchote commented Aug 25, 2017

Hardcoding the one-shot-kill will remove the chance of using this implementation for parasite behaviour

Parasite behaviour requires the targeting actor to enter the target, which is a completely different usecase. Trying to solve that with the same code as attack dogs makes for uglier and more complicated solutions than necessary for both.

@GraionDilach
Copy link
Contributor

Squid parasite didn't really entered into the target, was moreso attached to.

@pchote
Copy link
Member

pchote commented Sep 6, 2017

Squid parasite behaviour requires the targeting actor to attach to the target, which is a completely different usecase. Trying to solve that with the same code as attack dogs makes for uglier and more complicated solutions than necessary for both.

😉

@pchote
Copy link
Member

pchote commented Sep 24, 2017

@forcecore: do you plan on continuing with this in the short term?

@forcecore
Copy link
Contributor Author

Not in the short term, I have personal issues to sort out for about a few weeks.

@pchote pchote modified the milestones: Next + 1, Future Sep 24, 2017
@pchote
Copy link
Member

pchote commented Sep 24, 2017

Ok, no worries. I've moved this to the Future milestone so that we don't lose track of this, and will close it for now to help reduce the PR queue to items that need active review. Please do reopen this as soon as you are able to continue, because it would be good to finally fix this.

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

Successfully merging this pull request may close these issues.

Dog issues
6 participants