Page MenuHomeWildfire Games

[ai/petra] replace Cavalry with new FastMoving class
ClosedPublic

Authored by Nescio on Sep 2 2019, 8:56 PM.

Details

Reviewers
Angen
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23916: Use new FastMoving class instead of Cavalry in AI/petra.
Summary

This patch introduces a new (non-visible) FastMoving class, to be used by the AI instead of Cavalry. The advantages of this include:

  • cavalry can be used by auras and technology specifically for true cavalry units
  • camels, chariots, elephants could be separated from cavalry templates
  • mods can introduce zebras, unicorns, pegasi, whatever without having to label them with a Cavalry class
  • necessary for the AI to be able to train (Briton) war dogs (which does not mean they will train them; merely they could, in theory)

[EDIT]: CitizenSoldier vs Champion is a false dichotomy and could be replaced with !Champion vs Champion; perhaps something for a future patch.

Test Plan

Check for mistakes and omissions.

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

Nescio created this revision.Sep 2 2019, 8:56 PM
Owners added a subscriber: Restricted Owners Package.Sep 2 2019, 8:56 PM
Vulcan added a comment.Sep 2 2019, 8:57 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/53/display/redirect

Vulcan added a comment.Sep 2 2019, 9:00 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 0.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/baseManager.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/baseManager.js
| 192| 192| 		medium.sort((r1, r2) => r1.dist - r2.dist);
| 193| 193| 		faraway.sort((r1, r2) => r1.dist - r2.dist);
| 194| 194| 
| 195|    |-/*		let debug = false;
|    | 195|+		/*		let debug = false;
| 196| 196| 		if (debug)
| 197| 197| 		{
| 198| 198| 			faraway.forEach(function(res){
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 136| 136| 	{
| 137| 137| 		priority = 90;
| 138| 138| 		// basically we want a mix of citizen soldiers so our barracks have a purpose, and champion units.
| 139|    |-		this.unitStat.RangedInfantry    = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Ranged", "CitizenSoldier"],
|    | 139|+		this.unitStat.RangedInfantry = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Ranged", "CitizenSoldier"],
| 140| 140| 			"interests": [["strength", 3]] };
| 141| 141| 		this.unitStat.MeleeInfantry     = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Melee", "CitizenSoldier"],
| 142| 142| 			"interests": [["strength", 3]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 138| 138| 		// basically we want a mix of citizen soldiers so our barracks have a purpose, and champion units.
| 139| 139| 		this.unitStat.RangedInfantry    = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Ranged", "CitizenSoldier"],
| 140| 140| 			"interests": [["strength", 3]] };
| 141|    |-		this.unitStat.MeleeInfantry     = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Melee", "CitizenSoldier"],
|    | 141|+		this.unitStat.MeleeInfantry = { "priority": 0.7, "minSize": 5, "targetSize": 20, "batchSize": 5, "classes": ["Infantry", "Melee", "CitizenSoldier"],
| 142| 142| 			"interests": [["strength", 3]] };
| 143| 143| 		this.unitStat.ChampRangedInfantry = { "priority": 1, "minSize": 3, "targetSize": 18, "batchSize": 3, "classes": ["Infantry", "Ranged", "Champion"],
| 144| 144| 			"interests": [["strength", 3]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 142| 142| 			"interests": [["strength", 3]] };
| 143| 143| 		this.unitStat.ChampRangedInfantry = { "priority": 1, "minSize": 3, "targetSize": 18, "batchSize": 3, "classes": ["Infantry", "Ranged", "Champion"],
| 144| 144| 			"interests": [["strength", 3]] };
| 145|    |-		this.unitStat.ChampMeleeInfantry  = { "priority": 1, "minSize": 3, "targetSize": 18, "batchSize": 3, "classes": ["Infantry", "Melee", "Champion"],
|    | 145|+		this.unitStat.ChampMeleeInfantry = { "priority": 1, "minSize": 3, "targetSize": 18, "batchSize": 3, "classes": ["Infantry", "Melee", "Champion"],
| 146| 146| 			"interests": [["strength", 3]] };
| 147| 147| 		this.unitStat.RangedMounted     = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Ranged", "CitizenSoldier"],
| 148| 148| 			"interests": [["strength", 2]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 144| 144| 			"interests": [["strength", 3]] };
| 145| 145| 		this.unitStat.ChampMeleeInfantry  = { "priority": 1, "minSize": 3, "targetSize": 18, "batchSize": 3, "classes": ["Infantry", "Melee", "Champion"],
| 146| 146| 			"interests": [["strength", 3]] };
| 147|    |-		this.unitStat.RangedMounted     = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Ranged", "CitizenSoldier"],
|    | 147|+		this.unitStat.RangedMounted = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Ranged", "CitizenSoldier"],
| 148| 148| 			"interests": [["strength", 2]] };
| 149| 149| 		this.unitStat.MeleeMounted      = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Melee", "CitizenSoldier"],
| 150| 150| 			"interests": [["strength", 2]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 146| 146| 			"interests": [["strength", 3]] };
| 147| 147| 		this.unitStat.RangedMounted     = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Ranged", "CitizenSoldier"],
| 148| 148| 			"interests": [["strength", 2]] };
| 149|    |-		this.unitStat.MeleeMounted      = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Melee", "CitizenSoldier"],
|    | 149|+		this.unitStat.MeleeMounted = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Melee", "CitizenSoldier"],
| 150| 150| 			"interests": [["strength", 2]] };
| 151| 151| 		this.unitStat.ChampRangedMounted  = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Ranged", "Champion"],
| 152| 152| 			"interests": [["strength", 3]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 148| 148| 			"interests": [["strength", 2]] };
| 149| 149| 		this.unitStat.MeleeMounted      = { "priority": 0.7, "minSize": 4, "targetSize": 20, "batchSize": 4, "classes": ["Mounted", "Melee", "CitizenSoldier"],
| 150| 150| 			"interests": [["strength", 2]] };
| 151|    |-		this.unitStat.ChampRangedMounted  = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Ranged", "Champion"],
|    | 151|+		this.unitStat.ChampRangedMounted = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Ranged", "Champion"],
| 152| 152| 			"interests": [["strength", 3]] };
| 153| 153| 		this.unitStat.ChampMeleeMounted   = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Melee", "Champion"],
| 154| 154| 			"interests": [["strength", 2]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 150| 150| 			"interests": [["strength", 2]] };
| 151| 151| 		this.unitStat.ChampRangedMounted  = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Ranged", "Champion"],
| 152| 152| 			"interests": [["strength", 3]] };
| 153|    |-		this.unitStat.ChampMeleeMounted   = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Melee", "Champion"],
|    | 153|+		this.unitStat.ChampMeleeMounted = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Melee", "Champion"],
| 154| 154| 			"interests": [["strength", 2]] };
| 155| 155| 		this.unitStat.Hero                = { "priority": 1, "minSize": 0, "targetSize":  1, "batchSize": 1, "classes": ["Hero"],
| 156| 156| 			"interests": [["strength", 2]] };
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 152| 152| 			"interests": [["strength", 3]] };
| 153| 153| 		this.unitStat.ChampMeleeMounted   = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Melee", "Champion"],
| 154| 154| 			"interests": [["strength", 2]] };
| 155|    |-		this.unitStat.Hero                = { "priority": 1, "minSize": 0, "targetSize":  1, "batchSize": 1, "classes": ["Hero"],
|    | 155|+		this.unitStat.Hero = { "priority": 1, "minSize": 0, "targetSize":  1, "batchSize": 1, "classes": ["Hero"],
| 156| 156| 			"interests": [["strength", 2]] };
| 157| 157| 		this.neededShips = 5;
| 158| 158| 	}
|    | [NORMAL] ESLintBear (key-spacing):
|    | Extra space before value for key 'targetSize'.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 152| 152| 			"interests": [["strength", 3]] };
| 153| 153| 		this.unitStat.ChampMeleeMounted   = { "priority": 1, "minSize": 3, "targetSize": 15, "batchSize": 3, "classes": ["Mounted", "Melee", "Champion"],
| 154| 154| 			"interests": [["strength", 2]] };
| 155|    |-		this.unitStat.Hero                = { "priority": 1, "minSize": 0, "targetSize":  1, "batchSize": 1, "classes": ["Hero"],
|    | 155|+		this.unitStat.Hero                = { "priority": 1, "minSize": 0, "targetSize": 1, "batchSize": 1, "classes": ["Hero"],
| 156| 156| 			"interests": [["strength", 2]] };
| 157| 157| 		this.neededShips = 5;
| 158| 158| 	}
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before '='.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
| 161| 161| 		priority = 70;
| 162| 162| 		this.unitStat.RangedInfantry = { "priority": 1, "minSize": 6, "targetSize": 16, "batchSize": 3, "classes": ["Infantry", "Ranged"],
| 163| 163| 			"interests": [["canGather", 1], ["strength", 1.6], ["costsResource", 0.3, "stone"], ["costsResource", 0.3, "metal"]] };
| 164|    |-		this.unitStat.MeleeInfantry  = { "priority": 1, "minSize": 6, "targetSize": 16, "batchSize": 3, "classes": ["Infantry", "Melee"],
|    | 164|+		this.unitStat.MeleeInfantry = { "priority": 1, "minSize": 6, "targetSize": 16, "batchSize": 3, "classes": ["Infantry", "Melee"],
| 165| 165| 			"interests": [["canGather", 1], ["strength", 1.6], ["costsResource", 0.3, "stone"], ["costsResource", 0.3, "metal"]] };
| 166| 166| 		this.unitStat.Mounted = { "priority": 1, "minSize": 2, "targetSize": 6, "batchSize": 2, "classes": ["Mounted", "CitizenSoldier"],
| 167| 167| 			"interests": [["strength", 1]] };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 0.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|1140|1140| 
|1141|1141| 	if (blocker && blocker.hasClass("StoneWall"))
|1142|1142| 	{
|1143|    |-/*		if (this.hasSiegeUnits())
|    |1143|+		/*		if (this.hasSiegeUnits())
|1144|1144| 		{ */
|1145|1145| 			this.isBlocked = true;
|1146|1146| 			return blocker;
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|1142|1142| 	{
|1143|1143| /*		if (this.hasSiegeUnits())
|1144|1144| 		{ */
|1145|    |-			this.isBlocked = true;
|    |1145|+		this.isBlocked = true;
|1146|1146| 			return blocker;
|1147|1147| /*		}
|1148|1148| 		return undefined; */
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|1143|1143| /*		if (this.hasSiegeUnits())
|1144|1144| 		{ */
|1145|1145| 			this.isBlocked = true;
|1146|    |-			return blocker;
|    |1146|+		return blocker;
|1147|1147| /*		}
|1148|1148| 		return undefined; */
|1149|1149| 	}
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 0.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|1144|1144| 		{ */
|1145|1145| 			this.isBlocked = true;
|1146|1146| 			return blocker;
|1147|    |-/*		}
|    |1147|+		/*		}
|1148|1148| 		return undefined; */
|1149|1149| 	}
|1150|1150| 	else if (blocker)
|    | [NORMAL] ESLintBear (operator-assignment):
|    | Assignment can be replaced with operator assignment.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/ai/petra/attackPlan.js
|1538|1538| 				range = 30 + ent.attackRange("Ranged").max;
|1539|1539| 			else if (ent.hasClass("Mounted"))
|1540|1540| 				range += 30;
|1541|    |-			range = range * range;
|    |1541|+			range *= range;
|1542|1542| 			let entAccess = PETRA.getLandAccess(gameState, ent);
|1543|1543| 			// Checking for gates if we're a siege unit.
|1544|1544| 			if (siegeUnit)
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/562/display/redirect

