Page MenuHomeWildfire Games

Move hardcoded gain per garrisoned trader on ship to template
Needs ReviewPublic

Authored by Angen on Jul 21 2019, 2:11 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

This change would allow to have merchant ships among civilisations or one civ itself which would give different bonus when garrisoned traders.
Also removing hardcoded number to template ( pointed out by @Nescio in D2100)

Test Plan

Check logic is the same.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8524
Build 13942: Vulcan BuildJenkins
Build 13941: arc lint + arc unit

Event Timeline

Angen created this revision.Jul 21 2019, 2:11 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/163/display/redirect

Angen updated this revision to Diff 9032.Jul 21 2019, 2:44 PM

tab to spaces and revert var -> let

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/164/display/redirect

Angen updated this revision to Diff 9033.Jul 21 2019, 2:46 PM

fix typo

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/165/display/redirect

Angen added a subscriber: Nescio.Jul 21 2019, 2:52 PM
Angen edited the summary of this revision. (Show Details)Jul 21 2019, 2:56 PM

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.

Angen added a comment.Jul 21 2019, 3:07 PM

It looks like gameplay change so I cannot decide

Angen added a comment.Jul 21 2019, 3:11 PM

As jenkins is kind of broken, tests passed:

Angen added a comment.Jul 21 2019, 3:24 PM

@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

elexis added a subscriber: elexis.Jul 21 2019, 8:11 PM

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
18

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.)

Angen updated this revision to Diff 9055.Jul 22 2019, 1:32 PM

lets keep it this way, I dont know if I want to start balancing changes

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.

elexis added a comment.EditedJul 22 2019, 1:43 PM
  • 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.

Angen updated this revision to Diff 9056.Jul 22 2019, 1:55 PM

use early returns

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/186/display/redirect