Page MenuHomeWildfire Games

Separate stable from barracks
ClosedPublic

Authored by Nescio on Mar 17 2019, 3:27 PM.

Details

Summary

Currently the (cavalry) stable template is a child of the barracks template, which is odd; the elephant stable, dog kennel, and workshop all inherit from the military template, not from the barracks template.
This patch:

  • moves template_structure_military_barracks_stables.xml to template_structure_military_stable.xml
  • updates all its children accordingly
  • merges values from pers_stable.xml into the new generic template
  • changes generic name Stable (singular, without s)
  • standardizes rubble sizes to 5x5 (pers had 4x4)
  • corrects specific name for Greek factions
  • adds foundation actor to kush_stable.xml
Test Plan

Check if nothing is overlooked and everything still works.

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.Mar 17 2019, 3:27 PM
Vulcan added a subscriber: Vulcan.Mar 17 2019, 3:58 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Identity.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Identity.js
|  97|  97| 	this.visibleClassesList = GetVisibleIdentityClasses(this.template);
|  98|  98| };
|  99|  99| 
| 100|    |-Identity.prototype.Deserialize = function ()
|    | 100|+Identity.prototype.Deserialize = function()
| 101| 101| {
| 102| 102| 	this.Init();
| 103| 103| };
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1102/display/redirect

Nescio edited the summary of this revision. (Show Details)Apr 7 2019, 11:04 AM
Nescio updated this revision to Diff 7732.Apr 12 2019, 8:26 PM
Nescio edited the summary of this revision. (Show Details)

Updated because of D1793.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1203/display/redirect

Nescio updated this revision to Diff 7761.Apr 17 2019, 10:53 AM
Nescio edited the summary of this revision. (Show Details)

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1221/display/redirect

Nescio updated this revision to Diff 7897.May 2 2019, 11:48 AM
Nescio edited the summary of this revision. (Show Details)

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1318/display/redirect

Angen added a subscriber: Angen.May 2 2019, 12:01 PM

you removed class Barracks, Petra AttackManager needs update, else might not attack if no building with class Barracks.
source of problem line 320

Thanks for pointing that out. Is it necessary, though?
Kennel, Range, Workshop, etc. don't have a Barracks class either. And simulation/ai/petra/headquarters.js seems to distinguish between Barracks, Range, and Stable.

Angen added a comment.May 2 2019, 1:39 PM

building stables by ai is not a problem, problem is check in attack manager, no Barracks means no Attack, HugeAttack and Rush, only Raids from defence manager.
Really needed, hmm.
Kennel, Workshop - no problem, they are not vital for attack plans
If we can guarantee Petra will still have at least one Barracks, then ok, but we should not rely on it.

Nescio added a comment.May 2 2019, 1:57 PM

The Persian stable – the only stable currently buildable in game – explicitly removed the Barracks class, therefore I assumed it wouldn't cause problems. At least I haven't noticed the Persians have trouble attacking. If it is an issue, then it's already broken without this patch.

Angen added a comment.May 2 2019, 2:10 PM

I see.
As this is problem with Ranged as well, and works for Persians as pointed out, I do not have objections to this diff.

So here is something I was thinking about and I would like to know what others think.
We are making separate training buildings for ranged, melee and cavalry units.
I think we should ensure that Petra will attack even when it will not have build Barracks (melee) and only Ranged or Stables.
Why should be Barracks the building which will make difference between attacking and not?

Nescio added a comment.May 2 2019, 2:28 PM

Although I don't really understand how the AI works, it seems it can handle the new structures (range, stable, workshop, elephant stable). No range is currently in use, though.
As for why the Barracks makes a difference, I suppose it's because citizen-soldier infantry can gather all resources and build, cavalry can't. Besides, the barracks still trains not only melee infantry, but also ranged infantry and cavalry.

Angen added a comment.EditedMay 6 2019, 5:02 PM

Well, as I see reason why attack manager is blocked by barracks is that if there would be none, attack plans would keep failing because they in most cases wait until requested number of units is available, there are combinations of ranged, melee, cav and champions. If Ai cannot train units, plan fails but it creates it again and again fails. Thats why barracks are needed as requirments to ensure that plans will not fail because Ai has no facility to train units. Now imagine mod enabling stables and ranged and leaving barracks only for melee. I think this is why these buildings have been created at first place right? So my concerne is. Should game handle splitted training buildings with respect to needs of attack plans or should mods enabling them be aware that they have to provide Barracks else Ai will not attack.

This is fixing stable problem introduced in rP21751

Angen added a comment.May 30 2019, 8:11 PM

needs rebase

Nescio updated this revision to Diff 8223.May 30 2019, 9:23 PM
Nescio edited the summary of this revision. (Show Details)

Here you go

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1553/display/redirect

bb added inline comments.May 30 2019, 10:01 PM
binaries/data/mods/public/simulation/templates/structures/athen_stable.xml
9 ↗(On Diff #8223)

Would indeed be good to have some translation here, but I can't find a reference that Hippṓn is the correct term, wouldn't Hippóstasis be better?

binaries/data/mods/public/simulation/templates/template_structure_military_stable.xml
30 ↗(On Diff #8223)

k

45–47 ↗(On Diff #8223)

as the foldering suggests these techs are meant to be for the pers/successors only, thus shouldn't be in the parent

Nescio added inline comments.May 30 2019, 10:18 PM
binaries/data/mods/public/simulation/templates/structures/athen_stable.xml
9 ↗(On Diff #8223)

Both are correct; Xenophon uses ἱππών, Polybius ἱππόστασις. I picked the former mainly because that's the one @Anaxandridas ho Skandiates preferred: https://wildfiregames.com/forum/index.php?/topic/25268-specific-name-review-structures/&tab=comments#comment-367672

binaries/data/mods/public/simulation/templates/template_structure_military_stable.xml
45–47 ↗(On Diff #8223)

Those technologies have a required civ, so they'll only show up to Persians. Defining them here in the parent template has the advantage that it's less likely to forget adding them in the child templates; moreover, it means Persians can research these technologies at any (captured) stable, not only those built by pers. (However, only Persian stables are currently used, so effectively this changes nothing in the current situation.)

bb accepted this revision.May 30 2019, 10:55 PM
bb added inline comments.
binaries/data/mods/public/simulation/templates/template_structure_military_stable.xml
45–47 ↗(On Diff #8223)

right

This revision is now accepted and ready to land.May 30 2019, 10:55 PM
Nescio added inline comments.May 30 2019, 10:55 PM
binaries/data/mods/public/simulation/templates/template_structure_military_stable.xml
45–47 ↗(On Diff #8223)

For comparison, template_structure_military_workship.xml includes “siege_bolt_accuracy” in its technology list, even though that's available to only a minority of civs.

This revision was automatically updated to reflect the committed changes.
bb added inline comments.May 30 2019, 11:01 PM
binaries/data/mods/public/simulation/templates/template_structure_military_stable.xml
45–47 ↗(On Diff #8223)

Thanks for being so persistent, saved a nice inconsistency

Thank you for reviewing and committing this, I appreciate it! The fewer inconsistencies in the templates, the better. Are you also interested in doing D1794?