Page MenuHomeWildfire Games

templates/units/ champion naming consistency
AbandonedPublic

Authored by Nescio on Sep 13 2017, 1:27 PM.

Details

Reviewers
bb
s0600204
Summary

Currently the naming of civ specific champion and mercenary units in the templates/units/ folder is rather inconsistent; it appears to be designed at a time when all factions were supposed to have only one infantry champion and one cavalry champion. The current naming scheme makes it hard to add new champion units; e.g. Athens has the following champion units:

athen_champion_infantry.xml
athen_champion_marine.xml
athen_champion_ranged.xml
athen_thureophoros.xml

This patch applies a consistent naming scheme, similar to how citizen soldier templates and champion actor art files are named, which makes it more suitable for future additions and mods; e.g. Athens' champions are now named:

athen_infantry_spearman_champion.xml
athen_infantry_marine_champion.xml
athen_infantry_archer_champion.xml
athen_infantry_javelinist_champion.xml
Test Plan

Large patch, test if everything works, and nothing is overlooked (including structure production queues)

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5143
Build 8804: Vulcan BuildJenkins
Build 8803: arc lint + arc unit

Event Timeline

Nescio created this revision.Sep 13 2017, 1:27 PM
Owners added a subscriber: Restricted Owners Package.Sep 13 2017, 1:27 PM
fatherbushido added a subscriber: fatherbushido.EditedSep 13 2017, 2:14 PM

That's a move from something dirty to something dirty (with possible side effects).

Vulcan added a subscriber: Vulcan.Sep 13 2017, 2:14 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2020/ for more details.

Nescio added a comment.EditedSep 13 2017, 2:44 PM

That's a move from something dirty to something dirty (with possible side effects).

True, it's dirty, but at least a bit more consistent; any suggestions to make it less dirty?

The only thing I can think of is to remove distinct marine unit templates alltogether, which would result into leaving us with:
athen_champion_swordsman.xml (with Iphicratean reforms requirement)
athen_infantry_archer.xml (with Iphicratean reforms requirement)
pers_ship_trireme (with equine transports requirement) has:

<Entities datatype="tokens">
  units/pers_cavalry_swordsman_b
  units/pers_cavalry_javelinist_b
</Entities>

The disadvantage of this is that the Persians no longer can train triremes before researching equine transports

True, it's dirty, but at least a bit more consistent; any suggestions to make it less dirty?

I sadly see more what can't work than what can work :/
It's also intertwined with the units/*champion* template namming and the merc namming (and so on how merc are designed). And...
Well it's easier to discuss of that around a coffee/tea/beer than to write all in that comment window.

The main thing to have in mind is how we handle the production queue list (those one should be cleaned btw) and how it will behave with capture.

The only thing I can think of is to remove distinct marine unit templates alltogether, which would result into leaving us with:
athen_champion_swordsman.xml (with Iphicratean reforms requirement)
athen_infantry_archer.xml (with Iphicratean reforms requirement)
pers_ship_trireme (with equine transports requirement) has:

<Entities datatype="tokens">
  units/pers_cavalry_swordsman_b
  units/pers_cavalry_javelinist_b
</Entities>

The disadvantage of this is that the Persians no longer can train triremes before researching equine transports

Something like that looks perhaps a better way to do.

Nescio added a comment.EditedSep 13 2017, 3:45 PM

I sadly see more what can't work than what can work :/

That can often be very useful :)

It's also intertwined with the units/*champion* template namming and the merc namming (and so on how merc are designed). And...

Yeah, that's also a bit dirty. I have some ideas how to make it more consistent, but that would be quite an overhaul, and I'm not sure it would be a good idea (mods will have to be updated, etc.). E.g. for different civ/factions:

