Page MenuHomeWildfire Games

Move a health check from UnitAI to Attack and remove forceResponse
ClosedPublic

Authored by temple on Dec 2 2017, 2:09 AM.

Details

Summary

The Attack component should deal with whether some entity is a valid target, if it's an enemy and has the right classes and we can hurt it. UnitAI and UnitMotion should deal with the questions of behavior, how to move within range of the target and whether to chase it, and so on. The extra diplomacy, health, and capture checks in UnitAI aren't necessary and make the code longer and harder to understand.

Ceasefire was introduced with #2749 and there it was decided that the easiest way to handle that game mode was to make the players neutral and disallow them from attacking each other. At that time, if you were attacked by a neutral player (who must have had you set as an enemy), then your units would fight back, but you couldn't order them to attack like before. (I actually didn't play the game during this time, but this is my understanding of the code and discussion at #7, for example.)

This behavior was changed with rP19528 because a diplomacy check for enemies was added to CanAttack of Attack, and now when a neutral player attacks you (which means they have you set as an enemy), your units won't respond, they'll just die.

One solution to this is to fix that bug. But instead I think this is already a weird situation (which apparently has gone unnoticed). If an opponent marks you as an enemy then automatically your diplomacy stance towards them is set to enemy too, so if they attack you you'll fight back. It's only if you purposely change them to neutral or ally after that that they can attack you without being attacked back. So I feel like a player who does that already had warning and can't complain when he gets attacked.

Instead of keeping around the forceResponse code to deal with that weird situation, I think it's better to just say you can't attack neutral players, only enemies.

Test Plan

I hoped that the hashes would be the same with and without the patch, but I played a game with petra and the final state was different. So I'll have to check and see if I missed something.

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.Dec 2 2017, 2:09 AM
temple added a reviewer: Restricted Owners Package.Dec 2 2017, 2:15 AM
Vulcan added a subscriber: Vulcan.Dec 2 2017, 2:58 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
temple added a comment.Dec 2 2017, 3:50 PM

We go through all the attacks in CanAttack and then basically do the same thing in GetBestAttackAgainst, so it seems better to just have one function.

temple updated this revision to Diff 4493.Dec 2 2017, 10:53 PM

Unified CanAttack and GetBestAttackAgainst.

Test is slightly broken.
Currently PreferredClasses are split by attack type, but I think instead it's better to decide on the target first and later figure out the best way to attack it.
My goal is to unify the target selection functions from BuildingAI and UnitAI (both regular attack and walk-and-fight), so I'll try to do that before fixing the test.

bb added a comment.EditedDec 2 2017, 10:56 PM

Patch looks nice on first sight, but requires further review and testing though.

In D1099#43532, @temple wrote:

We go through all the attacks in CanAttack and then basically do the same thing in GetBestAttackAgainst, so it seems better to just have one function.

