Page MenuHomeWildfire Games

Prevent template editors from tagging military units as female support units
ClosedPublic

Authored by elexis on Mar 21 2017, 3:13 PM.

Details

Summary

As noted in #4490 and D242, the Female class is used to check for female support units and only those.
In particular the Petra bot assumes that.
However it is not inherently obvious that the proper noun Female is different from the common noun female in our case and
so people have confused it and added it to military females in rP19095 or intentionally used it for military females like in the Delenda Est mod for example.

Using precise language not only helps preventing future repetitions of this ambiguity but also allows modders to add a auras, techs or attack bonuses
for all kinds of females without having to use a workaround name.

Removal of female class proposed separately in D242.

Test Plan

Do a case-sensitive search for Female in all js xml and json files. Test that the summary screen still works as expected.

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

elexis created this revision.Mar 21 2017, 3:13 PM
Vulcan added a subscriber: Vulcan.Mar 21 2017, 5:35 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/562/ for more details.

fatherbushido requested changes to this revision.Mar 21 2017, 7:15 PM
fatherbushido added a subscriber: fatherbushido.

I was not personally disturbed by that but having less confusing naming is better.

FemaleCitizen sounded good too. (Female Citizen is a string used several times in some files). (1)

(((One can also rename all the female template and the related class and go with something politically correct not refering to gender (I am not for that).)))
((Seems to be an internal class so no need to move to VisibleClasses, in that case adding something translatable to the related unit can be done too.))

You must change (and so merge) also the affects of the tech simulation/data/technologies/health_females_01.json (2)
You can also edit simulation/components/tests/test_Attack.js (but not mandatory) (3)

(acked, checked line by line, run game with the AI, and all that kind of things I hope you know I did).

Consider (1) (3), request change for (2), else "Accepted" (I don't like the superiority flavour of that word...)

binaries/data/mods/public/simulation/components/Identity.js
58 ↗(On Diff #877)

alphabetical order sounds better

binaries/data/mods/public/simulation/components/TrainingRestrictions.js
10 ↗(On Diff #877)

((((I wonder what was that training restriction about))))

This revision now requires changes to proceed.Mar 21 2017, 7:15 PM
elexis marked 2 inline comments as done.Mar 22 2017, 3:23 PM
elexis added inline comments.
binaries/data/mods/public/simulation/components/TrainingRestrictions.js
10 ↗(On Diff #877)

Introduced by rP12832, explained by #1432#comment:31:

There is the females limit in the patch - just for testing, I'm going to remove it before committing.

That was the only remaining occurance of FemaleCitizen in that commit.

binaries/data/mods/public/simulation/templates/units/brit_hero_boudicca.xml
10 ↗(On Diff #877)

Should probably be done separately in D242, perhaps Boudica has something to say about her template x)

elexis updated this revision to Diff 888.Mar 22 2017, 3:26 PM
elexis edited edge metadata.
elexis marked an inline comment as done.

Rename SupportFemale to FemaleCitizen.
Add the change to the forgotton JSON file.
Apply the change to test_Attack.js for consistency.
The rename incidentally restores alphabetical order in Identity.js.

elexis updated this revision to Diff 889.Mar 22 2017, 3:33 PM

Rename Female to FemaleCitizen instead of FemaleSupport in test_Attack.js.

fatherbushido accepted this revision.Mar 22 2017, 5:30 PM
This revision is now accepted and ready to land.Mar 22 2017, 5:30 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/569/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/570/ for more details.

This revision was automatically updated to reflect the committed changes.

Thanks for the review! I can sleep much better with this identity class!

user1 added a subscriber: user1.EditedApr 17 2017, 10:49 PM

Turns out this change breaks the ratings bot.

The statistics that are gathered and ultimately stored are generated from the class names and the ratings bot uses these same names in its database.

This is breaking ratings bot becase the sqlite db keys are hardcoded when the ratings db is created by source/tools/XpartaMuPP/LobbyRanking.py and EcheLOn.py is using the generated names to lookup(and store) these ratings.

femaleUnitsTrained, femaleUnitsLost, and enemyFemaleUnitsKilled become femaleCitizenUnitsTrained, femaleCitizenUnitsLost, and enemyFemaleCitizienUnitsKilled.

LobbyRanking.py and EcheLOn.py need to be updated as well as the sqlite db being updated to reflect the new name.