units/*_citizen_archer_infantry_b.xml
units/*_mercenary_archer_infantry_b.xml
units/*_champion_archer_infantry.xml

Well it's easier to discuss of that around a coffee/tea/beer than to write all in that comment window.

Yes, I fully agree with this. Unfortunately we're limited by this comment format.

The main thing to have in mind is how we handle the production queue list (those one should be cleaned btw) and how it will behave with capture.

Yeah, however, I don't intend to try to change how it currently works.

Something like that looks perhaps a better way to do.

You think so? Then I'll update this patch proposal accordingly and see if anyone has a better suggestion.

By the way, do you think equine transports ought to be changed from city phase to town phase?

Nescio updated this revision to Diff 3655.Sep 13 2017, 4:05 PM
Nescio edited the summary of this revision. (Show Details)

Changed per discussion with fatherbushido

About the champion namming I would really like if it was done :)
There were some issues iirc as for example with the two mauryan infantry champions.
For the mercenary I have no idea. There are at least two annoying things:

  • there are champ and cs mercenaries
  • there are redundant type of mercenaries (for carth)
Nescio added a comment.EditedSep 13 2017, 4:48 PM

About the champion namming I would really like if it was done :)

This was actually one of the first things I changed in my mod (0abc), but I had to rename about all cavalry and infantry templates in existence. If you're not the only one who would like a consistent template naming scheme, and if someone is willing to review it, I believe I could create a new patch proposal for it. However, I'm not sure it's a good idea, as it would imply more work for other mods to get them updated, etc.

There were some issues iirc as for example with the two mauryan infantry champions.

Not really a problem, it's very easy to solve:
maur_champion_archer_infantry
maur_champion_mace_infantry
maur_champion_sword_infantry

For the mercenary I have no idea. There are at least two annoying things:

  • there are champ and cs mercenaries

Champion mercenaries are typically statistically identical to other champions; the only difference is the “mercenary” tag; so a mercenary champion could have the same name as a champion. Citizen mercenaries are different (cost etc), so in my mod I named them “mercenary”; however, in the vanilla 0ad no faction can train a citizen and mercenary version of the same unit, so just giving them the same name as citizens could work here.

I also created a “hoplite” template to allow distinguishing between different spearmen (e.g. Persian Immortal a champion spearman and Persian Kardakes a champion hoplite); however, I believe this change is beyond the scope of what's discussed above.

  • there are redundant type of mercenaries (for carth)

Yeah, this was a bit annoying; in my mod I solved it by redefining some sword units to axe:
cart_mercenary_axe_cavalry (iber)
cart_mercenary_spear_cavalry (ital)
cart_mercenary_sword_cavalry (gaul)
cart_mercenary_axe_infantry (gaul)
cart_mercenary_javelin_infantry (iber)
cart_mercenary_sling_infantry (iber)
cart_mercenary_sword_infantry (ital)
Again, this works for my mod, but might not be a good idea to implement in the default 0ad distribution.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2021/ for more details.

Nescio updated this revision to Diff 3656.Sep 13 2017, 7:46 PM
Nescio retitled this revision from naming consistency for units trained at ships to templates/units naming consistency: champion, marine, trireme, and merc units.
Nescio edited the summary of this revision. (Show Details)
Nescio edited the test plan for this revision. (Show Details)

Applied a consistent template naming scheme to units/{civ}_champion_*, per discussion with fatherbushido

Nescio updated this revision to Diff 3657.Sep 13 2017, 7:50 PM
Nescio edited the summary of this revision. (Show Details)
Nescio updated this revision to Diff 3658.Sep 13 2017, 7:54 PM
Nescio updated this revision to Diff 3659.Sep 13 2017, 7:58 PM
Nescio retitled this revision from templates/units naming consistency: champion, marine, trireme, and merc units to templates/units/ naming consistency: champion, marine, trireme, and merc units.
Nescio edited the summary of this revision. (Show Details)Sep 13 2017, 8:22 PM
Nescio updated this revision to Diff 3661.Sep 13 2017, 8:33 PM
Nescio edited the summary of this revision. (Show Details)

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2022/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2023/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2024/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2025/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2027/ for more details.

“This revision needs review, but there are no reviewers specified.” Any suggestions where I can find a reviewer? (I suppose I just have to be patient.)

Don't forget to check the map folder.

I also created a “hoplite” template to allow distinguishing between different spearmen (e.g. Persian Immortal a champion spearman and Persian Kardakes a champion hoplite); however, I believe this change is beyond the scope of what's discussed above.

Also that axe things seems interesting (I find it ugly to have redundant units for a said civ). But we would move to a design thing, I guess that you have some historical input about that? (I need to read again your mod description)

Don't forget to check the map folder.

I also created a “hoplite” template to allow distinguishing between different spearmen (e.g. Persian Immortal a champion spearman and Persian Kardakes a champion hoplite); however, I believe this change is beyond the scope of what's discussed above.

Also that axe things seems interesting (I find it ugly to have redundant units for a said civ). But we would move to a design thing, I guess that you have some historical input about that? (I need to read again your mod description)

Maps? Forgive me my ignorance, but why and what do I have to look for in there?

As for hoplites etc, the main difference is the type of shield used:

  • pikemen: a small, crescent-shaped shield (peltê)
  • hoplites: a heavy, circular shield (aspis)
  • spearmen: an elongated shield (thureos) (which could be curved or flat; made from wicker, wood, leather, or a combination of materials; figure-eight, elliptical, oval, rectangular, tower, etc)

I'm quite happy with implementing the hoplite template (which is different from the spearman in my mod, but remains statistically similar in this diff proposal).

The usage of axes as battle weapons (as opposed to tools; adzes were unuseful as weapons) is well attested in the Near East (Assyria, Egypt, Kush, etc: epsilon axe and others) and separately for the Iranian peoples (Persians, Scythians, etc: sagaris). Axes certainly existed in the Greek, Italic, and Celtic worlds, where they were used as tools or as a form of currency; unfortunately I'm unaware of any evidence of use in battle.

In my mod I had already made axe templates and applied it to the Hyrcanian cavalry (the Persian “sword” cavalry; its visual actor clearly has an axe). In my definition, a sword can be used for stabbing and is therefore double-edge (and symmetrical); the Gaulish mercenary cavalry clearly has a sword, as do most other sword cavalry units. The Iberian mercenary cavalry, however, uses the so-called falcata (a name introduced in the 19th C AD), which is a single-edged, concave scythe-sword which can only be used for cutting and hacking, but not for stabbing, so I figured defining it as an axe is justifiable (although I'm not entirely happy with it).
The falx and rhomphaia (Thracian black cloak) are also single-edged, concave scythe-swords, although typically much longer and two-handed; I'm at a loss what to do with them, because they're neither axe, spear, nor sword.

The real problem is in distinguishing the Celtic and Italic mercenary infantry. For both the use of true swords is well attested, and evidence for battle axes is lacking; besides, both units have a visual actor which uses a sword. I'm actually quite unsatisfied with my solution of renaming the Gaulish mercenary to “axe”; it's a temporary solution, until I've found something better.
Of course, the easiest solution is simply to remove the Italic mercenary (I wouldn't suggest that). Alternatively, replace it with a spearman or hoplite (yes, the Samnite gladiator type (AD) had a gladius (sword), but the Samnites (BC) themselves primarily fought as hoplites), however, there already exists the default Lybian spearman.

The above was (and is) intended as an explanation why I left the black cloaks and Carthaginian mercenaries untouched in this diff proposal.

PS This is actually the “short answer”; maybe I ought to write a longer version and post it somewhere else. This phabricator comment format is totally unsuitable for discussing or explaining something.

Maps? Forgive me my ignorance, but why and what do I have to look for in there?

Maps place units by using their template names (with specific things for scenario...)

I'm quite happy with implementing the hoplite template (which is different from the spearman in my mod, but remains statistically similar in this diff proposal).

I am not really seduced by that as it would be better to have non specific things

The usage of axes as battle weapons [...]

Thanks for the (short) answer!

Thank you for your quick reply :)

Maps? Forgive me my ignorance, but why and what do I have to look for in there?

Maps place units by using their template names (with specific things for scenario...)

Good to know, I'll have a look at them.

I'm quite happy with implementing the hoplite template (which is different from the spearman in my mod, but remains statistically similar in this diff proposal).

I am not really seduced by that as it would be better to have non specific things

Yes, I could understand that. Personally I consider “hoplite” a general term, applicable also to other similar units. Anyway, in this diff proposal I've *not* implemented a separate “hoplite” template, although I have used the hoplite name in the name of several champions (e.g. Immortal a spearman and Kardakes an hoplite); I suppose you consider this a somewhat “dirty” compromise?

The usage of axes as battle weapons [...]

Thanks for the (short) answer!

Well, I'm still open to suggestions what to do with black cloaks and Italic mercenary swordsmen...

Nescio updated this revision to Diff 3673.Sep 15 2017, 10:20 PM
Nescio edited the summary of this revision. (Show Details)

Checked and corrected the /maps/ files for occurances of champion, kardakes, marine, merc, thorakites, thureophoros, and pers_cavalry_*_trireme. (That was a bit annoying, although not as much work as feared.)

Owners added a subscriber: Restricted Owners Package.Sep 15 2017, 10:20 PM
In D906#35493, @Vulcan wrote:

Build has FAILED

Link to build: http://jenkins-master:8080/job/phabricator/2033/
See console output for more information: http://jenkins-master:8080/job/phabricator/2033/console

How can I figure out what I did wrong and what I have to do for a successful build? Those links do not seem to work.

macedonian cavalry javelinists are mercenaries

(btw kardakes are tagged somewhere mercenaries but shouldn't from an history pov, right? But we'll avoid that kind of discussion here)

To clarify: this patch renames civ unit templates but does not change classes or other statistics of the templates. All units which had the “Mercenary” visible class still have it. Templates with *_merc_* in their name were actually a minority of the mercenary units and this inconsistency is removed by this patch; mercenary templates now follow the same template naming as their non-mercenary counterparts.

(And historically the Persians employed mercenary Greek hoplites for nearly two centuries; the “Kardakes” were most likely a non-Greek corps equiped and fighting in Greek style, to supplement the Greek mercenaries; hence their name.)

Anyway, I still don't understand Vulcan's “Build has FAILED” message and I would appreciate help to solve it.

sorry if i wasn't clear. I meant that this unit template name is inconsistent because it is a mercenary but not mentioned on its template name:
https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/units/mace_cavalry_javelinist_b.xml

Yes, I understand that. That's actually the default naming so there is no need to rename that template. With this patch the inconsistency is removed because “mercenary” is no longer reflected in *any* template name.
Currently in 0 A.D. mercenaries are nothing special, they behave the same as their non-mercenary counterparts.

@Nescio :
same as in the other patch, we are lazy, could you sum up the change (that kind of move templates is hard to read on phab)?

To summarize, this patch renames civ-specific unit templates for consistency, e.g.:

maur_champion_infantry.xml
maur_champion_infantry_barracks.xml
maur_champion_maiden.xml
maur_champion_maiden_archer.xml
maur_champion_maiden_barracks.xml

are renamed to:

maur_champion_archer_infantry
maur_champion_mace_infantry
maur_champion_mace_infantry_barracks
maur_champion_sword_infantry  
maur_champion_sword_infantry_barracks

Actually you yourself requested the champion renaming here earlier:

About the champion namming I would really like if it was done :)
There were some issues iirc as for example with the two mauryan infantry champions.
For the mercenary I have no idea. There are at least two annoying things:

  • there are champ and cs mercenaries
  • there are redundant type of mercenaries (for carth)
Nescio edited the summary of this revision. (Show Details)Sep 25 2017, 12:29 PM

Thanks for the sum up, it makes thing easier to review/include.

Actually you yourself requested the champion renaming here earlier:

Sure. I would be happy if you could solve the whole puzzle ;-)

Those merc champs are still annoying.

The carth one could perhaps be solved with your historic input. That would require a little design proposal (ping @Itms).

I (but that's personal) think we shouldn't have two similar units for a said civ (I thought the converse one year later). (By similar, I mean for example 2 swordman champs). But you can't hardly do something against that. (who can?). That would at least makes thing easier.
Perhaps we need another generic template for the maceman if we use that name in child template.
Another thing is to say that we have for example max 2 infantry and max 2 cavalry champ by civ and use infantry_01 infantry_02 for the name.
At least, a perhaps dedicated subfolder could be usefull for scenario/atlas/funny units (else we can have easter egg units popping up in captured buildings).

So to be honnest, wait a bit before updating the patch, as differents issues are intertwined (easily solvable with your own mod, harder in the public mod).

Nescio added a comment.EditedSep 25 2017, 3:49 PM

So to be honnest, wait a bit before updating the patch, as differents issues are intertwined (easily solvable with your own mod, harder in the public mod).

Yeah, I'm fully aware what works in a mod is not necessarily suitable for the main distribution.

Thanks for the sum up, it makes thing easier to review/include.

You're welcome.

Sure. I would be happy if you could solve the whole puzzle ;-)

Starting from scratch is much easier than changing an existing complex in small steps :)

Those merc champs are still annoying.

Which ones specifically?

The carth one could perhaps be solved with your historic input. That would require a little design proposal (ping @Itms).

I (but that's personal) think we shouldn't have two similar units for a said civ (I thought the converse one year later). (By similar, I mean for example 2 swordman champs). But you can't hardly do something against that. (who can?). That would at least makes thing easier.

Currently Carthage has two sword infantry mercenaries and two sword cavalry units (by choosing the Iberian and Italiote embassies and ignoring the Gaulish one they have a full roster; I agree this could have been designed better). There are several easy options I can think of:

  • disable the Gaulish embassy (I'd recommend against, as it limits a potentially interesting choice)
  • replace one of the swordsmen with a spearman (does not solve the problem, because Carthage already has a spearman)
  • rename them to:
cart_balearic_slinger
cart_gaulish_infantry
cart_gaulish_cavalry
cart_iberian_infantry
cart_iberian_cavalry
cart_italic_infantry
cart_italic_cavalry

(works, but is ugly and inconsistent with other mercenary templates)

  • remove the “cart” part:
balearic_sling_infantry
gaulish_sword_infantry
gaulish_sword_cavalry
iberian_javelin_infantry
iberian_sword_cavalry
italic_sword_infantry
italic_spear_cavalry

Carthage is the only faction which can construct embassies, however, any faction which controls an embassy can train its neutral (non-cart) mercenary units.

  • create new unit types (e.g. axe, longsword) and redefine one of the duplicates (compromise historical authencity if necessary)

As you can see, each of these options is far from perfect, so I'm hoping someone has another suggestion.

Perhaps we need another generic template for the maceman if we use that name in child template.

Yes, I could do that, and perhaps also one for the hoplite (e.g. Kardakes, Sacred Band), distinct from the spearman (e.g. Fanatic, Immortal).

Another thing is to say that we have for example max 2 infantry and max 2 cavalry champ by civ and use infantry_01 infantry_02 for the name.

Actually I've toyed with that idea in my mod as well, and implemented it for ships (docks can train ship_01, ship_02, ship_03, etc. instead of bireme, trireme, quinquereme, etc.), but rejected it for other units, because it actually complicates things and increases the numbers of unwanted “easter egg units popping up in captured buildings”. The choice of the numbers is crucial and has to be reviewed every time an unit is added. E.g. the Gauls have two infantry champions (fanatic, swordsman), the Athenians four (guard, marine, Scythian, thureophoros); suppose they're numbered in that order, then Athenians can train hoplites at captured taverns and marines at Gaulish forts; with different number choices there is still a similar problem.

At least, a perhaps dedicated subfolder could be usefull for scenario/atlas/funny units (else we can have easter egg units popping up in captured buildings).

You mean the Spartan pikeman and other existing but unavailable units?

Admittedly, the production queues of captured buildings are a bit of a problem. {civ}_champion_* is problematic because it allows the player to train its supposedly unique units at occassionally unrelated buildings of other civs; athen_champion* is problematic because it allows a player to train units of a different faction.
Things would be less complicated if it were possible to specify a civ or tech requirement inside the production queue of structure templates.

It's nice to discuss with you. You have put all the ingredients in the pot.
(I'll answer a bit later.)

You think so? Personally I think my posts are too long posts with too limited mark up (phabricator is unsuitable for clear discussions) ...

bb requested changes to this revision.Dec 26 2017, 10:48 PM
bb added a subscriber: bb.

Move files instead of adding/deleting

imo the naming should be athen_champion_infantry_hoplite.xml (as a infantry is a bigger class that hoplite)
_war_elephant => _elephant
juggernaught => its a ship
spear vs spearman, sword vs swordsman => choose a convention (idc which)

This revision now requires changes to proceed.Dec 26 2017, 10:48 PM
Nescio updated this revision to Diff 5886.Feb 23 2018, 1:00 PM
Nescio retitled this revision from templates/units/ naming consistency: champion, marine, trireme, and merc units to templates/units/ champion naming consistency.

Updated. Champion naming scheme standardized, e.g.:

maur_infantry_maceman_champion.xml

(instead of maur_champion_infantry.xml).

Exceptions:

  • athen_infantry_marine_champion (because we don't want other factions to be able to train their swordsmen at captured Athenian docks)
  • gaul_infantry_fanatic_champion (because it's rather unique)
  • pers_infantry_immortal_champion (because it's quite different from ordinary spearman champions)
  • ptol_ship_juggernaut (because it's a ship)
Nescio edited the summary of this revision. (Show Details)Feb 23 2018, 1:03 PM
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/75/display/redirect

Stan added a subscriber: Stan.Feb 23 2018, 3:15 PM

Could also be athen_infantry_swordsman_marine_champion
Easier to look for in Atlas if you want swordsman units.

In any case that's a nice patch.

Could also be athen_infantry_swordsman_marine_champion

Personally I'd favour simply athen_infantry_swordsman_champion, without “marine”, and remove the unit from the Athenian docks — but then this patch would change gameplay :)

Stan added a comment.Feb 23 2018, 5:29 PM

Well since it's a marine unit it wouldn't make sense.

They are not mercs either so that wouldn't work

But actually I remember Mimo saying that there is a way to do what you wanr

s0600204 requested changes to this revision.Mar 19 2019, 12:57 AM
s0600204 added a subscriber: s0600204.

A valiant effort!

So, firstly, the patch no longer applies cleanly (maur_barracks creates a rejected hunk, and kush_temple_apedemak is now known as kush_temple).

Secondly, I'm a little concerned about the Spartan sword champion (as was). I understand why you've named it what it is, but the new name is technically incorrect. There was some discussion on the forums from November 2016 to May 2017 (unfortunately in the staff-only area, sorry) about that unit. Essentially, it's supposed to be on par with other sword champions in the game, but - for historical accuracy - have the abilities of citizen-soldiers (gather, build, etc).

Anyhow, the conclusion at the end was that the template name should be changed. It then wasn't. I'd say give it the correct name (_e suffix) and add it back to the Spartan barracks so they can train it. in case you're worrying about this being game-changing... well, so is renaming it with the _b suffix.

And finally (for now), as remarked in the comments above, there are various maps that will be broken by this changeset (mostly scenarios). (I think you did include corrections in an earlier version of the patch, but they're no longer present.) As such, this revision probably won't be committed until that's resolved. Now, this revision is large enough as it is, so if you could create a new revision with the updates to the map files necessary, and make it depend on this one, that would be awesome.

And thank you for the time you've spent so far on this.

This revision now requires changes to proceed.Mar 19 2019, 12:57 AM
Nescio abandoned this revision.Jan 18 2021, 6:35 PM

This patch is heavily outdated, rebasing is more work than just doing an entirely new patch, all civ templates have been moved, many renamed more than once, most recently in D3384/rP24657, which partly implemented what was proposed here, though for different reasons.