Ptolemian units
ClosedPublic

Authored by fatherbushido on Jan 9 2017, 3:44 PM.

Details

Summary

As said in the forum, it was decided but never done to remove the nubian archers (it's before the -500 / -1 timeframe), to replace it with the cretan archers and to move instead a javelinist in the cc. The provided patch do the said modification. I renamed the old nubian archers to keep it for atlas/fun (I checked, there was only the basic actor).

Also, in last enrique art commit, a new pikemen champ was introduced, perhaps can we include it? (It fits well with the other ptolemaic champ).

https://wildfiregames.com/forum/index.php?/topic/18242-petition-removal-of-mercenary-nubian-archer-from-ptolemaic-army/

Test Plan

-

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.
fatherbushido updated this revision to Diff 180.Jan 9 2017, 3:44 PM
fatherbushido retitled this revision from to Ptolemian units.
fatherbushido updated this object.
fatherbushido edited the test plan for this revision. (Show Details)
fatherbushido set the repository for this revision to rP 0 A.D. Public Repository.
Vulcan added a subscriber: Vulcan.Jan 9 2017, 4:28 PM

Build is green

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

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

I'm probably quite out of touch for balance, but I envisioned the mercenary archers to be a bit like a handicap early game for Ptolemies (low health, costs metal) in terms of early game military might (with the exception of camel archers). With free basic structures, are there any handicaps for Ptolemies to stop developing quicker than other civs?

@scythetwirler
Thanks for your input!
Handicap: Long time to build buildings.
In the above patch as the javelinist is a mercenary, metal cost too.

Ah ok. Sounds good. I'll review the other parts soonish.

binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_nubian.xml
19 ↗(On Diff #180)

Does this cause it to promote from a Nubian archer to an advanced Cretan one?

Ah ok. Sounds good. I'll review the other parts soonish.

oh yes, should remove that.

elexis edited edge metadata.Jan 11 2017, 6:14 AM

About the archers:
It's good to be able to produce skirmishers from the civic center. This will be the first citizen soldier unit (besides camel archers) that Ptolemians can use. The pikemen are extremely slow (scythe can we buff them a little more? supertux was right at the time, but their buff wasn't big enough IMO. Just a tiny touch, perhaps +10%?) so that using them for aggression or economy is discouraged. Since the archers cost metal and that being often precious, players started training infantry only from the barracks.

About the champion pike:
I'd split the champion infantry pikemen into a separate commit.
While making it available, we should also check that it's healthy to the game balance. Guess they're identical to seleucid ones, so should work out. Their strength is basically invincibility, so I'm ok with them being slow af.

Also the newly added file doesn't apply using arc patch and the context is not available in the diff, try to use the arc diff way.

binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_nubian.xml
19 ↗(On Diff #180)

Wait, the old, then unused file wasn't actually replaced but only the reference to it? Sounds wrong if we don't want to see that unit anymore.

fatherbushido added a comment.EditedJan 11 2017, 7:13 AM

Also the newly added file doesn't apply using arc patch and the context is not available in the diff, try to use the arc diff way.

Here i had issues with renaming.

About the champion pike:
I'd split the champion infantry pikemen into a separate commit.
While making it available, we should also check that it's healthy to the game balance. Guess they're identical to seleucid ones, so should work out. Their strength is basically invincibility, so I'm ok with them being slow af.

I was not so convinced of the need of it. But as Enrique included it in art, as it is nice and fits well, as it will not really change balance and as it was trainable as an easter egg, it seems ok to include it.
From balance, yes it's the same as the seleucid one.

The pikemen are extremely slow (scythe can we buff them a little more? supertux was right at the time, but their buff wasn't big enough IMO. Just a tiny touch, perhaps +10%?) so that using them for aggression or economy is discouraged.

I really like the fact to see them slow. But why not if we still notice that it's a really slow unit.

fatherbushido added inline comments.Jan 11 2017, 7:14 AM
binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_nubian.xml
19 ↗(On Diff #180)

atlas,scenario units

fatherbushido added inline comments.Jan 11 2017, 3:58 PM
binaries/data/mods/public/simulation/templates/structures/ptol_fortress.xml
19 ↗(On Diff #180)

should be units/{civ}_champion_infantry_pikeman

elexis requested changes to this revision.Jan 11 2017, 6:26 PM
elexis edited edge metadata.

That what scythe said, the pikeman aren't actually trainable form the fort due to a typo. Perhaps you can just commit the correct ptol fort enty and correct and reupload the archer patch.

binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_nubian.xml
19 ↗(On Diff #180)

Just saying, since there was this other post about removing unused templates. But I agree that keeping some of them is fun, even if not in the historical timeframe. Perhaps we can move them to a new directory to separate the ones relevant to the timeframe and civs (independent of being trainable, only available in scenario maps or being unused) from those ones not fitting to the timeframe and civs; so that people wanting to help the games balancing are not confused with weird entries?

This revision now requires changes to proceed.Jan 11 2017, 6:26 PM
fatherbushido edited edge metadata.

update (fix typo and promotion stuff)

Build is green

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

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

fatherbushido added inline comments.Jan 11 2017, 8:46 PM
binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_nubian.xml
19 ↗(On Diff #180)

Sure, that's the plan.

fatherbushido edited edge metadata.
fatherbushido removed rP 0 A.D. Public Repository as the repository for this revision.

(edit something with portraits)

update with portraits stuff

fatherbushido added a comment.EditedJan 15 2017, 1:10 PM

Uhm I wonder if it's ok to add the champ pikemen are we will have more than a row of trainable units.
(And keeping two rows for techs is better, for pair techs).
(perhaps we can make basic pikemen upgradable / promotable(by xp or tech) to those one?)
So perhaps in a first time decide on the other part of the diff.

Build is green

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

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

Or we could move most heroes to the CC (like Macedonians currently).

fatherbushido added a comment.EditedJan 15 2017, 7:29 PM

Or we could move most heroes to the CC (like Macedonians currently).

Oh yes, I m all for that

EDIT: in the scope of the diff, I do it only for ptols

update, move heros to cc

Build is green

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

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

scythetwirler requested changes to this revision.Jan 19 2017, 6:48 PM

I'm not really liking having 9 units in the mercenary camp stretching onto two lines...perhaps we can remove the non-mercenary units from that building?

binaries/data/mods/public/simulation/data/civs/ptol.json
176 ↗(On Diff #260)

Should probably be changed the skirmisher.

This revision now requires changes to proceed.Jan 19 2017, 6:48 PM
fatherbushido edited edge metadata.

update

Sure, I forgot token concatenation. Uhm, I should put back the women or not?

I'd say nah. What's the purpose behind the tokens deletion line, btw?

I'd say nah. What's the purpose behind the tokens deletion line, btw?

So it doesn't concatenate the civil centre (parent) training list.

I'm still getting 9 units for the mercenary camp. I'll check on my end to see if it's a caching issue or something.

I'm still getting 9 units for the mercenary camp. I'll check on my end to see if it's a caching issue or something.

If you do it in atlas, use ptol_military_colony and not ptol_mercenary_camp

Ah okay, I was using the sandbox 2 for ptol.

scythetwirler accepted this revision.Jan 19 2017, 7:56 PM

Build is green

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

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

Update (use replace instead, cleaner)

Build is green

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

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

elexis requested changes to this revision.Jan 20 2017, 4:41 PM
  • Wouldn't remove the woman from the colony, as that building can be used to rebuild society after getting the CC destroyed. If we remove the woman, rebuilding with 5 units will be much harder. Also there seems to be no reason for removing the woman (anymore I guess).
  • The new ptolemian infantry champion now trainable by players is well balanced:
    • The damage output is very low in comparison to all other champions, but seems good enough, as
    • the peculiarity of that champion to be really tanky with 75% armor is present.
    • The citizen soldier pikeman has 10 10 15 armor, the champion one 13 13 20, so that seems well adjusted.
    • Training time and cost are inherited, so the same costs as a typical champion.
  • Didn't realize the skirmishers were mercenaries too. What was the intention behind the switch from barracks to civic center and vice versa?
  • The commit message should explain all changes briefly, so that the reader doesn't have to puzzle that out from the diff or this revision. For that reason, splitting it into two commits would be advantageous:

1:

  • Allow training the new Ptolemian Champion Pikemen from the fortress
  • Train Ptolemian heroes from the civic center instead of the fortress

2:

  • Replace the ashitorical Numibian Archer with the Macedonian Mercenary Cretan Archer
  • Train mercenary skirmishers at the Civic Center and the mercenary archers at the barracks
binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_b.xml
23 ↗(On Diff #260)

Why is it called Nubian Mercenary Archer if the intention was to remove the ahistorical unit?
Can't we inherit the macedonian template, so as to avoid the redundancy (to prevent forgetting to change one stat in the other file)?

This revision now requires changes to proceed.Jan 20 2017, 4:41 PM
fatherbushido added inline comments.Jan 20 2017, 7:17 PM
binaries/data/mods/public/simulation/templates/units/ptol_infantry_archer_b.xml
23 ↗(On Diff #260)
  1. mistake (perhaps it wasn't in the first one, I wanted first use rename stuff)
  2. I don't really like that (a bit horizontal inheritance)
fatherbushido edited edge metadata.

update

Build is green

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

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

This revision is now accepted and ready to land.Jan 23 2017, 7:12 PM
elexis accepted this revision.Jan 24 2017, 3:27 AM
elexis added a reviewer: elexis.
elexis added a subscriber: elexis.

Thanks for fixing the woman in the colony and the wrong name (which were the only things that I required to change before accepting it, other ones are suggestions. Indeed phabricator might become confused when splitting the diff in two commits.)
Tested the patch again and everything is fine now (doesn't have regression tests though :P)

Happy to finally see ptolemian infantry champions!

elexis accepted this revision.Jan 24 2017, 3:31 AM
This revision was automatically updated to reflect the committed changes.
elexis changed the visibility from "All Users" to "Public (No Login Required)".Jul 19 2017, 3:09 PM
elexis edited the summary of this revision. (Show Details)Jul 19 2017, 3:15 PM