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 FrozenActors. #15866

Merged
merged 7 commits into from Dec 17, 2018

Conversation

Projects
None yet
7 participants
@pchote
Copy link
Member

pchote commented Nov 28, 2018

With the dust from #13995, #15176 and all the related PRs sufficiently settled, we can now progress to fixing the actor-side logic.

The workarounds that replaced FrozenActors with Actors in the order resolving, and to allow the attack activities to target actors that are hidden under the fog have been removed, and the attack activities can now fire on FrozenActors directly. The FrozenActor ⇆ Actor switching when a structure becomes/loses visibility is handled by making the activity Targets non-readonly and introducing a Target.Recalculate extension method that switches between them when needed. This also lets us propagate targets across Actor → Actor transformations for (almost) free.

Fixes #11265.
Fixes #14872.
Fixes #12856.
Fixes #10004.
Fixes #10887.
Fixes #14616.
Supersedes #12333 / #13363.

This fixes a couple of high-impact bugs in the RA mod (artillery sometimes rushing forward when ordered to attack, units refusing to target camo-pillboxes), and one of our biggest blockers for TS (stealth generators make the camo-pillbox bug affect everything). I'd therefore like to get this in for the playtest, and once the dust settles we can migrate the remaining activities over to the new system for Next + 1.

One of the side-effects of these fixes is that artillery in the "attack everything" stance will now automatically target structures within range that are under the fog, and any attacks (including manually ordered ones) will only automatically stop once another actor reveals that the target is dead. This has caused some controversy in IRC as it is arguably a gameplay regression. I'm moving forward with that here because we can't think of a better alternative: never fixing the targeting isn't viable, and dropping targets when they become hidden under fog seems like a bigger gameplay regression (but does remain an option if needed).

@pchote pchote added this to the Next Release milestone Nov 28, 2018

@pchote pchote referenced this pull request Nov 28, 2018

Closed

Frozen attack fix #13363

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from a704f88 to 3d2687b Nov 28, 2018

@matjaeck
Copy link
Contributor

matjaeck left a comment

When you reveal a cloaked structure so that you see its frozen actor and then reveal the fog of war again without revealing the cloaked actor, the frozen actor of the cloaked structure will be removed (expected), but units will still auto-target the structure when the cloaked actor is under the fog again.

Click to view gif

peek 2018-11-29 03-08


So this can happen:

Click to view gif

peek 2018-11-29 03-32


@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 29, 2018

Good catch. I have added a new commit which fixes that and also #14616.

The potential regression surface area has gone up, but IMO this is still more than worthwhile.

@matjaeck
Copy link
Contributor

matjaeck left a comment

Deleted and fetched the PR branch again. When I start a game with two local clients in the lobby, I get this exception:

Click to view exception log
OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
on map 6382a91476f7927a445e93be6dcd99c9e51ef661 (Northwest Passage by Raymundo).
Date: 2018-11-29 22:10:08Z
Operating System: Linux (Unix 4.15.0.39)
Runtime Version: Mono 5.16.0.220 (tarball Mon Nov 26 17:17:59 UTC 2018) CLR 4.0.30319.42000
Exception of type `System.NullReferenceException`: Object reference not set to an instance of an object
  at OpenRA.Primitives.PlayerDictionary`1[T].get_Item (OpenRA.Player player) [0x00001] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Mods.Common.Traits.FrozenUnderFog.IsVisibleInner (OpenRA.Actor self, OpenRA.Player byPlayer) [0x0002d] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.Mods.Common.Traits.FrozenUnderFog.IsVisible (OpenRA.Actor self, OpenRA.Player byPlayer) [0x00036] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.Actor.CanBeViewedByPlayer (OpenRA.Player player) [0x00035] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Traits.FrozenActor.RefreshState () [0x00023] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Mods.Common.Traits.FrozenUnderFog.UpdateFrozenActor (OpenRA.Actor self, OpenRA.Traits.FrozenActor frozenActor, System.Int32 playerIndex) [0x00017] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.Mods.Common.Traits.FrozenUnderFog+<OpenRA_Traits_INotifyCreated_Created>c__AnonStorey1.<>m__0 (OpenRA.Player player, System.Int32 playerIndex) [0x00034] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.Primitives.PlayerDictionary`1[T]..ctor (OpenRA.World world, System.Func`3[T1,T2,TResult] valueFactory) [0x00043] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Mods.Common.Traits.FrozenUnderFog.OpenRA.Traits.INotifyCreated.Created (OpenRA.Actor self) [0x00015] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.World.CreateActor (System.Boolean addToWorld, System.String name, OpenRA.Primitives.TypeDictionary initDict) [0x00023] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.World.CreateActor (System.String name, OpenRA.Primitives.TypeDictionary initDict) [0x00001] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Mods.Common.Traits.SpawnMapActors.WorldLoaded (OpenRA.World world, OpenRA.Graphics.WorldRenderer wr) [0x000a0] in <038f250609474dabb5adaa5cb1bda14a>:0 
  at OpenRA.World.LoadComplete (OpenRA.Graphics.WorldRenderer wr) [0x00080] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.StartGame (System.String mapUID, OpenRA.WorldType type) [0x000c3] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Network.UnitOrders.ProcessOrder (OpenRA.Network.OrderManager orderManager, OpenRA.World world, System.Int32 clientId, OpenRA.Order order) [0x005b9] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Network.OrderManager.TickImmediate () [0x00109] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Sync+<CheckSyncUnchanged>c__AnonStorey0.<>m__0 () [0x00001] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Sync.CheckSyncUnchanged[T] (OpenRA.World world, System.Func`1[TResult] fn) [0x00007] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Sync.CheckSyncUnchanged (OpenRA.World world, System.Action fn) [0x0000e] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x0010a] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.LogicTick () [0x00050] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.Loop () [0x000d6] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.Run () [0x00042] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00011] in <905b3a324e8a410ca7585982155ef6e8>:0 
  at OpenRA.Program.Main (System.String[] args) [0x0004f] in <905b3a324e8a410ca7585982155ef6e8>:0 

