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

Base AttackAircraft on AttackFollow and get rid of SequenceActivities. #16369

Merged
merged 2 commits into from May 4, 2019

Conversation

@tovl
Copy link
Contributor

commented Mar 30, 2019

This removes the last instance of SequenceActivities(..., this) to be replaced by childactivities (when #16241 and #16193 get merged that is).

Getting rid of this pattern in FlyAttack required that the actual DoAttack be placed outside the activity, similar to AttackFollow. So, I've based AttackAircraft on AttackFollow instead of AttackFrontal and duplicated the required bits of AttackFrontal. This has the added benefit of making opportunity fire available for aircraft as well.

Fixes #14193

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

I suspect we may want to disable opportunity fire by default on aircraft. It feels really weird in TD (which doesn't have limited ammo, so doesn't default to hold fire) because apaches will fire on anything immediately in front of it when flying overhead, but not anything even slightly off to the side.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

That is probably because FacingTolerance is set to 0 by default. tweaking that number should give it a reasonable firing cone.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

It turns out that wasn't the problem. The problem was that the aircraft didn't check whether an opportunity target was in it's firing arc; It only checked when it tried to actually fire. So it would keep targeting a target that it couldn't reach anymore. This should now be fixed.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Agree with pchote to disable opportunity fire by default on aircraft, it does not feel right in RA when ammunition is a rare commodity and units fire rather unpredictable. I find it also harder to micro MiGs with opportunity fire than with direct targeting. Personally I find the concept good for turreted units (decoupled moving / targeting) but don't really like it for units that can only attack frontal.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

I agree that it's use is probably limited in the default mods due to ammo conservation and no one-hit kills, so I'm fine with having it disabled by default if that is the consensus. However, considering that aircraft are on hold-fire stance by default, what is actually the disadvantage to allowing it?

Another question: If opportunity fire is disabled, sticky targeting should then also be disabled, correct? Maybe even put it on the same yaml parameter?

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

However, considering that aircraft are on hold-fire stance by default, what is actually the disadvantage to allowing it?

Stances don't work for airplanes the last time I checked in #14193. They seem to be generally fixed here though but didn't test in depth. There is the issue that once you target something, opportunity fire will stay active for this target regardless of hold fire stance if you don't cancel the targeting with a stop order (probably related to #8373).

I also found it more intuitive to target with an explicit attack order than to rely on the passive attacking while giving move orders.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Stances don't work for airplanes the last time I checked in #14193. they seem to be fixed here though.

Ironically, they are only fixed with OpportunityFire enabled (except for Aggressive, which now acts the same as Defensive ). This is because aircraft are never truly idle and opportunity fire is in essence just applying the autotarget behaviour to non-idle actors.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

except for Aggressive, which now acts the same as Defensive

You mean attack anything? Perhaps I misunderstood, but this appears to work as intended: units attack buildings but do not when in defend stance.

The general question about what to do here needs input form other people, my concern is that while fixing stances, it can cause unexpected behavior and frustration when attacking the wrong target. A solution might be to disable opportunity fire for hold fire (which is in itself a contradictory combination) and only allow direct targeting like now.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

You mean attack anything? Perhaps I misunderstood, but this appears to work as intended: units attack buildings but do not when in defend stance.

Yes, I meant attack anything. I forgot about the buildings part; the chasing part does not work.

A solution might be to disable opportunity fire for hold fire (which is in itself a contradictory combination)

It is already disabled for hold fire, hence my point about hold-fire being the default. Unless you mean the sticky targeting part?

@tovl tovl force-pushed the tovl:refactor-attackfly branch from c97d7c1 to 9213dde Apr 2, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Yes, I meant attack anything. I forgot about the buildings part; the chasing part does not work.

Not a problem with aircraft IMO.

It is already disabled for hold fire, hence my point about hold-fire being the default. Unless you mean the sticky targeting part?

Don't know the terminology but this sounds like what I mean (that they will fire on a once targeted enemy unit when they get in range again ). If you can disable that, then I personally see no reason why not to enable opportunity fire / sticky targeting on other stances because it wouldn't change the current behavior but only add new features (in RA).

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Removed the sticky targeting part now.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Ok, this works now really nice in RA I think. Yaks could finally become a viable tool to attack blobs of infantry (well rocket soldiers still murder them) because you can now control the strafe with move orders while attacking and targeting works automatically. In situations where you need precision, you have hold fire and direct targeting like before. I like this a lot but let's see what others say.

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

We already tried adding strafe logic to yaks, but had to remove it again after strong negative feedback - #13651. I've tried raising this idea again several times since then, and opinion does not seem to have changed within the core MP community.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

Looking at the comments in that PR and the previous PR it doesn't strike me as strong negative feedback, just mild concerns about the reduced ability to concentrate fire on a single target. That concern is not warranted here, because this changes nothing about the attack behaviour in the default stance. It only adds new behaviour in the more aggresive stances that is in line with how stances work for other units (whereas up till now the more aggresive stances didn't function at all). This is a win-win.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

