Page MenuHomeWildfire Games

Template organization: maceman
ClosedPublic

Authored by Nescio on Nov 14 2017, 10:15 PM.

Details

Summary

Created a new generic template for the champion maceman; currently there is only one (Mauryan Yoddha), but others might be added in the future.
Cf. the generic hero healer template, which is also used only once.

Test Plan

Test if everything works

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 3706
Build 6437: Vulcan BuildJenkins
Build 6436: arc lint + arc unit

Event Timeline

Nescio created this revision.Nov 14 2017, 10:15 PM
Owners added a subscriber: Restricted Owners Package.Nov 14 2017, 10:15 PM
Vulcan added a subscriber: Vulcan.Nov 15 2017, 12:34 AM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
bb added a subscriber: bb.Nov 17 2017, 1:42 PM

googling, maceman indeed says us that the meant unit is called a maceman

binaries/data/mods/public/simulation/components/Identity.js
65

shouldn't that be Maceman then, so avoid confusion with the mace civ (notice we also have Archer, Slave, Trader etc too, which refers to persons, so Maceman is correct too)
this ofc doesn't all needs to be swapped, but this one should be to avoid the confusion

binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_maceman.xml
28

Maceman

binaries/data/mods/public/simulation/templates/units/maur_champion_infantry.xml
5

this should go to the parent, shouldn't it?

Nescio added inline comments.Nov 17 2017, 2:27 PM
binaries/data/mods/public/simulation/components/Identity.js
65

Pike, Sling, Spear, Sword also lack “man”.
Furthermore, ethnic classes (typically non-visible) are written in full, e.g. Iberian, Italian.
So if a Macedonian is to be introduced, it should be “Macedonian”, not “mace”.

binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_maceman.xml
28

See earlier remark.

binaries/data/mods/public/simulation/templates/units/maur_champion_infantry.xml
5

No, not in this case. Sometimes civ specific templates redefine the generic name, cf. pers_champion_infantry.xml

bb added inline comments.Nov 17 2017, 2:33 PM
binaries/data/mods/public/simulation/components/Identity.js
65

But mace is used as reference to macedonian in other code, f.e template names so I guess (and know I would) people would be confused when having the same string (ok cap) for two totally different things. Agree that not all have the "man" but it this case we have a confusion problem. Also you are right when civs are classes they get there full name.

(It is indeed choosing between two evils...)

binaries/data/mods/public/simulation/templates/units/maur_champion_infantry.xml
5

You are right indeed...

Nescio added inline comments.Nov 17 2017, 2:42 PM
binaries/data/mods/public/simulation/components/Identity.js
65

Although I see your point, I don't think it's really a problem here (classes are clearly distinguished from civs).
If it's to be “Maceman”, then probably we should change dozens of files for consistency to have “Spearman” etc. as well.

bb added inline comments.Nov 17 2017, 2:47 PM
binaries/data/mods/public/simulation/components/Identity.js
65

They are and that is why I would say to have the man extension to avoid confusion, and as that is the real only reason there is no need for changing others,

perhaps just meh

Nescio added inline comments.Nov 17 2017, 2:50 PM
binaries/data/mods/public/simulation/components/Identity.js
65

Do classes and template names have to match? That is currently not the case.

bb added inline comments.Nov 17 2017, 2:55 PM
binaries/data/mods/public/simulation/components/Identity.js
65

no since templates can have more than one class and classes can be defined in more than one template. Also classes can be really any string which apply to the to unit, so there is no need to make them matching. It is really just avoiding confusion when looking at two things with exactly the same name, but yet very different meanings (It is nice that at least a barrack template has the barrack class, but a spearman template can have the spear class with is fine too in the same way)

bb accepted this revision.EditedDec 24 2017, 4:32 PM

Going for Maceman

grepped Sword in the sim and the only place currenty used (so non templates) is the steel working tech (this has no effect on sim since maceman just no hack damage currently, but as we are talking about razer sharp things, adding it to macemans is fine. It can be misleading however to mention it in tooltip, since it doesn't change anything, so leaving the tooltip as is). I will add this class there. (Notice there are some wrong names in the tests, but that is out of scope)

binaries/data/mods/public/simulation/templates/template_unit_champion_infantry_maceman.xml
31–32

don't know why, but not wanting to change the sim with it

This revision is now accepted and ready to land.Dec 24 2017, 4:32 PM
This revision was automatically updated to reflect the committed changes.