Page MenuHomeWildfire Games

Promote mercenaries with a technology to advanced rank
ClosedPublic

Authored by Grugnas on May 2 2017, 10:03 PM.

Details

Summary

This patch intrdouces a new technology that promotes mercenaries to advanced rank but increases their training time by 20%, and a technology specific for Carthage that promotes advanced rank mercenaries to elite rank but increases their training time by another 30%.

Test Plan

Being classified as mercenary, besides the 50% of food cost replaced with metal cost, is sign of vetaran soldiers serving their experience to a faction, which should be seen as a peculiarity and not as a ballast.
Units lost during a game session are enough to keep players from prefer an army of mercenaries instead of investing metal in more powerful units.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

Imo don't join the picture to that patch, I can't commit art things.

binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
4 ↗(On Diff #1843)

spaces

5 ↗(On Diff #1843)

spaces

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!

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

Grugnas updated this revision to Diff 1847.May 11 2017, 3:04 PM

spaces fixed and placeholder used.

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!

http://jw:8080/job/phabricator/1139/ 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!

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

fatherbushido added inline comments.May 11 2017, 6:57 PM
binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
12 ↗(On Diff #1847)

put ], in the next line

4 ↗(On Diff #1843)

no space before ] or after [

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!

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

fatherbushido added inline comments.May 11 2017, 9:14 PM
binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
4 ↗(On Diff #1843)

spaces before }

Grugnas updated this revision to Diff 1854.May 11 2017, 9:17 PM

more spaces

fatherbushido added inline comments.May 11 2017, 9:18 PM
binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
4 ↗(On Diff #1843)

again

Grugnas updated this revision to Diff 1855.May 11 2017, 9:30 PM

spaces?

Grugnas updated this revision to Diff 1856.May 11 2017, 9:36 PM

last space, i guess

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!

http://jw:8080/job/phabricator/1149/ 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!

http://jw:8080/job/phabricator/1150/ 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!

http://jw:8080/job/phabricator/1151/ 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!

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

Imarok added a subscriber: Imarok.May 13 2017, 6:37 PM

Is it intended that all already produced mercenaries will rank up to advanced when this tech is researched?

In D407#19692, @Imarok wrote:

Is it intended that all already produced mercenaries will rank up to advanced when this tech is researched?

I guess so.

In D407#19692, @Imarok wrote:

Is it intended that all already produced mercenaries will rank up to advanced when this tech is researched?

actually yes.
I guess there is no way to let an eventually affected structure to train advanced mercenaries since the applymodification doesn't replace strings as far as i know.
Also, the same side effect applies when the Agoge technology is researched for sparta and SIlver Shield technology is researched for Macedonians.

In D407#19692, @Imarok wrote:

Is it intended that all already produced mercenaries will rank up to advanced when this tech is researched?

I guess so.

The tech description reads like only newly produced once will be at advanced rank. So I'd either change the tech description or the tech functionality.
(Won't be this a way to work around the increased train time? Just first produce the mercs and then research the tech...)

In D407#19702, @Imarok wrote:
In D407#19692, @Imarok wrote:

Is it intended that all already produced mercenaries will rank up to advanced when this tech is researched?

I guess so.

The tech description reads like only newly produced once will be at advanced rank. So I'd either change the tech description or the tech functionality.
(Won't be this a way to work around the increased train time? Just first produce the mercs and then research the tech...)

Yes.
For the second comment it's often a problem with self balancing techs :) (cost vs bonus) but I m fine with that.

Grugnas updated this revision to Diff 1904.May 13 2017, 7:47 PM

tooltip modified to give a clear understanding of its effect.

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1186/
See console output for more information: http://jw:8080/job/phabricator/1186/console

Grugnas retitled this revision from Train Advanced mercenaries instead of base rank to Promote advanced mercenaries with a technology.May 14 2017, 3:46 PM
Grugnas edited the summary of this revision. (Show Details)

Sounds ok, simple and clear enough, letting choice to player. I would commit that.
Other opinions? @mimo @scythetwirler @elexis ?

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1245/
See console output for more information: http://jw:8080/job/phabricator/1245/console

elexis accepted this revision.May 19 2017, 1:09 PM

Persians also have mercenaries, but those are champions, so okay.
Tested and works. Checked all templates and complete.
Why isn't the elite rank tech added? If it's OP, make it really expensive.

Created new icons for both techs.

That arrow might be hard to notice if scaled down for the tech panel. Should have more contrast or a different color (golden?) ideally. But I guess the artist goes on vacation next week.

The numbers were confirmed by borg- right? I recall him playing two games against Grugnas with this patch.

binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
9 ↗(On Diff #1904)

get -> require

14 ↗(On Diff #1904)

indeed upgradearmoy sounds better than techcomplete

4 ↗(On Diff #1843)
{
	"all": [
		{ "tech": "phase_town" },
		{ "any": [{ "civ": "athen" }, 	{ "civ": "cart" }, { "civ": "mace" }, { "civ": "ptol" }, { "civ": "sele" }] }
	]
},

?

In D407#20895, @elexis wrote:

Persians also have mercenaries, but those are champions, so okay.
Tested and works. Checked all templates and complete.
Why isn't the elite rank tech added? If it's OP, make it really expensive.

There was a discussion about introducing the elite rank tech added as Carthaginian only peculiarity tech.
The reason of this choice is because Carthaginians, despite they inspired the introduction of the patch and historically notorious for their wealth and mercenaries usage, citizen soldiers mercenaries can be trained very easy because there isn't any building limit on buildings capable to train mercenary citizen soldiers.
The Elite Mercenaries tech would be a peculiarity of Carthage only as civ bonus and eventually followed by an hero to promote a mercenary based strategy.

Grugnas updated this revision to Diff 2037.May 19 2017, 9:38 PM

Elite Mercenaries tech added for Carthage only for 500 wood 500 metal.
Mercenaries train 50% slower than basic units and rise mercenaries metal cost by 40%.

In D407#21055, @Grugnas wrote:

Elite Mercenaries tech added for Carthage only for 500 wood 500 metal.
Mercenaries train 50% slower than basic units and rise mercenaries metal cost by 40%.

There is a thing called "plan changes" :/

I am sorry for that. The main idea was to upload it once the Advanced rank Tech was accepted and commited since it it relies on this diff.
I can assure you that there will not be any other change on this diff though.

In D407#21075, @Grugnas wrote:

I am sorry for that. The main idea was to upload it once the Advanced rank Tech was accepted and commited since it it relies on this diff.
I can assure you that there will not be any other change on this diff though.

np, I was waiting anyway ;-)
But now, all need to be checked again

fatherbushido added inline comments.May 19 2017, 10:50 PM
binaries/data/mods/public/simulation/data/technologies/upgrade_rank_elite_mercenary.json
2 ↗(On Diff #2037)

names of tech are both ok but a bit inconsistent

5 ↗(On Diff #2037)

Ok.

There is two kind of techs:

  • special tech or civ bonus
  • common techs.

That one would be of the first type. I really like that kind of techs, but they should be rare (prone to mess, hard to maintain or to balance...). Moreover doing so we decide to change the design (giving a speacial tech or a civ bonus).

Why not (it makes sense), but it should be acted as such.

13 ↗(On Diff #2037)

1.2 * 1.5 = 1.8
So build time is increased by 80% from base.
It's to consider but seems ok.

14 ↗(On Diff #2037)

As you spend the metal on the techs, it's perhaps useless.

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!

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

Grugnas added inline comments.May 20 2017, 10:54 AM
binaries/data/mods/public/simulation/data/technologies/upgrade_rank_elite_mercenary.json
2 ↗(On Diff #2037)

the intended name was "Art of War"

13 ↗(On Diff #2037)

The affecting target logic is different from the tech advanced/elite_unit_bonus.
While the advanced_unit_bonus affects also elite units, the upgrade_rank_elite_mercenary tech doesn't have any info about the upgrade_rank_advanced_mercenary tech going in that way to affect directly basic units. The elite 1.5 refers to the basic mercenary unit:
infantry basic 10 sec -> advanced 12 sec -> elite 15 sec + 35 metal (same price and metal cost of skiritai). The comparison with the skiritai is the reason why of the 1.4x metal cost (by the way i didn't test it in multiplayer, so i can't give an accurate response about metal cost increase worthness).

Grugnas updated this revision to Diff 2044.May 20 2017, 11:07 AM

technology moved in the proper folder since it is a civ bonus. Technology name changed into "Art of War".
Extra metal cost removed because, as fatherbushido pointed out, there is already a consistent amount of metal investend into the technologies research, moreover the "Art of War" technology requires City phase which foresee metal usage.

Grugnas retitled this revision from Promote advanced mercenaries with a technology to Promote mercenaries with a technology to advanced rank; up to elite rank for Carthage only.May 20 2017, 11:11 AM
Grugnas 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!

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

(One could also differentiate the techs for the 3 embassies for carth...).
I will suggest to wait a bit finally and include that in a more global thing.

fatherbushido accepted this revision.Jun 1 2017, 9:39 AM

Accepted for the advanced part.

This revision is now accepted and ready to land.Jun 1 2017, 9:39 AM
Grugnas updated this revision to Diff 2350.Jun 1 2017, 12:59 PM

split elite merc diff from the currend diff.

Grugnas retitled this revision from Promote mercenaries with a technology to advanced rank; up to elite rank for Carthage only to Promote mercenaries with a technology to advanced rank.Jun 1 2017, 1:00 PM

(One could also differentiate the techs for the 3 embassies for carth...).
I will suggest to wait a bit finally and include that in a more global thing.

My plan would be to extend the Embassy count limit from 2 to 3 and limit the single civ embassy to 1 entity only in order to have 3 different embassies with part of their cost in metal.
They would share the same promoting mercenaries technology because units trained may change due the situation while research 3(or even 6) different technologies would be probably too costly.

fatherbushido accepted this revision.Jun 1 2017, 1:19 PM
In D407#24092, @Grugnas wrote:

(One could also differentiate the techs for the 3 embassies for carth...).
I will suggest to wait a bit finally and include that in a more global thing.

My plan would be to extend the Embassy count limit from 2 to 3 and limit the single civ embassy to 1 entity only in order to have 3 different embassies with part of their cost in metal.
They would share the same promoting mercenaries technology because units trained may change due the situation while research 3(or even 6) different technologies would be probably too costly.

ok, nice ;-) (many people have plans I guess :p)

binaries/data/mods/public/simulation/data/technologies/upgrade_rank_advanced_mercenary.json
12 ↗(On Diff #2350)

(I guess we don't really need the second affects)

Grugnas updated this revision to Diff 2351.Jun 1 2017, 1:29 PM

removed the second affects

Vulcan added a comment.Jun 1 2017, 2:01 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://jw:8080/job/phabricator/1431/ for more details.

wraitii added a subscriber: wraitii.Jun 1 2017, 2:24 PM

Just to mention that D270 and D266 would enable us to do this in a more flexible way than "upgrade any mercenary to advanced permanently and forever".

Vulcan added a comment.Jun 1 2017, 2:49 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://jw:8080/job/phabricator/1432/ for more details.

elexis added a comment.Jun 1 2017, 5:50 PM

Hannibals icon is just too noticeably a copy of the tower upgrade icon and the arrow is almost invisible ingame. So I'd rather use the unused icon in your patch. Other than that it's still correct and complete as mentioned above.
As noted by others, we can first train all units and then upgrade them, but that has the disadvantage to the player that he might lose them in an early surprise battle.
A good thing that the tech choice offers is keeping the basic mercenary actors present.

This revision was automatically updated to reflect the committed changes.