The game started when I used only one client and added an AI.

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from 026dd0b to 8902dfa Nov 29, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 29, 2018

Fixed. The crash was introduced in the last commit, and only happened if the explored map option was enabled.

@matjaeck
Copy link
Contributor

matjaeck left a comment

LGTM in-game 👍

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 1, 2018

Tested this on two comp stomps. No ground-breaking regressions spotted yet apart from the one mentioned in the end of the OP. Which IS a gameplay-changing thing, but one I think actually makes the game better as now your units won't have the knowledge about buildings that you don't have - if you see a building under fog you are able to attack it and can continue to attack it even if it was destroyed/sold. Shift-clicking defences with V2RLs and moving on won't work now, you'll have to bring in a scout as well or else your rockets will keep falling onto the cell that once was a flame tower you'd destroyed. Also, you can't send an infantry dispatch on AssaultMove against a base scouted before, because it will get stuck on the first under-fog building as it is able to fire further than it can see.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 1, 2018

I gather that we may want to rebalance weapon and/or sight ranges in order to fix that for "normal" infantry/vehicles before the playtest.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 1, 2018

A less disruptive option could be to change the targeting logic so that units move to within their sight range when firing on frozen actors (with a flag to disable it for artillery-style units)

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 1, 2018

OK, found a bug here. Likely to be connected with GPS.
Buildings (AA Gun and Power Plant) are under fog, yet I see their HP bars (setting is "show on damage"), and in dynamics even - I can see the AA Gun being repaired and/or damaged by my artillery. I also can select these buildings just fine.
image

It only concerns these two buildings, and not the other ones. I also was able to see the AA Gun disappear when it got destroyed, even without re-discovering it.

Next observation - when I targeted another Power Plant nearby with artilleries, it made possible to select it and see its health bar as well; same for a pillbox. But not for the refinery to the right.

Upd: AI got those buildings repaired, and I can't select them yet again, nor do I see the health bar. Probably due to me rediscovering the area with a longbow I guess.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 1, 2018

Yet another update for the GPS logic - I've seen the turret, and then it got under fog. Artilleries kept firing it (even after "stop" command) until it died and disappeared from under the fog. And artilleries still continue firing at the spot where the turret was even though it disappeared due to GPS not registering it anymore. Another stop command retargeted the arties onto a pillbox nearby, also under fog.

Same behavior occurs when a moving unit gets destroyed under fog - artillery keeps firing and GPS shows that there is nothing there.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 1, 2018

Being able to select buildings under fog (but not all of them):
openra-frozen-actors-1

Artillery fires onto a spot where harvester has died before, then is sent on AssaultMove top (notice how one piece moves forward while other keep shooting the barracks even though those are destroyed):
openra-frozen-actors-2

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 1, 2018

@netnazgul I expect that all of these regressions will be caused by the change in 8902dfa. Can you please try testing on 3d2687b (zip link if required) to confirm that those problems go away?

@matjaeck

This comment has been minimized.

Copy link
Contributor

matjaeck commented Dec 1, 2018