Stan added inline comments.Sep 2 2019, 10:45 PM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

How is a dog a 'Mounted' unit ?

Nescio added inline comments.Sep 3 2019, 9:06 AM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

Feel free to suggest a more appropiate term!
Anyway, the class is non-visible, and without it, the AI won't train war dogs.

Angen added inline comments.Sep 3 2019, 9:46 AM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

The real question is. Will petra spam dogs instead cavalry where cavalry would be better choice ?

Nescio added inline comments.Sep 3 2019, 9:59 AM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

Cavalry etc gets the Mounted class as well, so I suppose what Petra trains depends on the stats of the units in question (hence outside the scope of this patch).

Nescio added inline comments.Sep 3 2019, 10:07 AM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

Looking at the attackPlan.js code again (above), the AI won't actually be able to train war dogs; to do so they would need not only the Mounted class, but also either the CitizenSoldier or the Champion class.

Nescio updated this revision to Diff 9611.Sep 3 2019, 10:41 AM
Nescio edited the summary of this revision. (Show Details)

!Champion vs Champion

binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9604)

And now they can.

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/59/display/redirect

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/568/display/redirect

the AI can (and will) now train (Briton) war dogs and (Mauryan) elephant archers

Did you try this? Because I just observed two (not significant, I know) PetraAI vs PetraAI-matches and they did not produce a war dog (nor a Kennel, but even when I intervened and built a Kennel no wardogs were trained).

