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

Reimplement MADtank logic as activity. #16805

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@tovl
Copy link
Contributor

commented Jul 19, 2019

Fixes #16780.
Fixes #13470.

@pchote pchote added this to the Next Release milestone Jul 19, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

The timing of the screen shake effect also seems to have regressed.

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Scope creeping suggestion / request: If we changed MadTank to be a subclass of AttackFrontal or AttackBase (like AttackLeap, but with extra deploy stuff) it would fix force-attack and target-outside-range cursors not working as expected.

@tovl tovl force-pushed the tovl:madtank-fix branch from a7cf3e5 to 71441a9 Jul 21, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Applied fixups.

I think turning this into an Attack* trait is a good idea, but I'd rather leave that for next+1.

@tovl tovl force-pushed the tovl:madtank-fix branch from 71441a9 to bf62860 Jul 21, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

The MoveAdjacentTo doesn't appear to be working.
Repro case:

  • Build a mad tank
  • Target an enemy building outside range (using the attack cursor, not the deploy one)

The tank will deploy in its current location instead of moving. We'll also need to be careful to check that the target is still valid before deploying, so that it doesn't automatically deploy in the middle of nowhere if the target dies while its moving.

@tovl tovl force-pushed the tovl:madtank-fix branch from bf62860 to d31c5d0 Jul 21, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Fixed.

@tovl tovl force-pushed the tovl:madtank-fix branch from d31c5d0 to 1840539 Jul 21, 2019

@tovl tovl force-pushed the tovl:madtank-fix branch from 1840539 to 6d74a03 Jul 21, 2019

@tovl tovl force-pushed the tovl:madtank-fix branch 2 times, most recently from 825f132 to 3457248 Jul 21, 2019

self.QueueActivity(new CallFunc(StartDetonationSequence));
public override bool Tick(Actor self)
{
if ((self.Location - target.Actor.Location).LengthSquared > 3)

This comment has been minimized.

Copy link
@pchote

pchote Jul 21, 2019

Member

This will behave inconsistently for actors with big footprints. Enter solves this by calling IMove.CanEnterTargetNow, which (despite the name) does exactly what we need here (assuming that we are serious about wanting to be adjacent to the target...).

Can we please instead cache IMove from the ctor and then change this to if (target.Type != TargetType.Invalid && !move.CanEnterTargetNow(self, target))?

I couldn't find any more bugs after testing with this change locally, so 👍 once you can change this.

This comment has been minimized.

Copy link
@pchote

pchote Jul 21, 2019

Member

If we want to be more relaxed about exactly where we deploy we could instead define something like

public readonly WDist TargetDetonationRange = WDist.FromCells(3);

in the MADTankInfo and then use

if (target.Type != TargetType.Invalid && !target.IsInRange(self.CenterPosition, mad.info.TargetDetonationRange))
{
	QueueChild(new MoveWithinRange(self, target, WDist.Zero, mad.info.TargetDetonationRange, targetLineColor: Color.Red));
	return false;
}    

here.

This is probably a better solution, MoveAdjacentTo was always a weird choice for this, but I haven't tested this suggestion.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jul 21, 2019

Contributor

The second suggestion sounds better to me.

This comment has been minimized.

Copy link
@pchote

pchote Jul 21, 2019

Member

Thinking about this further, is there any reason why we couldn't/shouldn't use the range defined on DetonationWeapon? This would be better than defining a separate TargetDetonationRange.

This comment has been minimized.

Copy link
@tovl

tovl Jul 22, 2019

Author Contributor

I would presume that when you use the madtank you want to take out as many buildings at once, so you would choose a target in the middle of a clump of buildings. Deploying as soon as the target is on the edge of the detonation radius doesn't seem very effective.

Especially if you want to later on add support for force-attack commands, it seems most intuitive to me if the madtank would actually move to the ordered tile instead of somewhere on the edge of the range-circle. So I think that the first suggestion makes more sense from a practical viewpoint.

@tovl tovl force-pushed the tovl:madtank-fix branch from 3457248 to cab07e9 Jul 23, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Updated.

@teinarss

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Not possible to cancel/change order after issue DetonateAttack

@tovl tovl force-pushed the tovl:madtank-fix branch from cab07e9 to 2ef5aff Jul 25, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

I forgot to put in an IsCanceling clause. Fixed.

@pchote pchote referenced this pull request Jul 26, 2019
@teinarss
Copy link
Contributor

left a comment

Looks good

@abcdefg30 abcdefg30 merged commit 207305e into OpenRA:bleed Jul 30, 2019

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

commented Jul 30, 2019

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