Details
- Reviewers
- None
- Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP23238: Move hardcoded gain per garrisoned trader on ship to template
Confirm ship with that element and garrisoned trader provides trading bonus for the player.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 10041 Build 16991: Vulcan Build Jenkins Build 16990: Vulcan Build (Windows) Jenkins Build 16989: arc lint + arc unit
Event Timeline
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/163/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/164/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/165/display/redirect
Shouldn't it be Trader-based instead of Merchant-based? That would allow e.g. gaul trader adds +0.15, mace trader +0.20, pers trader +0.25.
@Nescio But you can achieve the same effect with current state of the patch giving the numbers to ships. Current approach could be considered not fully satisfying if one could capture trader or merchant ship, what currently as I know can not. If it would be decided as better solution I would move it to another patch anyway as not everyone could agree with that change.
True, neither traders nor merchant ships are capturable in 0 A.D.
I'm wondering though what would be the most logical place to define this bonus: the entity template that receives it or the entity template that gives it?
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (space-unary-ops): | | Unexpected space after unary operator '+'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 12| 12| "<ref name='positiveDecimal'/>" + | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| |- "<element name='GainMultiplierPerGarrisonedTrader'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 15|+ "<element name='GainMultiplierPerGarrisonedTrader'"> +"<ref name='positiveDecimal'/>" + | 17| 16| "</element>" + | 18| 17| "</optional>"; | 19| 18| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 3. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| 15| "<element name='GainMultiplierPerGarrisonedTrader'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 16|+ "<ref name='positiveDecimal'/>" + | 17| 17| "</element>" + | 18| 18| "</optional>"; | 19| 19| | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 95| 95| cmpTargetMarket.AddTrader(this.entity); | 96| 96| } | 97| 97| else if (this.markets.length == 1) | 98| |- { | | 98|+ | 99| 99| // If we have only one market and target is different from it, | 100| 100| // set the target as second one | 101| 101| if (target == this.markets[0]) | 107| 107| cmpTargetMarket.AddTrader(this.entity); | 108| 108| this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]); | 109| 109| } | 110| |- } | | 110|+ | 111| 111| else | 112| 112| { | 113| 113| // Else we don't have target markets at all, | | [NORMAL] ESLintBear (no-else-return): | | Unnecessary 'else' after 'return'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 100| 100| // set the target as second one | 101| 101| if (target == this.markets[0]) | 102| 102| return false; | 103| |- else | 104| |- { | | 103|+ | 105| 104| this.index = 0; | 106| 105| this.markets.push(target); | 107| 106| cmpTargetMarket.AddTrader(this.entity); | 108| 107| this.goods.amount = this.CalculateGain(this.markets[0], this.markets[1]); | 109| |- } | | 108|+ | 110| 109| } | 111| 110| else | 112| 111| { | | [NORMAL] ESLintBear (object-curly-spacing): | | A space is required before '}'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 276| 276| var max = 1; | 277| 277| if (cmpObstruction) | 278| 278| max += cmpObstruction.GetUnitRadius()*1.5; | 279| |- return { "min": 0, "max": max}; | | 279|+ return { "min": 0, "max": max }; | 280| 280| }; | 281| 281| | 282| 282| Trader.prototype.OnGarrisonedUnitsChanged = function() Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/171/display/redirect
I'm wondering though what would be the most logical place to define this bonus: the entity template that receives it or the entity template that gives it?
One could argue for both, the GarrisonHolder can give a bonus or the Garrisoned trade can give a bonus.
Is the bonus as high as think it is?
15 trade carts on a ship = 4 times as much profit as 15 trade carts trading themselves?
Seems kinda wrong, since the the individual trade carts are also not unlikely more exposed than one ship, but whatever.
There was a ticket for garrisoned traders not being profitable, #3428.
Decide for yourselves whether you want to make this a big rework with logic and feature changes or whether you just want this ugly hardcoding removed.
(I suppose the most fruitful reworks are those that end up in the game visible to players.)
binaries/data/mods/public/simulation/components/Trader.js | ||
---|---|---|
19 | missing feature description |
Yes, a merchant ship with 15 traders inside generates four times as much resources as an empty one. Or to put it differently, garrisoning traders in merchant ships is only profitable if an empty merchant ship generates more than five times as much as a trader walking its own route.
A much cheaper option is to simply construct additional merchant ships. (Four empty merchant ships cost 4×100=400 metal in total; one merchant ship with 15 traders inside costs 100+15×80=1300 metal and 15×100=1500 food in total.)
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/185/display/redirect
By the way, do fishing boats get a gather bonus from garrisoning a worker inside? I vaguely recall reading that somewhere (might have been a previous alpha, non-implemented feature, or mod), but if it's implemented, it should probably be moved to its xml template as well.
- cmpIdentity.HasClass("Ship") seems useless unless I miss something
- the template value is a string, so it could be 0, but in that case its truthy and the condition is met
- (BuffHeal is defined in the GarrisonHolder, should this value be defined there as well? Would seem a bit out of place to me rather, and I suppose buildings arent healers)
The fishing gather loadingscreen tip was removed, you'll find it in the revision log.
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/186/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (space-unary-ops): | | Unexpected space after unary operator '+'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 12| 12| "<ref name='positiveDecimal'/>" + | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| |- "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 15|+ "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> +"<ref name='positiveDecimal'/>" + | 17| 16| "</element>" + | 18| 17| "</optional>"; | 19| 18| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 3. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| 15| "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 16|+ "<ref name='positiveDecimal'/>" + | 17| 17| "</element>" + | 18| 18| "</optional>"; | 19| 19| Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1101/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/587/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (space-unary-ops): | | Unexpected space after unary operator '+'. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 12| 12| "<ref name='positiveDecimal'/>" + | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| |- "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 15|+ "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> +"<ref name='positiveDecimal'/>" + | 17| 16| "</element>" + | 18| 17| "</optional>"; | 19| 18| | | [NORMAL] ESLintBear (indent): | | Expected indentation of 1 tab but found 3. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 13| 13| "</element>" + | 14| 14| "<optional>" + | 15| 15| "<element name='GainMultiplierPerGarrisonedTrader' a:help='Additional gain for garrisonable unit for each garrisoned trader (1.0 means 100%)'"> + | 16| |- "<ref name='positiveDecimal'/>" + | | 16|+ "<ref name='positiveDecimal'/>" + | 17| 17| "</element>" + | 18| 18| "</optional>"; | 19| 19| Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1103/display/redirect
binaries/data/mods/public/simulation/templates/template_unit_ship_merchant.xml | ||
---|---|---|
50 | Very long node name (33 letters). |
binaries/data/mods/public/simulation/templates/template_unit_ship_merchant.xml | ||
---|---|---|
50 | Feel free to propose shorter name :) |
binaries/data/mods/public/simulation/components/Trader.js | ||
---|---|---|
16 | You should move the last ". | |
56 | One can write garrisonedTradersCount * +this.template.GainMultiplierPerGarrisonedTrader to avoid the extra braces. | |
binaries/data/mods/public/simulation/templates/template_unit_ship_merchant.xml | ||
50 | Would you deem it also out of scope to not hard-code the trader part? So not allowing more multipliers but just adding a GarrisonGainMultiplierClasses-node? (One could argue that garrisoning slaves would also increase the gain or something.) Then instead of let cmpGarrisonedUnitTrader = Engine.QueryInterface(entity, IID_Trader); if (cmpGarrisonedUnitTrader) one could do: MatchesClassesList(...)? |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/594/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... Executing section JS... | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/Trader.js | 140| 140| if (this.template.GarrisonGainMultiplier === undefined) | 141| 141| return undefined; | 142| 142| return ApplyValueModificationsToEntity("Trader/GarrisonGainMultiplier", +this.template.GarrisonGainMultiplier, this.entity); | 143| |-} | | 143|+}; | 144| 144| | 145| 145| Trader.prototype.HasBothMarkets = function() | 146| 146| { binaries/data/mods/public/simulation/components/Trader.js | 143| } | | [NORMAL] JSHintBear: | | Missing semicolon. Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1110/display/redirect