Page MenuHomeWildfire Games

Move hardcoded gain per garrisoned trader on ship to template
ClosedPublic

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

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23238: Move hardcoded gain per garrisoned trader on ship to template
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)

Adding support for modifiers for that value.

Test Plan

Confirm ship with that element and garrisoned trader provides trading bonus for the player.

Event Timeline

Silier 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

Silier 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

Silier 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

Silier added a subscriber: Nescio.Jul 21 2019, 2:52 PM
Silier 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.

It looks like gameplay change so I cannot decide

As jenkins is kind of broken, tests passed:

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

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

Silier 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

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

Silier updated this revision to Diff 10339.Nov 16 2019, 6:20 PM

try to reupload

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

Nescio added inline comments.Nov 16 2019, 6:31 PM
binaries/data/mods/public/simulation/templates/template_unit_ship_merchant.xml
50

Very long node name (33 letters).
Also, how much work would it be to allow for custom classes? E.g. a DonkeyTrader adds 0.1, OxTrader 0.2, CamelTrader 0.3; somewhat similar to bonus attacks.
Just wondering; I'm not sure it's worth it, though.

Silier added inline comments.Nov 16 2019, 6:53 PM
binaries/data/mods/public/simulation/templates/template_unit_ship_merchant.xml
50

Feel free to propose shorter name :)
Short answer: Not much.
Longer:
That would make computation a bit slower, although I do not think noticeably slower.
Currently I do not see it as something we need to do. If there would be proposition with defined gains per trader type and it would be mostly balanced and have sense in general and agreed it is something game design needs or wants, than patch allowing that will be made.
Anyway I feel it to be out of scope of this patch.

Freagarach added inline comments.
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(...)?
(Name could be GarrisonGainMultiplier then?)

Silier planned changes to this revision.Nov 17 2019, 9:48 AM

fix schema

Silier updated this revision to Diff 10346.Nov 17 2019, 12:18 PM
Silier edited the summary of this revision. (Show Details)
Silier edited the test plan for this revision. (Show Details)

fix schema
shorter name
add support for modifiers
remove bracket

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

This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2019, 9:31 PM
This revision was automatically updated to reflect the committed changes.