Nescio added a comment.EditedSep 6 2019, 10:49 PM

Did you try this? Because I just observed two (not significant, I know) PetraAI vs PetraAI-matches and they did not produce a war dog (nor a Kennel, but even when I intervened and built a Kennel no wardogs were trained).

Good question! I just ran three Britons vs Britons vs Britons games, but in none of those they built kennels, hence no war dogs.
I do recall Britons training war dogs when I tried this out some weeks ago, but that was in a A23 mod, admittedly, and I may have changed other things then as well; I can't reproduce it with this patch.

Nescio edited the summary of this revision. (Show Details)Sep 6 2019, 10:50 PM
Nescio edited the summary of this revision. (Show Details)Jan 3 2020, 10:04 PM
Nescio added a reviewer: Angen.
Nescio removed subscribers: bb, Angen.
Angen added a comment.Jan 5 2020, 2:14 PM

[EDIT]: CitizenSoldier vs Champion is a false dichotomy; this patch replaces it with !Champion vs Champion.

This should go elsewhere not in this patch as it is ouf of scope.

The main reason why Petra cares about Cavalry class in combat is because they ought to be faster than infantry.
The same for hunting. It is because Cavalry is fast moving and has better meat gathering rate, what looks maybe more like coincidence.

So maybe FastMoving or something like that ?
Then petra could be more flexible with using faster units based on mod for hunt or to rush towards opponent.

binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9611)

how is dog human

Nescio added a comment.EditedJan 5 2020, 8:59 PM

This should go elsewhere not in this patch as it is ouf of scope.

I guess I included it here because it is the same lines that are changed here, but yes, including it here is perhaps inappropiate; I'll revert that.
[EDIT]: Actually I did it because the AI considers only CitizenSoldiers or Champions; dogs are neither and will thus never be trained; by replacing CitizenSoldier with !Champion (or Champion with !CitizenSoldier), the AI might actually consider dogs, hence why including that change in this patch could make sense.

The main reason why Petra cares about Cavalry class in combat is because they ought to be faster than infantry.
The same for hunting. It is because Cavalry is fast moving and has better meat gathering rate, what looks maybe more like coincidence.

So maybe FastMoving or something like that ?
Then petra could be more flexible with using faster units based on mod for hunt or to rush towards opponent.

Good point! I suggested Mounted because I was looking for a common denominator for the camels, cavalry, chariots, dogs, and elephants and couldn't think of anything better; they're all typically faster than infantry, so FastMoving would work too (should it then also apply to the plane cheat unit?). Other suggestions for the class name are welcome (the important thing is that it's not Cavalry).

binaries/data/mods/public/simulation/templates/template_unit_dog.xml
39 ↗(On Diff #9611)

I suppose because Healers can only heal units of the Human class. Could be changed.

Nescio updated this revision to Diff 10891.Jan 5 2020, 9:17 PM
Nescio retitled this revision from AI: replace Cavalry with new Mounted class to AI: replace Cavalry with new FastMoving class.
Nescio edited the summary of this revision. (Show Details)

Redone per @Angen.

Vulcan added a comment.Jan 5 2020, 9:18 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/958/display/redirect

Vulcan added a comment.Jan 5 2020, 9:20 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/54/display/redirect

Vulcan added a comment.Jan 5 2020, 9:21 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1476/display/redirect

Angen added inline comments.Jan 6 2020, 10:45 AM
binaries/data/mods/public/simulation/templates/template_unit_champion_cavalry.xml
26 ↗(On Diff #10891)

should not be visible

Nescio added inline comments.Jan 6 2020, 1:13 PM
binaries/data/mods/public/simulation/templates/template_unit_champion_cavalry.xml
26 ↗(On Diff #10891)

Oops, my mistake.

Nescio updated this revision to Diff 10897.Jan 6 2020, 1:14 PM

correct Class mistake

Vulcan added a comment.Jan 6 2020, 1:17 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/962/display/redirect

Vulcan added a comment.Jan 6 2020, 1:18 PM

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

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/58/display/redirect

Vulcan added a comment.Jan 6 2020, 1:22 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1480/display/redirect

Angen accepted this revision.EditedJan 8 2020, 7:53 AM

This is allowing ai to be more flexible with army composition based on not visible class when it needs/wants fast moving units to hunt or raid enemy. Every reason why was cavalry used was speed advantage. If some mod would like to introduce some other unit which would be faster with ability to hunt or attack, it would be better choice for ai to use instead. That would mean one need to or edit petra or label unit with cavalry class when he/she clearly does not want to because there exist auras and bonuses unit should not get and in that case condition for classes in auras and technologies could get complicated pretty fast.

This revision is now accepted and ready to land.Jan 8 2020, 7:53 AM
Stan added a subscriber: bb.Jan 8 2020, 9:01 AM

As @bb stated on IRC wouldn't it be better for Petra to have a g_SlowUnitTreshold = x; And use the unitmotion instead of classes?

Angen added a comment.Jan 8 2020, 9:30 AM

That would need to change based on civilisation petra is playing because that value may in theory differ per civilization. And if so, what would be the correct number? If that would not be calculated by petra, that would mean another value to keep in sync with templates. I am not against to try that, I just think this way is easier to maintain. Other than that, it is removing binding to certain type of unit and using rather description class like "Worker".
(I would be in favor to remove bindings to citizensoldier and femalecitizen too and use some general classes for them in petra)

Stan added a comment.Jan 8 2020, 10:13 AM
In D2251#106407, @Angen wrote:

That would need to change based on civilisation petra is playing because that value may in theory differ per civilization. And if so, what would be the correct number? If that would not be calculated by petra, that would mean another value to keep in sync with templates. I am not against to try that, I just think this way is easier to maintain. Other than that, it is removing binding to certain type of unit and using rather description class like "Worker".
(I would be in favor to remove bindings to citizensoldier and femalecitizen too and use some general classes for them in petra)

Well not really? Since cavalry or "FastMoving" units are an order of magnitude faster than their counterparts, only one value could suffice. It would indeed remove the binding to some classes :)

(I would be in favor to remove bindings to citizensoldier and femalecitizen too and use some general classes for them in petra)

As would I. Ideally auras and technologies would only use visible classes and the AI use internal classes with functional and unambiguous names (Citizen isn't).

Other than that, it is removing binding to certain type of unit and using rather description class like "Worker".

Wouldn't it be better to use Builder and Gatherer instead of Worker?

Since cavalry or "FastMoving" units are an order of magnitude faster than their counterparts

Not exactly true. Let's have a look at e.g. Macedonian village phase units:
infantry pikeman: 7.2 walk, 12.0 run
infantry javelinist: 12.6 walk, 21.0 run
cavalry spearman: 19.3 walk, 32.2 run
So the javelinist is 75% faster than the pikeman, but the cavalry only 53% faster than the javelinist, still clearly faster, yes, but not an order of magnitude. A threshold of e.g. 15.0 would work, but setting any value is rather arbitrary, so using a FastMoving class seems more consistent.

Angen added a comment.Jan 8 2020, 11:56 AM

Wouldn't it be better to use Builder and Gatherer instead of Worker?

To make petra more error prone, it would need to indeed split worker to builder and gatherer as these two roles do not have to be together on one unit. But, doing that would require to ensure petra will have enough builders but on the other side not too many,
As in general these two roles are shared among workers, there is no urgent need to do so and to complicate the code.

It would make Petra more flexible at handling mods, though.

Angen added a comment.Jan 8 2020, 12:08 PM

Sure, I just see there possible hard times trying to achieve that while making it not broken.

elexis added a subscriber: elexis.Jan 8 2020, 1:00 PM

Removing the Cavalry hardcoding of the AI is good, adding a new Identity class adds problems that could be avoided.

The AI code would work more correctly if it was not a binary distinction fast vs. not fast, but comparisons of WalkSpeed.
For instance:

  • The AI could pick the unit with the highest walk speed and armor for raiding.
  • The AI could only hunt deer with units that have a RunSpeed similar to the RunSpeed of the attacked animal etc.

Since that will not be implemented here, looking for units with a high WalkSpeed is more maintainable and less error prone than looking for units that have an identity class.

Problems:

  • https://en.wikipedia.org/wiki/Single_source_of_truth pattern. With this approach you store which units can move fast in two places. This opens up for the following problems:
  • add the FastMoving tag for units with small WalkSpeed
  • miss / forget / never consider adding the FastMoving tag to units that have high WalkSpeed
  • It costs attention and time to maintain the tags that could be spend on other things if this code would just do a WalkSpeed > hardcodedNumber test.
  • It is not explained anywhere when a unit should be FastMoving. Is 10 WalkSpeed good enough? 8? 6? 6.5? Whereas a WalkSpeed > petraAI.hardcodedNumber would be clear.
  • When someone changes the WalkSpeed, he may not consider/forget/miss keeping the tag in sync (i.e. removing it when lowering walk speed or adding it when increasing walk speed).
  • Someone actually wanting to fix this code would have to remove now even more code than before.

So if you want to remove the Cavalry hardcoding and the WalkSpeed(unit1) > WalkSpeed(unit2) comparison is out of reach, replacing the tag check with WalkSpeed > n would not introduce an array of new problems to remove one problem.

(There is also the problem that a unit may have little WalkSpeed by defaut but gain a WalkSpeed tech/aura bonus that makes it much faster. This has been neglected and may be neglected in the future too for performance, as subscribing to ValueModification sounds expensive.)

elexis added a comment.Jan 8 2020, 2:01 PM

Consider how the templates would look like if this approach was used on a large scale.
For example if the AI was to consider which units to attack buildings with by using a Siege/Elephant class and that class was to be superseded by a GoodAgainstBuildings class, a GoodGatheringOnFields/Wood/Metal/... class, a FastBuilder class, etc.
In the worst case it would be one class per template property, doubling the size of logical templates properties, while making it more than double the effort to keep everything in sync, since one has to compare every template value against the identity class and then for the mismatches see where they arose from, whether the identity class is wrong or whether the template value is wrong, cross comparing files, traversing inheritance etc.

(Theres also the hardcoding problem, which is more obvious with FastGatheringOnFields and GoodAgainstBuildings since mods may not have fields and since some buildings may not have high crush armor and would be better attacked with units dealing another damage type. In the case for this revision proposal, it's that some units may be very well fast enough to hunt deer but too slow to raid. I put it into parenthesis since removing the hardcodings was not the intent of the patch, it would make the hardcoding not much qualitatively worse but quantitatively.)

@elexis, you make valid points. Nonetheless, I still believe this patch in its current form is an improvement over the status quo.

Nescio added a comment.Jun 1 2020, 6:18 PM

If people want to make the AI use a certain WalkSpeed value threshold (e.g. 15.0) instead of a class, feel free to commandeer. I'm not a programmer, my understanding of the AI and JavaScript is too limited to have even a vague idea how to implement such a thing.

Nescio retitled this revision from AI: replace Cavalry with new FastMoving class to [ai/petra] replace Cavalry with new FastMoving class.Wed, Jul 15, 3:08 PM
borg- added a subscriber: borg-.Tue, Jul 21, 3:32 PM

Some new?

No. This patch works, or at least it did last time I checked.
I believe the hold up is that rather than introducing a new class, a more elegant solution would be to have a walk speed cut-off, as rightly pointed out by @elexis. While I agree in principle, in practice my understanding is too limited, so implementing that is beyond my abilities. If someone else knows how to, feel free to commandeer this patch.
However, the current patch is available and solves the issue from a template (and gameplay) perspective, so it's an improvement over the status quo, or at least a short term solution.

wraitii updated this revision to Diff 12914.Sat, Jul 25, 11:11 AM
wraitii added a subscriber: wraitii.

Slight rebase & slight update to the AI changes.

Nescio added inline comments.Sat, Jul 25, 11:13 AM
binaries/data/mods/public/simulation/templates/template_unit_dog.xml
38 ↗(On Diff #12914)

Given that war dog speed has been reduced today (D2879/rP23893), this line is probably no longer a good idea.

wraitii accepted this revision.Sat, Jul 25, 11:13 AM

I like my hardcoding to be explicit. This is then indeed an improvement on the status quo.

The AI is already using classes in a very hardcoded, "semantic" manner in a number of places, least of all the distinction between Ranged and melee units. Some of those classes have additional uses beyond their "AI-hint-edness", but that does not seem a valid reason to discard AI-hints-as-classes.

While I agree that the alternative is to have a "treshhold" for fast moving units, this isn't trivial to compute, since it depends on what "slow" means. Further, attack plans are extremely hardcoded at the moment and very much not trivial to change.

As such, I think this is a straight improvement, and the "de-hard-coding" of the AI can come later.


I will commit this in a few days.

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2778/display/redirect

wraitii updated this revision to Diff 12984.Sat, Aug 1, 10:45 AM

Rebased, will commit once lights are green

Owners added a subscriber: Restricted Owners Package.Sat, Aug 1, 10:45 AM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2823/display/redirect

This revision was automatically updated to reflect the committed changes.