What tovl said, I was a bit too enthusiastic in my last comment.

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Copied from #16382:

There seem to be problems with the facing of non-turreted units:

facings

The issue does not occur on bleed.

@tovl tovl force-pushed the tovl:refactor-attackfly branch 3 times, most recently from 00af395 to ff39a52 Apr 4, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Should be fixed now.

@matjaeck
Copy link
Contributor

left a comment

Works as advertised. 👍

This was referenced Apr 11, 2019
@pchote
Copy link
Member

left a comment

I haven't looked at this in detail yet, but one issue immediately stuck out when giving the code a quick skim:

@tovl tovl force-pushed the tovl:refactor-attackfly branch from ff39a52 to ad12ea3 Apr 22, 2019

mods/cnc/rules/defaults.yaml Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:refactor-attackfly branch 2 times, most recently from c8ef755 to 018c728 Apr 28, 2019

OpenRA.Mods.Common/Activities/Air/FlyAttack.cs Outdated Show resolved Hide resolved
@@ -31,15 +31,15 @@ public class AttackLeapInfo : AttackFrontalInfo, Requires<MobileInfo>

public class AttackLeap : AttackFrontal
{
readonly AttackLeapInfo info;
readonly AttackLeapInfo leapInfo;

This comment has been minimized.

Copy link
@pchote

pchote May 4, 2019

Member

What is the purpose of this change?

This comment has been minimized.

Copy link
@tovl

tovl May 4, 2019

Author Contributor

Clarity and consistency with AttackAircraft (and AttackFrontal but that is not relevant anymore).

This comment was marked as resolved.

Copy link
@pchote

pchote May 4, 2019

Member

We could then drop the changes here if we take the second option.

This comment has been minimized.

Copy link
@tovl

tovl May 4, 2019

Author Contributor

Reverted.

OpenRA.Mods.Common/Activities/Air/FlyAttack.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/AttackBomber.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackFollow.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackFrontal.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackFrontal.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/AutoTarget.cs Show resolved Hide resolved
mods/ra/rules/infantry.yaml Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented May 4, 2019

Enabling opportunity fire on the TD helicopters is a potentially disruptive change to the default unit behaviour, so i'd prefer if we could keep disable it for them here and then consider the gameplay implications of enabling it in a future issue and/or PR.

@tovl tovl force-pushed the tovl:refactor-attackfly branch from 018c728 to be247d0 May 4, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Yes, the TD behaviour still feels a bit awkward. It will probably require changes to other weapon characteristics to get the right feel. I've disabled it for now, and I'll maybe take a look at it again after the upcoming Attack refactor.

@tovl tovl force-pushed the tovl:refactor-attackfly branch from be247d0 to ab8f273 May 4, 2019

@pchote
Copy link
Member

left a comment

I think we're just about good here. Only a couple of minor new nits:

OpenRA.Mods.Common/Activities/Air/FlyAttack.cs Outdated Show resolved Hide resolved
@@ -31,15 +31,15 @@ public class AttackLeapInfo : AttackFrontalInfo, Requires<MobileInfo>

public class AttackLeap : AttackFrontal
{
readonly AttackLeapInfo info;
readonly AttackLeapInfo leapInfo;

This comment was marked as resolved.

Copy link
@pchote

pchote May 4, 2019

Member

We could then drop the changes here if we take the second option.

OpenRA.Mods.Common/Activities/Air/FlyAttack.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/HeliAttack.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackFollow.cs Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:refactor-attackfly branch from ab8f273 to 26da50f May 4, 2019

@pchote
pchote approved these changes May 4, 2019

@pchote pchote added the PR: Needs +2 label May 4, 2019

@matjaeck
Copy link
Contributor

left a comment

Tested in RA and looks good to me. Opportunity fire for Yaks and MiGs adds more options how you can micro them and stances become finally useful for aircraft.

Thoughts for future PRs: usability of stances could further be improved to be more reliable - here air units only attack in Defend stance for example when they are lucky enough to face towards the enemy.

@reaperrr reaperrr merged commit 9abf715 into OpenRA:bleed May 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.