The observation that getbestattack and canattack do not really work well together is correct. (they duplicate some code indeed), So getbestattack could call canattack a couple of times (see #252), however they are different functions as canattack should return the possibility of attack with some constraint and getbestattack should always return a type (when it is possible to attack with one ofc), which potentially does not fall under the constraint. Also for performance reasons it is good to keep two separated function.

EDIT: comment was based on previous diff (not newest)

bb added inline comments.Dec 2 2017, 11:09 PM
binaries/data/mods/public/simulation/components/Attack.js
3 ↗(On Diff #4493)

there were reasons to not add this, maybe has changed though

224–227 ↗(On Diff #4493)

(in this approach this function is simply useless), however earlier comment

232 ↗(On Diff #4493)

doing something with wantedtypes here is indeed the plan

234–236 ↗(On Diff #4493)

Nuking the canattack function this way does not work imo (when seeing some of my planned changes), one could however nuke a lot of duplication in getbestattack with calling canAttack and making some list of that and use that as types
(see the pastebin I sended for some ideas perhaps)

238 ↗(On Diff #4493)

probably it is done somewhere under the hood, but ents do things on there own too

241 ↗(On Diff #4493)

I guess one of the reasons for not adding slaughter in the global list => this could be slaughter

268 ↗(On Diff #4493)

(Do notice multiple melee/ranged attacks should be allowed too f.e. sword + pike)

269 ↗(On Diff #4493)

but we don't want units to attack rams with arrows when they have daggers...

276 ↗(On Diff #4493)

probably true

285 ↗(On Diff #4493)

giant hardcoding of the types, nukes the whole point of #252, so nope

Vulcan added a comment.Dec 3 2017, 3:39 PM

Build was aborted.

Updating workspaces...
Updating workspaces failed!
GCC build (release)...
GCC build (debug)...
In file included from ../../../source/ps/UniDoubler.h:55:0,
                 from ../../../source/ps/CStr.h:40,
                 from ../../../source/pch/engine/precompiled.h:29:
../../../source/ps/CStr.h:347:1: fatal error: can’t write PCH file: No space left on device
 }
 ^
compilation terminated.
Preprocessed source stored into /tmp/ccQxAJUx.out file, please attach this to your bugreport.
make[1]: *** [obj/engine_Debug/precompiled.h.gch] Error 1
make: *** [engine] Error 2
make: *** Waiting for unfinished jobs....
/tmp/ccSouJzG.s: Assembler messages:
/tmp/ccSouJzG.s: Fatal error: can't write obj/simulation2_Debug/CCmpTerrain.o: No space left on device
/tmp/ccSouJzG.s: Fatal error: can't close obj/simulation2_Debug/CCmpTerrain.o: No space left on device
make[1]: *** [obj/simulation2_Debug/CCmpTerrain.o] Error 1
make[1]: *** Waiting for unfinished jobs....
/tmp/ccg5TLTB.s: Assembler messages:
/tmp/ccg5TLTB.s: Fatal error: can't write obj/simulation2_Debug/CCmpFootprint.o: No space left on device
/tmp/ccg5TLTB.s: Fatal error: can't close obj/simulation2_Debug/CCmpFootprint.o: No space left on device
make[1]: *** [obj/simulation2_Debug/CCmpFootprint.o] Error 1
make: *** [simulation2] Error 2
Running release tests...
Running cxxtest tests (308 tests).
In TestComponentScripts::test_scripts:
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:255:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:230:2
@simulation/components/tests/test_Attack.js:259:1
Expected equal, got (void 0) !== "Melee"
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:255:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:230:2
@simulation/components/tests/test_Attack.js:259:1
Expected equal, got (void 0) !== "Melee"
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:255:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:230:2
@simulation/components/tests/test_Attack.js:260:1
Expected equal, got (void 0) !== "Ranged"
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:255:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:230:2
@simulation/components/tests/test_Attack.js:260:1
Expected equal, got (void 0) !== "Ranged"
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:219:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:202:2
@simulation/components/tests/test_Attack.js:261:1
Expected equal, got false !== true
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:227:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:202:2
@simulation/components/tests/test_Attack.js:261:1
Expected equal, got "Melee" !== "Slaughter"
../../../source/test_setup.cpp:134: Error: Test failed: Stack trace:
testGetBestAttackAgainst/<@simulation/components/tests/test_Attack.js:227:1
attackComponentTest@simulation/components/tests/test_Attack.js:127:2
testGetBestAttackAgainst@simulation/components/tests/test_Attack.js:202:2
@simulation/components/tests/test_Attack.js:261:1
Expected equal, got "Melee" !== "Slaughter"
../../../source/test_setup.cpp:134:
temple added a comment.Dec 3 2017, 3:50 PM
In D1099#43669, @Vulcan wrote:

../../../source/ps/CStr.h:347:1: fatal error: can’t write PCH file: No space left on device
/tmp/ccSouJzG.s: Fatal error: can't write obj/simulation2_Debug/CCmpTerrain.o: No space left on device
/tmp/ccSouJzG.s: Fatal error: can't close obj/simulation2_Debug/CCmpTerrain.o: No space left on device
/tmp/ccg5TLTB.s: Fatal error: can't write obj/simulation2_Debug/CCmpFootprint.o: No space left on device
/tmp/ccg5TLTB.s: Fatal error: can't close obj/simulation2_Debug/CCmpFootprint.o: No space left on device

Is that normal?
I knew the attack test would fail, should I have done something differently (like not upload the patch)?

binaries/data/mods/public/simulation/components/Attack.js
269 ↗(On Diff #4493)

That's a good point.

Itms added a subscriber: Itms.Dec 3 2017, 4:02 PM
In D1099#43673, @temple wrote:

Is that normal?

No that is Jenkins not having space left, I fixed that. However why would the test fail? Is it fixable? You can cancel builds if you know they are useless (but that means the patch is not committable). Should I restart the build or do you want to upload a new revision?

temple added a comment.Dec 3 2017, 4:12 PM
In D1099#43675, @Itms wrote:
In D1099#43673, @temple wrote:

Is that normal?

No that is Jenkins not having space left, I fixed that. However why would the test fail? Is it fixable? You can cancel builds if you know they are useless (but that means the patch is not committable). Should I restart the build or do you want to upload a new revision?

The test fails because it was too much work to fix it, but I still wanted to upload the patch to get feedback. I suppose I could have deleted or commented out the bad parts of the test, so I'll do that next time.

temple updated this revision to Diff 4770.Dec 13 2017, 10:41 PM

Revert to the previous diff. The CanAttack/GetBestAttackAgainst stuff should be put in a separate patch.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
bb accepted this revision as: bb.Dec 23 2017, 4:33 PM
bb removed a reviewer: Restricted Owners Package.

forceRespond is a relic from before I ever looked at the unitAI, trying to interfere with the "force" tag in an undefined way, so removing is good.

Those few arguments can be done when committing.

binaries/data/mods/public/simulation/components/Attack.js
266 ↗(On Diff #4770)

the checks indeed point in the correct direction

binaries/data/mods/public/simulation/components/UnitAI.js
4682–4688 ↗(On Diff #4770)

remove the true's here

This revision is now accepted and ready to land.Dec 23 2017, 4:33 PM
This revision was automatically updated to reflect the committed changes.