Page MenuHomeWildfire Games

Fix individual combat when attacked
ClosedPublic

Authored by temple on Nov 18 2017, 8:57 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20568: Fix individual combat when attacked
Summary

A few problems:

  • If you run out of enemies you'll start to capture buildings, but then if ranged units ungarrison and start attacking you, you'll ignore them and continue to capture. This is pretty annoying.
  • There's an obsolete SortEntitiesByPriority function, which for example says that if you're attacking melee infantry and get attacked by melee cavalry, then switch to attacking the cavalry, but if you're attacking melee cavalry and get attacked by melee infantry, then continue to attack the cavalry. Instead in these situations we should always just continue attacking our current target.
  • The chasing section is missing the Attacked part, so if you're attacking a unit and they run away, while you chase them melee units can attack you and you'll continue to chase instead of stopping and attacking the new enemy.

The fixes:

  • When we're attacking, if we're capturing (unforced) and are attacked by some other unit, then we should stop capturing and instead attack the unit.
  • When we're approaching or chasing (unforced) and are attacked by a melee unit, then we should stop and instead attack this unit, because our original target might be protected by melee units and otherwise we could die before ever reaching them. (We shouldn't respond to ranged attacks though, because those units could be farther away than our original target. But maybe we should add the capture cases here too.)
  • targetAttackersPassive is always true so I don't see any point in keeping it around. I'm not sure I'm a fan of targetAttackersAlways (used in violent stance to override forced commands), but I left it in.

Ideally units should have attacking preferences, e.g. melee first then capture, so it wouldn't have to be hard-coded. (Maybe in the future we'd have capture specialists who have melee as a backup attack, for example.) We have target preference classes, but it seems like there's issues with that because it's per attack type and the Attack.js code doesn't really appreciate that. I think this stuff needs to be rethought, is what I'm trying to say. (Currently it doesn't matter since capture has special coding everywhere and no unit has both melee and ranged attacks, and those are the only three attack types.)

(It looks like walk-and-fight creates forced attacks, which might not be what we want. I'll try to look into that later.)

History at rP9663 and rP11798, maybe more.

Test Plan

Test and agree with the behavior changes.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

temple created this revision.Nov 18 2017, 8:57 PM
temple edited the summary of this revision. (Show Details)Nov 18 2017, 9:00 PM
Stan added a subscriber: Stan.EditedNov 18 2017, 11:27 PM

Hey, sounds nice.

Can you add some tests as well ? UnitAI is a big mess, and the more we can assert how it works, the easiest it would be not to break other stuff in the future.

leper added a reviewer: Restricted Owners Package.Nov 19 2017, 9:46 AM
leper added a subscriber: leper.

SortEntitiesByPriority

I actually have a TODO comment about that being mostly unneeded, and too complicated for that one use, given that it could be done in linear time (finding the best target) locally. (There's also a comment about AttackEntitiesByPreference doing too much work for what it is used for, in case you are looking for more things to do.)

(Also yes, stances are still quite hacky in some ways. It would also be nice to have a few more, for e.g. siege engines that might want different behaviour than other units. There's a ticket or 3 somewhere.)

Code looks mostly ok, then again someone will have to decide if that behaviour change is what we want, if that Melee/Capture ordering hard-coding is fine, and if there are no unexpected side-effects.

binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

I guess that 'Melee' (and 'Capture' below) hard-coding isn't too bad. (Also this hunk is only moved.)

The original comment was also nice. (Having a reason for doing that as opposed to just stating what we are doing.)

2035 ↗(On Diff #4268)

"unit" seems a bit specific after the generic opening.

2055 ↗(On Diff #4268)

Yes, that "stronger" was mostly a lie.

wraitii added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

It would be rather nice to someday be name-of-attack agnostic so that people could have various attack types, including melee and such. Agreed on the commenting part.

leper added inline comments.Nov 19 2017, 11:52 AM
binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

I actually had part of that as a comment (though mostly because I initially confused damage types with attack types (#4801)), and it would be nice to have that at some point, but at least the melee/ranged part will need to stay in one way or another. (And that assumes that we can actually make capture (or the current capture) a melee attack.)

wraitii added inline comments.Nov 21 2017, 1:18 PM
binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

it would be nice to have that at some point, but at least the melee/ranged part will need to stay in one way or another. (And that assumes that we can actually make capture (or the current capture) a melee attack.)

I'm not too sure about that. The big difference is that Ranged involves a projectile, but there's no reason why melee attacks couldn't have a Minimal range or an Elevation Bonus or splash. In fact a "beam" weapon in 0 A.D. could have all of those but ought to be "melee" in the current system.

Anyways, that's probably a project for another day.

Capture is a bit of a different beast since it doesn't deal damage.

bb added a subscriber: bb.Nov 21 2017, 6:24 PM
bb added inline comments.
binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

All of that is planned for my #252 approach, even capture is made more consistent with that, as in:

  • "normal" and capture can be combined in the same attack
  • there can be projectiles with capture
  • any attack can have splash and min range and whatever values there are more
  • Splash can include capture

The hardCoding of melee, isn't too worse (yet should be changed in #252 ofc) since there can be rather simple function determining if the attack deals damage and is "close enough" to react uppon (why not react also to very short ranged ranged attacks? => later patch ofc)

In D1047#41365, @Stan wrote:

Can you add some tests as well ? UnitAI is a big mess, and the more we can assert how it works, the easiest it would be not to break other stuff in the future.

That might be nice, but currently there's not any tests (besides two formations), so it should be done for everything and I don't have the energy for that.

binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

I didn't think about ranged/projectile/splash capture, that sounds interesting. What about those things for heal too?

On the other hand, I'm not sure about combining normal and capture in the same attack, seems messy.

I spent some time yesterday splitting up capture from attack (adding a Capture component, adding a capturable range query and a CAPTURE state to UnitAI), and it went pretty smoothly. The difficulty though was in figuring out which things should have precedence. For example, if we're given some generic "attack" order, we might want to attack units, then capture buildings, then attack buildings. But how to represent that? And then there's healers, who potentially might have attack or capture properties. And maybe we want repair to be included in the mix too, since that's similar to heal. Attack is against enemies, heal/repair are for allied units/structures, and capture is for both.

So I got stuck at this point.

I suppose I should look at #252 more closely.

Stan added a comment.Nov 21 2017, 8:08 PM

Yeah I understand. Maybe you could write a scenario on how it supposed to work ? So people like me can write them afterwards.

Since you're bringing #252 up I'd like to remind that there is also #2577 that allow multiple attacks on a single units by introducing the concept of turret.

wraitii accepted this revision.Nov 25 2017, 2:56 PM

See leper's comments before committing, but the changes are nice imo.

binaries/data/mods/public/simulation/components/UnitAI.js
1800 ↗(On Diff #4268)

That sounds quite good to me, agreed on the last bit.

Indeed the main advantage is having several melee and/or ranged attacks.

This revision is now accepted and ready to land.Nov 25 2017, 2:56 PM
temple updated this revision to Diff 4381.Nov 26 2017, 2:03 AM

Improved comments.

This revision was automatically updated to reflect the committed changes.