Add Persian and Carthanginian team bonus.
ClosedPublic

Authored by fatherbushido on Jan 2 2017, 7:23 PM.

Details

Summary

From design docs.

Test Plan

Test

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 110.Jan 2 2017, 7:23 PM
fatherbushido retitled this revision from to Add Persian and Carthanginian team bonus..
fatherbushido updated this object.
fatherbushido edited the test plan for this revision. (Show Details)
fatherbushido added a reviewer: mimo.
fatherbushido set the repository for this revision to rP 0 A.D. Public Repository.
fatherbushido updated this object.
fatherbushido edited the test plan for this revision. (Show Details)
Vulcan added a subscriber: Vulcan.Jan 2 2017, 8:06 PM

Build is green

Updating workspaces.
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  Status ret = ERR::FAIL;
         ^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

mimo added inline comments.Jan 2 2017, 8:15 PM
binaries/data/mods/public/simulation/data/auras/teambonuses/cart_player_teambonus.json
6 ↗(On Diff #110)

Internation bonus is a relative factor, usually 25% of the trader gain for each market (and thus this result in a small gain of about 8% of the trader gain). So i think it is clearer to define it additively and increase it a bit, for example "add": 0.10 as is done in the cart market xml.

9 ↗(On Diff #110)

Trade bonus works by market: only the owner of the market, so only the ally, will have the bonus, not the cart player.

binaries/data/mods/public/simulation/data/auras/teambonuses/pers_player_teambonus.json
3 ↗(On Diff #110)

Shouldn't it be "Trader+!Ship". IIRC a blank is interpreted as an OR

6 ↗(On Diff #110)

I agree it is the value in the doc, but this seems to me to be a huge bonus compared to other team bonuses (as the cart one for example). I would decrease it to 1.15

binaries/data/mods/public/simulation/templates/structures/cart_dock.xml
22 ↗(On Diff #110)

As said before, the cart player will not take profit of the team bonus, only its ally. so why removing that one? it is not a double counting.

binaries/data/mods/public/simulation/templates/structures/cart_market.xml
15 ↗(On Diff #110)

same as above, should be kept.

binaries/data/mods/public/simulation/templates/units/pers_support_trader.xml
22 ↗(On Diff #110)

same as above. Should be kept as only the traders of the allies will have the team bonus, while only the pers traders will have that one. It is usually not double counting, except if two pers are allied. Don't know if there is an easy way to prevent that.

mimo added inline comments.Jan 2 2017, 8:28 PM
binaries/data/mods/public/simulation/data/auras/teambonuses/cart_player_teambonus.json
6 ↗(On Diff #110)

of course I meant "a small gain of about 6%" :)

fatherbushido planned changes to this revision.Jan 2 2017, 8:36 PM

Thanks for the review!
I will do the changes, I don't know if you prefer to use MutualAlly in auras or ExclusiveMutualAlly in auras and modifications in templates.

binaries/data/mods/public/simulation/data/auras/teambonuses/cart_player_teambonus.json
6 ↗(On Diff #110)

Sure, my bad!

9 ↗(On Diff #110)

I set it to MutualAlly and not to ExclusiveMutualAlly to do both in the same time. We can still split it, I don't know what is better.

binaries/data/mods/public/simulation/data/auras/teambonuses/pers_player_teambonus.json
3 ↗(On Diff #110)

There are two form, the one used in xml (with strings) and the other (used in json) with array (in that one, the OR is ",") and the AND is the whitespace.

6 ↗(On Diff #110)

ok nice, I m fine with that too

mimo edited edge metadata.EditedJan 2 2017, 8:44 PM

OK sorry, i didn't pay enough attention to the MutualAlly <-> ExclusiveMutualAlly, but I thought the team bonus were supposed to apply to the ally only, so should have ExclusiveMutualAlly. And all my comments were assuming ExclusiveMutualAlly.

Separating both with ExclusiveMutualAlly have two advantages imo:

  • consistency with other teambonuses
  • allow to have different values for the civ bonus itself (the one in the template) and the team bonus which may not necessary be the same.
elexis added a subscriber: elexis.Jan 2 2017, 9:28 PM

(The proposed balancing sounds fine to me.)

fatherbushido updated this revision to Diff 111.Jan 2 2017, 9:35 PM
fatherbushido edited edge metadata.

Should adress the remarks.

Build is green

Updating workspaces.
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  Status ret = ERR::FAIL;
         ^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

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

mimo accepted this revision.Jan 2 2017, 11:11 PM
mimo edited edge metadata.

Code looks fine to me now.
Tested in trade demo map, and didn't noticed anything wrong.

This revision is now accepted and ready to land.Jan 2 2017, 11:11 PM
fatherbushido edited edge metadata.
This revision was automatically updated to reflect the committed changes.
elexis changed the visibility from "All Users" to "Public (No Login Required)".Jun 26 2017, 2:24 PM