From design docs.
Details
Details
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
| 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. |
| 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%" :) |
Comment Actions
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 |
Comment Actions
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.
Comment Actions
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.
Comment Actions
Code looks fine to me now.
Tested in trade demo map, and didn't noticed anything wrong.