Good job @netnazgul (and sorry for my useless approving review, just don't take the stuff I write too seriously).

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from 8902dfa to bede0a8 Dec 1, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 1, 2018

Updated with fixes for all the points @netnazgul raised, but I haven't had time to test these thoroughly so would appreciate if you could repeat your testing and confirm no new regressions.

Units that don't explicitly define TargetFrozenActors: True, and which aren't being force-targeted, will now move within sight range rather than firing into fog.

Re lack of update rules: we can't access weapon stats from the actor updates, so theres no way to detect and warn about units that may need to have TargetFrozenActors enabled. I think the best way to handle this is with an entry in the SDK update notes, which is how we have handled similar changes in the past.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 2, 2018

I don't encounter the issues mentioned earlier anymore, so I consider that was fixed. Will conduct further testing later.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 2, 2018

Now units (except artillery) require either direct vision or active GPS to fire at buildings under fog. This will mean that you won't be able to attack turrets with infantry from a distance for example - they will automatically come closer to get into the vision range.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 3, 2018

You can use the force-attack modifier to do that.

@netnazgul

This comment has been minimized.

Copy link
Contributor

netnazgul commented Dec 3, 2018

No, you can't. If you use a force-attack on a building under fog, it changes to attack (it is confirmed by the building flashing) and the same logic applies.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 3, 2018

This is something that I took pains to implement and test, and as far as I can tell it works as it should. Can you please provide some specific testcases that fail?

The weapon vs sight range on most RA infantry isn't big enough for them to be able to consistently fire into the fog from all facings, but you can bump up the weapon range to unambiguously see it working. The attack cursor will switch from the out-of-range version to the in-range version when you hold down the modifier. Frozen actors flash when targeted, so I don't understand your comment there.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Dec 3, 2018

No, you can't. If you use a force-attack on a building under fog, it changes to attack (it is confirmed by the building flashing) and the same logic applies.

Im not experience this, just tested to attack a power plant under fog from the left side with a rocket solider in TD

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Dec 3, 2018

I think that MSAM (Rocket Launcher) should also have TargetFrozenActors: True defined in the TD mod.

Ping @AoAGeneral

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from bede0a8 to 8a494ff Dec 3, 2018

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from 5eb694a to 1a2b8e0 Dec 14, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 14, 2018

Updated.

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from 1a2b8e0 to b85dcb7 Dec 15, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 15, 2018

Added a RemoveAttackIgnoresVisibility update rule as requested.

@pchote pchote force-pushed the pchote:attack-frozen-actors branch 2 times, most recently from a6c220d to 3e2815a Dec 15, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 16, 2018

Updated to split the IgnoresVisibility changes into their own commit. Note that the commits tab and list above does not show the right commit order.

pchote added some commits Nov 28, 2018

Cache FrozenActorLayer on the Player object.
This avoids unnecessary trait queries.
Tweak FrozenActorLayer queries:
- FrozenActorsInRegion now filters for valid and (optionally) visible FAs
- Add new FrozenActorsInCircle to mirror World.FindActorsInCircle.

The first change means that SupportPowerDecision now correctly ignores
FrozenActors that the AI has not discovered.
Remove AttackBase.IgnoresVisibility.
This was a workaround for D2K sandworms, which is
now implemented using a custom attack activity.
Add Actor.ReplacedByActor to track transformations.
This isn't great conceptually, but has precedent
in the Generation number.
Allow Attack activities to target FrozenActors directly.
Removing the legacy FrozenActor to Actor workaround
fixes a number of long-standing bugs.

This also prevents units from losing their target when
it transforms into a different actor type.

@pchote pchote force-pushed the pchote:attack-frozen-actors branch from 3e2815a to 0054dd7 Dec 17, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 17, 2018

Fixed.

}
else if (target.Type == TargetType.FrozenActor)
{
if (attackStances == OpenRA.Traits.Stance.Enemy && self.Owner.Stances[target.FrozenActor.Owner] == OpenRA.Traits.Stance.Ally)

This comment has been minimized.

@obrakmann

obrakmann Dec 17, 2018

Contributor

How about a Target.AppearsHostileTo()?

This comment has been minimized.

@pchote

pchote Dec 17, 2018

Member

What would this return for location and invalid targets?

This comment has been minimized.

@obrakmann

obrakmann Dec 17, 2018

Contributor

false, I guess. But it was just an off-the-cuff suggestion, so feel free to ignore it

This comment has been minimized.

@pchote

pchote Dec 17, 2018

Member

Ok. Lets leave it as is for now, then we can reevaluate this if we find a second usecase for such a method.

[Desc("Does not care about shroud or fog. Enables the actor to launch an attack against a target even if he has no visibility of it.")]
public readonly bool IgnoresVisibility = false;
[Desc("Allow firing into the fog to target frozen actors without requiring force-fire.")]
public readonly bool TargetFrozenActors = false;

This comment has been minimized.

@obrakmann

obrakmann Dec 17, 2018

Contributor

Are there examples where Actors have longer firing range than vision where we would disable this? Or in other words, can we take the burden of remembering having to set this from the modder and automatically detemine it via code instead?

This comment has been minimized.

@pchote

pchote Dec 17, 2018

Member

Unfortunately yes, several. RA infantry set a weapon range ~1 cell larger than their range, so depending on their subcell and angle of attack they can sporadically get hung-up on frozen dead buildings if assault-moved into an enemy base.

@obrakmann
Copy link
Contributor

obrakmann left a comment

Let's merge it then, see what falls out.

@obrakmann obrakmann merged commit 224377f into OpenRA:bleed Dec 17, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

obrakmann commented Dec 17, 2018

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