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

Make Attack activities explicitly account for hidden actors. #16067

Merged
merged 14 commits into from Jan 26, 2019

Conversation

Projects
None yet
7 participants
@pchote
Copy link
Member

pchote commented Jan 16, 2019

Part two of #15866, addressing a major oversight that nobody realized before the first playtest.

This PR overhauls the activities used by the attack logic to bring a unit into firing range of a target.
The old behaviour that (rather inconsistently) either cancelled the activity or returned a move order when the target becomes hidden under the fog or is killed has been replaced with:

  • Unit continues moving towards the last known location of the target
  • Unit will reacquire the original target if it is revealed before it reaches the last known location
  • Unit drops the target only once it reaches the last known location and the target remains hidden

Fixes #16010.
Fixes #10224.
Closes #10223.

Note that this does not change units dropping targets that become hidden while inside firing range -- this isn't a bug (see my final comment in #10223), and will instead be addressed by #11663.

I strongly suggest reviewing individual commits in order, which by themselves should be straightforward enough.

  • The first four commits address limitations in Target and DrawLineToTarget needed by the later commits, and fix some code style issues while i'm there.
  • The "Pass target line color to inner move activities." commit introduces the plumbing needed to be able to swap back and forward between the original target and the static "last seen" position. This also fixes the major blocker that kept sinking the various attempts at #12019, so I expect that we will be able to resurrect #14384 for Next + 1. We will be able to remove all of the SetTargetLine(..., false) calls introduced in the later commits once this happens.
  • The "Define plumbing to pass initial target positions..." commit adds a parameter to the move interface and activities that lets outer activities pass a fallback position to inner activities. This allows us to propagate the last known position to child activities to preserve the behaviour explained above. Doing the plumbing change in its own commit allows us to split the implementation across multiple commits without breaking the build.
  • The remaining commits implement the behaviour described above in all of the attack-related activities (non-attack targeting activities fall under #16048). The same code pattern is used in all of them, so once you understand (and agree with) the basic logic, reviewing is just a case of making sure I have used checkTarget etc in the right places.

When testing, I expect that regressions will either take the form of "Attempting to query the position of an invalid Target" crashes, target line weirdness, or actors abandoning (or not) targets too early (or never). I have tried to test all of the specific behaviour cases, but have not had time to play general games to look for unexpected regressions.

@pchote pchote added this to the Next Release milestone Jan 16, 2019

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Jan 17, 2019

  1. target yak to attack a tank
  2. tank disappears under fog
  3. yak flies to the last known location
  4. FIRST time yak just does a flyby over the tank
  5. CONSECUTIVE flybys it does attack the target

Looks like yak first tries to reach its target via MOVE, and only then switches to ATTACK afterwards
openra_fog-actors-attack3

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Jan 17, 2019

  1. Turrets still point to the last ordered target through fog (this supposed to be fixed here as well I think).

  2. Also some tanks here re-order themselves to the target's original location when they reach it's last known one.
    openra_fog-actors-attack4

upd: (1) all tanks go to the last known location first, then
(2) some reorder themselves to the location where their target was pointed first and move there. All of them re-target when anything hostile comes into vision after (1) though.

upd2 AFTER killing the target tanks continue to re-route to (2)

@pchote pchote force-pushed the pchote:restore-hidden-targets branch from 37769b7 to 3a5da7b Jan 17, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 17, 2019

Have fixed the yak and tank re-ordering issues.

The turret facing will require a bit more work and testing, so will defer that to the weekend.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 19, 2019

Updated. I've made some bigish changes to AttackFollow to make it work more like the other attack cases and remove (hopefully) old unused code.


DoAttack(self, Target);
IsAiming = Target.IsValidFor(self);
if (requestedTarget.Type != TargetType.Invalid)

This comment has been minimized.

@pchote

pchote Jan 19, 2019

Author Member

Note: I've set it up this way so that a followup PR can introduce an OpportunityFire flag and an auto-scanned opportunityTarget that is used if requestedTarget can't be aimed at. This will fix #14009.

allowMove = Info.AllowMovement && Stance > UnitStance.Defend;

// PERF: Avoid LINQ.
foreach (var attackFollow in attackFollows)

This comment has been minimized.

@pchote

pchote Jan 19, 2019

Author Member

I didn't specifically check, but at face value it looks like this shouldn't have been used at all. AttackFollow and its activity went out of its way to invalidate attackFollow.Target when a stop order was issued, and the idle check above will filter out anything that is actively targeting before we get here.

Dropping this workaround makes the code a lot cleaner and easier to reason about, so fingers crossed that this doesn't regress anything.

This comment has been minimized.

@pchote

pchote Jan 25, 2019

Author Member

Turns out that this broke tank kiting. Oops.

@Punsho

This comment has been minimized.

Copy link

Punsho commented Jan 19, 2019

When demo truck exploded some tanks/infantry my game crashed
exception-2019-01-19T112615Z.log
Also I really dislike turrets losing their target once it goes out of range, it would be ok if the turret would turn to it's target almost instantly and could retarget units while on the move but it is not the case is RA

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 19, 2019

The RA shellmap crashes for me after 1 minute https://gist.github.com/matjaeck/4016a1cf631e631ddd772ef1a6948609.

@pchote pchote force-pushed the pchote:restore-hidden-targets branch from f7d1294 to b90bffb Jan 19, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 19, 2019

Fixed the crash. I'd forgotten that move can be artificially null in Attack if the caller doesn't want the actor to move.

Also I really dislike turrets losing their target once it goes out of range, it would be ok if the turret would turn to it's target almost instantly and could retarget units while on the move but it is not the case is RA

This is a prerequisite for #14009 - the turret won't be able to acquire other targets if spends the whole time locked on to the primary target (which may be on the other side of the map).

@TheChosenEvilOne
Copy link
Contributor

TheChosenEvilOne left a comment

after some testing it seems to work fine.
👍

@matjaeck
Copy link
Contributor

matjaeck left a comment

It seem that yaks do not acquire new targets when a-moved (like in last release). Helicopters work fine when a-moved. Otherwise, #16010 is fixed.

#10223: Fixes artillery units too as far as I can tell because they will only move up to the last known position of a target until they are in firing range. The new behavior and the re-acquiring of targets worked well in my testgames (and I haven't found any issues with gap gens).

Turret alignment might be controversial but from my perspective I think it "looks" better and doesn't affect casual gameplay too much.

Played some AI matches (new AI rocks!)

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 21, 2019

It seem that yaks do not acquire new targets when a-moved (like in last release). Helicopters work fine when a-moved. Otherwise, #16010 is fixed.

This appears to have regressed between release-20171215 and playtest-20190106, so isn't related to the changes here. I'll look into it and file a fix in another PR unless whatever caused this is better reverted here.

Edit: Confirmed that this is completely unrelated to the targeting code. Filed #16092 to track the issue and #16093 to work around it.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 22, 2019

Capture orders on actors under the fog are not executed.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 22, 2019

Regression confirmed. The fix is easy (adding an ignoreTargetVisibility flag to MoveAdjacentTo to bridge the gap until #16048 can be tackled), but I've run out of time to do this tonight.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 22, 2019

Adding a fallback to reintroduce the old behaviour for capture etc appears to be more work than simply fixing these other activities properly, and would risk introducing regressions in the attack logic that has now had a fair amount of testing.

New plan is to scope-creep this to fix #16048, which may take a few more days.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jan 25, 2019

Updated to remove the target-invalidation when AttackFollow.AttackActivity is cancelled, and restore the hacks in AutoTarget that it was supposed to replace. Turns out that this code was what supported our current limited tank kiting abilities.

We can safely remove the hacks when fixing #14009 - desired behaviour will be maintained if AttackFollow.AttackActivity's IsCancelled branch can switch the requestedTarget over to opportunityTarget first before invalidating requestedTarget.

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Jan 25, 2019

The regression in auto targeting behavior is fixed and the old behavior restored.

wasMovingWithinRange = true;
return ActivityUtils.SequenceActivities(
move.MoveWithinRange(target, WDist.Zero, lastVisibleMaximumRange, checkTarget.CenterPosition, Color.Red),
this);

This comment has been minimized.

@obrakmann

obrakmann Jan 26, 2019

Contributor
  1. I think move could be null here
  2. and let's at least not introduce new ones of these: new Attack(self, target, move != null, forceAttack) instead of this

This comment has been minimized.

@pchote

pchote Jan 26, 2019

Author Member

move == null is included in the check just above

This comment has been minimized.

@pchote

pchote Jan 26, 2019

Author Member
  1. is going to be a pain here because it will throw away the lastVisibleTarget state, and I half intentionally avoided adding plumbing to send that around - it's a special type of Target that has multiple Positions.

Would you mind too much if we deferred these changes to a future PR?

wasMovingWithinRange = true;
return ActivityUtils.SequenceActivities(
move.MoveWithinRange(target, minRange, maxRange, checkTarget.CenterPosition, targetLineColor),
this);

This comment has been minimized.

@obrakmann

obrakmann Jan 26, 2019

Contributor

new Follow(self, target, lastVisibleTarget.CenterPosition, targetLineColor)?

This comment has been minimized.

@pchote

pchote Jan 26, 2019

Author Member

Likewise here.

Show resolved Hide resolved OpenRA.Mods.Common/TargetExtensions.cs Outdated
wasMovingWithinRange = true;
return ActivityUtils.SequenceActivities(
move.MoveWithinRange(target, minRange, maxRange, checkTarget.CenterPosition, Color.Red),
this);

This comment has been minimized.

@obrakmann

obrakmann Jan 26, 2019

Contributor

new AttackActivity(self, target, move != null, forceAttack)

@pchote pchote force-pushed the pchote:restore-hidden-targets branch from 40c3a2e to 4ceb568 Jan 26, 2019

@pchote pchote merged commit 60fcf59 into OpenRA:bleed Jan 26, 2019

2 checks passed

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

@pchote pchote deleted the pchote:restore-hidden-targets branch Jan 26, 2019

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