#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 )
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)
Change History (20)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Description: | modified (diff) |
---|---|
Summary: | 'Trade/International' seems to refers to nothing in simulation/helpers/TraderGain.js → Moving the international bonus constant from simulation/helpers/TraderGain.js to template |
comment:3 by , 8 years ago
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 |
comment:4 by , 8 years ago
Keywords: | review added |
---|
by , 8 years ago
Attachment: | tradeinternational.2.diff added |
---|
by , 8 years ago
Attachment: | tradeinternational.3.diff added |
---|
by , 8 years ago
Attachment: | tradeinternational.4.diff added |
---|
comment:5 by , 8 years ago
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.
by , 8 years ago
Attachment: | tradeinternational.5.diff added |
---|
comment:6 by , 8 years ago
Thx for the review, i hope i adressed all the remarks. I didn't yet merge the DISTANCE_FACTOR.
by , 8 years ago
Attachment: | tradeinternational.6.diff added |
---|
comment:7 by , 8 years ago
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 by , 8 years ago
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.
by , 8 years ago
Attachment: | tradeinternational.7.diff added |
---|
comment:10 by , 8 years ago
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
by , 8 years ago
Attachment: | tradeinternational.diff added |
---|
by , 8 years ago
Attachment: | tradeinternational.8.diff added |
---|
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).