UnitAI and BuildingAI should deal with which targets we should try to attack first (e.g. typically humans before structures) and Attack should deal with whether we can attack a target and which type of attack to use (e.g. for structures try capture before melee/ranged). Currently the two types of preferences are mixed together, and this patch separates them.
Details
Details
- Reviewers
bb
Currently slaughter and capture are hard-coded in Attack, but this will allow us to treat them more generally in the future.
The target selection algorithms in UnitAI and BuildingAI need work, and moving preferred classes to those components is a first step.
Mauryan warriors and Iberian flaming skirm cav deal crush damage but I kept their preference to capture buildings.
Check that I didn't miss any templates.
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 4112 Build 7229: Vulcan Build Jenkins Build 7228: arc lint + arc unit
Event Timeline
Comment Actions
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...
| binaries/data/mods/public/simulation/components/Attack.js | ||
|---|---|---|
| 25 | We should mention here exactly what the tag (should) do, that is choosing the attackType based on the entity Also this tag doesn't do anything (correct?) in the proposed code, however we plan on using it => I will make a dependant revision fixing that. | |
| 288 | swapping this function to unitAI is indeed correct, however when required by other code (thus not in this patch) there could be an attack component equivalent returning something sensible with the new definitions. But for now this is ok. | |
| binaries/data/mods/public/simulation/components/BuildingAI.js | ||
| 23 | delimited => seperated?, period | |
| 281 | period | |
| 285 | Why not store it in the init? | |
| 287–289 | looks like ternary candidate | |
| 298 | Why could very simply allow classes like human+unit here by using matchesClassList instead, also to allow spaces (or's) too we could simply use another delimiter than space and just keep the space in the class. | |
| 351 | we crash on this line since we are in a made function, I guess we should just nuke the function and inline it in the loop below (by concenating the unitaitarget to the target list) | |
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
| 60 | delimited => separated, period also there it is probably used for healing and stuff instead of just attacking | |
| 5596–5598 | put returns on a new line | |
| 5625 | isn't an arrow function possible? | |
| 6034 | Comments => see buildingAI Kinda sad we have this same function in two places now, but no easy way around I guess... | |
| binaries/data/mods/public/simulation/templates/template_unit_cavalry.xml | ||
| 13–22 | correct, a later patch can nuke the hardcodings then in the attack component | |
| binaries/data/mods/public/simulation/templates/template_unit_cavalry_melee.xml | ||
| 15 | There is one downside on setting the things here explicitly: two different unit types can react differently when giving the same order due to some template value (which can have an arbitrary value). However we have I planned to set hotkeys for different attack types, and maybe some on/off switch per attack type can solve these problems. So IMO not a big problem. | |
| binaries/data/mods/public/simulation/templates/template_unit_cavalry_ranged.xml | ||
| 16 | line doesn't seem that wrong | |
| binaries/data/mods/public/simulation/templates/template_unit_champion.xml | ||
| 32 | so we will ignore structures to the last moment, hmmm its a gameplay important change but ok with me | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_cavalry_archer.xml | ||
| 16 | also this one doesn't look that wrong | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_cavalry_javelinist.xml | ||
| 4–6 | Out of scope: sad we have no champ_cav_ranged template :/ | |
| 16 | same | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_elephant_melee.xml | ||
| 17 | so we ignore ships and relics, why? | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_archer.xml | ||
| 16 | doesn't look wrong | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_javelinist.xml | ||
| 16 | well etc. | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_pikeman.xml | ||
| 22 | So pikes will always automatically use melee attack on sieges, hmmm don't know if we want that | |
| binaries/data/mods/public/simulation/templates/template_unit_dog.xml | ||
| 14–15 | good | |
| binaries/data/mods/public/simulation/templates/template_unit_hero.xml | ||
| 67 | so structures last, can be ok | |
Comment Actions
I guess we should be careful about the new preferred target classes. I like the idea of matching classes, so then melee units can attack the nearest siege or human, rather than first humans then siege or first siege then humans. Let me think about it.
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 6034 | The alternative is to put it in the attack component, which was my original plan. | |
| binaries/data/mods/public/simulation/templates/template_unit_cavalry_ranged.xml | ||
| 16 | We need PreferredClasses only when we have to choose between two attacks, and currently there's only capture, slaughter, ranged, and we can't capture or slaughter Human, so it's unnecessary. | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_elephant_melee.xml | ||
| 17 | Not ignored, just after everything else. | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_pikeman.xml | ||
| 22 | Why not? | |
Comment Actions
I forgot to add some of my consideration to that: the template list works with a space separated list, so using another delimiter there is not too trivial, so perhaps only allowing the "+" is possible now. However that would be somewhat nontrivial since we allow half of a feature, which probably isn't any better than not doing it at all.
| binaries/data/mods/public/simulation/components/UnitAI.js | ||
|---|---|---|
| 6032 | period | |
| 6034 | both options look worse indeed | |
| binaries/data/mods/public/simulation/templates/template_unit_cavalry_ranged.xml | ||
| 16 | true | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_elephant_melee.xml | ||
| 17 | y, ok, but why them after everything else? | |
| binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_pikeman.xml | ||
| 22 | That indeed is a good question too... | |