Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#3917 closed defect (fixed)

[PATCH] Moving the international bonus constant from simulation/helpers/TraderGain.js to template

Reported by: fatherbushido Owned by: mimo
Priority: Should Have Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

In simulation/helpers/TraderGain.js, there is a global constant for the internation bonus trading. Then there value modification compute the tech/auramodification of Trade/International. So i suggest to put that constant in the Trader component. So it can be modified in templates.

Attachments (8)

tradeinternational.2.diff (9.5 KB) - added by fatherbushido 19 months ago.
tradeinternational.3.diff (9.5 KB) - added by fatherbushido 19 months ago.
tradeinternational.4.diff (9.7 KB) - added by fatherbushido 19 months ago.
tradeinternational.5.diff (12.0 KB) - added by fatherbushido 19 months ago.
tradeinternational.6.diff (15.6 KB) - added by fatherbushido 19 months ago.
tradeinternational.7.diff (15.1 KB) - added by fatherbushido 19 months ago.
tradeinternational.diff (16.5 KB) - added by fatherbushido 19 months ago.
tradeinternational.8.diff (16.5 KB) - added by fatherbushido 19 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 19 months ago by fatherbushido

As an example, i put an extra bonus for carth relative to something from the design docs (it can be perhaps implemented too as team bonus for all allies).

TB1 Name: Trademasters History: The Phoenicians and Carthaginians were broadly known as the greatest trading civilization of the ancient and classical world. Effect: +33% trade profit ‘international’ routes for allies.

comment:2 Changed 19 months ago by fatherbushido

Description: modified (diff)
Summary: 'Trade/International' seems to refers to nothing in simulation/helpers/TraderGain.jsMoving the international bonus constant from simulation/helpers/TraderGain.js to template

comment:3 Changed 19 months ago by fatherbushido

Keywords: review removed
Summary: Moving the international bonus constant from simulation/helpers/TraderGain.js to template[PATCH] Moving the international bonus constant from simulation/helpers/TraderGain.js to template

I will rework the patch and clean TraderGain?.js

Last edited 19 months ago by fatherbushido (previous) (diff)

comment:4 Changed 19 months ago by fatherbushido

Keywords: review added

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.2.diff added

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.3.diff added

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.4.diff added

comment:5 Changed 19 months ago by mimo

Thanks for working on this patch. Here are some comments:

  • InternationalBonus?: wouldn't it be more logical to make it a Market element (we now have a Market component) rather than a Trader element ?
  • be careful with the GetTradeRateMultiplier? ! lines 57 and 61 were an assignment in cmpPlayer to have the right TradeRateMultiplier? of the market owner, while in the patch, it is always the one from the trader (a player should not take advantage of the AI bonus when an AI is trading with him).

While working on it, we could also add (either in this ticket or a new one):

  • a tech for this internationalBonus (a placeholder would be to decrease a bit its initial value to 0.20, and having a cheap tech which would add 0.10, could be a "Commercial Treaty" or something like that)
  • we could also remove the DISTANCE_FACTOR const and merge it with the GainMultiplier? element inside a new GainPerUnitSquaredDistance? (or some better name as we may in the future go away from the gain proportionnal to the squared distance). That would be cleaner, and would avoid to have to redefine this constant in the AI as is presently done.

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.5.diff added

comment:6 Changed 19 months ago by fatherbushido

Thx for the review, i hope i adressed all the remarks. I didn't yet merge the DISTANCE_FACTOR.

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.6.diff added

comment:7 Changed 19 months ago by fatherbushido

V6 with the constant merge. I didn't yet change the name of GainMultiplier?. I didn't look at Petra. (Perhaps the Commercial Treaty should require the unlock_shared_los or unlock_shared_dropsite tech ?).

comment:8 Changed 19 months ago by mimo

thanks for the changes. Here are some comments still on v5 (i didn't noticed you had a new version). And don't worry for the petra changes, they can be done independantly (so after this one is commited).

in Trader.js, the InternationalBonus? in the schema example should be removed.

in TraderGain?.js lines 6-7 and 23-26 would be better imo as: let gainMultiplier; if (trader) {

let cmpTrader = Engine.QueryInterface?(trader, IID_Trader); if (!cmpTrader)

return null;

gainMultiplier = cmpTrader.GetTraderGainMultiplier?();

} else called from the gui, modifications already applied {

if (!traderTemplate)

return null;

gainMultiplier = traderTemplate.GainMultiplier?
1;

}

lines 27-30 could be moved just after 16, as well as 43-45

As the internationalBonus applies only when market1Owner != market2Owner, i think it is cleaner to have all the code related to it inside a block if (ownerMarket1 != ownerMarket2) {

...

} instead of all these ternaries

so lines 40-41 would go in this block and be simply let market1PlayerBonus = cmpMarket1.GetTradeRateMultiplier?(); let market2PlayerBonus = cmpMarket2.GetTradeRateMultiplier?(); also lines 47-48 would go in this block, as well as 50-51.

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.7.diff added

comment:9 Changed 19 months ago by fatherbushido

Thx for the remarks!

comment:10 Changed 19 months ago by mimo

The patch looks nearly ok now :) although I have not yet tested it. Just a few last comments

Trader.js

  • change the help in the Trader Schema for the GainMultiplier?, it should be now something like "trader gain for a 1m distance"
  • and I propose to change it to "trader gain for a 100m distance" so that the value will be more readable (0.756 instead of 0.0000756, and maybe round it to 0.75).

TraderGain?.js

  • tabs in lines 3 to 17
  • lines 21 to 37 should be the first when entering the function (the lines dealing with position being last). There is no need to compute the gainMultiplier if we have to return null
  • line 43 -> gain.traderGain = distanceSq * gainMultiplier / 10000; if switching to 0.75

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.diff added

Changed 19 months ago by fatherbushido

Attachment: tradeinternational.8.diff added

comment:11 Changed 19 months ago by mimo

Owner: set to mimo
Resolution: fixed
Status: newclosed

In 18108:

Move the const from simulation/helpers/TraderGain.js into the templates (InternationalBonus? and GainMultiplier?)
Add a tech to improve the internalBonus and give an improved bonus to cart
Patch from fatherbushido, fixes #3917

comment:12 Changed 19 months ago by mimo

Keywords: review removed

Thanks for the patch

Note: See TracTickets for help on using tickets.