Page MenuHomeWildfire Games

Separate range from barracks
ClosedPublic

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

Details

Summary

Currently the range 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_range.xml to template_structure_military_range.xml
  • updates all its children accordingly
  • changes generic name to Practice Range (because it not only trains archers, but also slingers and javelinists)
  • footprint, rubble, obstruction, and visual actors are removed from civ specific templates, unless they differ from the generic template
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:41 PM
bb added a comment.Mar 17 2019, 3:49 PM

Can I propose to make a template_structure_military_training (or find better name) template as a parent of barracks, stable and range? That would solve the duplication of this patch and D1790

But then again, why would barracks, range, and stable inherit from a different template than the elephant stable, kennel, and workshop?
What could be done is move some of the duplication into the shared parent template_structure_military.xml.

Vulcan added a subscriber: Vulcan.Mar 17 2019, 4:00 PM

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

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

bb added a comment.Mar 17 2019, 4:07 PM

That is a good point indeed, so we should be looking at which properties we can move to the military parent. Territoy, Vision, GarrsionHolder and Sound seem to be the biggest candidates

Sound, TerritoryInfluence, and Vision, yes, those make sense, maybe BuildRestrictions too (Category=Military).
As for GarrisonHolder, different structures can shelter different classes and numbers (e.g. barracks 10 infantry, kennel 5 dogs, workshop 5 siege, etc.); the shared attributes could be moved into the Military template, but perhaps it's better to have a bit more duplication and keep all of GarrisonHolder in the separate templates.

Also, which route do you prefer?

  • First separate range and stable from barracks, then remove duplication in a separate patch
  • First remove duplication in a new patch, then update these two to separate range and stable afterwards
bb added a comment.Mar 17 2019, 4:25 PM

GarrisonHolder: change the Max attribute in the childs, put the rest in the parent

IMO first remove the duplication then update

As for vision, barracks and blacksmith currently have 32, embassy has 24, dock, elephant stable, kennel, workshop have 40, fortress 80. Which value should the military template have?

bb added a comment.Mar 17 2019, 4:40 PM

Candidate != must be moved, I guess those should be in the children then...

Nescio edited the summary of this revision. (Show Details)Apr 7 2019, 11:03 AM
Nescio updated this revision to Diff 7735.Apr 12 2019, 8:49 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/1204/display/redirect

bb accepted this revision.Apr 15 2019, 11:35 PM

Cav got removed from ranges (which makes sense)

binaries/data/mods/public/simulation/templates/structures/spart_range.xml
12 ↗(On Diff #7735)

why that?

binaries/data/mods/public/simulation/templates/template_structure_military_range.xml
22 ↗(On Diff #7735)

Village should be a visible class
and as we train all ranged infantry shouldn't Archery => Range?

This revision is now accepted and ready to land.Apr 15 2019, 11:35 PM
This revision was automatically updated to reflect the committed changes.
Nescio added inline comments.Apr 16 2019, 9:07 AM
binaries/data/mods/public/simulation/templates/structures/spart_range.xml
12 ↗(On Diff #7735)

This is the specific name of the Spartan barracks, other Greek ranges (athen, mace, ptol, sele) don't have a specific name yet either, and it's better to have none than a clearly incorrect one, therefore I had removed it.

binaries/data/mods/public/simulation/templates/template_structure_military_range.xml
22 ↗(On Diff #7735)

It still is a bit confusing to me which classes should be visible and which not.
Anyway, Range might be confused with the Ranged class, so perhaps it should be PracticeRange instead (cf. ElephantStable)? And shouldn't it be visible too? (Barracks and Stable (D1790) are.)