[PATCH] Rework of the diminishingReturns code

[mimo]

1) the current code is in the ResourceGatherer component, while it should be part of the ResourceSupply component. That is fixed in the attached patch. 2) the function itself used is very weird: using such an oscillating function has no sense, and the chosen function is not mod-friendly at all: for example, if we increase the maxGatherer of the field, the sum of the rates of the gatherers will start to decrease. So the MaxGatherer and DiminishingReturn value of the template are completely linked, with no easy way to guess the behaviour of the diminishingReturn when one of them is changed. A more predictable function should be used. I will have a look at it in a later patch.

[fatherbushido]

2) It's an euphemism :) 1) (is there issue with ressources used by different players and value mod ?)

[mimo]

Replying to fatherbushido:

1) (is there issue with ressources used by different players and value mod ?)

i don't understand the comment? nothing is changed with that patch, just moving code from ResourceGatherer to ResourceSupply as the diminishing returns is a property of the supply and needs only info from the supply (numGatherers), so it has no reason to be in ResourceGatherer. And i've tested with a replay that the hash is not changed by the patch.

EDIT: about 1) it's not in the code but ​http://trac.wildfiregames.com/wiki/TechModifications that one should write a comment.

hopefully, with 2) most (if not all) of that comment will become useless and be removed.

[fatherbushido]

Indeed it makes more sense to put that in the GetDiminishingReturns function. Considered it as reviewed (test, typos checked). (I don't see any issue wich can occur with that.) About 2) Sure the patch doesn't change anything of that. (I am not just not clear with myselef thinking at a tech wich can modify for example the diminishing return of a berry tree wich is shared by different players).

[mimo]

Thanks for the review;

Replying to fatherbushido:

(I am not just not clear with myselef thinking at a tech wich can modify for example the diminishing return of a berry tree wich is shared by different players).

Hard to answer as i don't know how you would want to do it. But i guess the best way to implement that would be to add a new element in the ResourceGatherer component (possibly one by type of resource, but currently only fields have a diminishing returns) which can be modified by Tech or Aura, and governs the way you want to modify the diminishing return, which then can be altered just after line 261.

[mimo]

In 18978:

move the diminishing returns computation from the ResourceGatherer to the ResourceSupply component, reviewed by fatherbushido, refs #4356

[mimo]

As said in this ticket, the current diminishing returns is not clear at all, it is hard to have a quantitative idea of it, and futhermore it has a very weird behaviour. So i propose to switch to something more intuitive, with a simple geometric progression for each additional gatherer. So if q represents the diminishing return (a number between 0 and 1), the first gatherer will have its full rate, the second will have rate*q, the next rate*pow(q,2), and the nth rate*pow(q,n-1), so that the total rate will be rate*(1-pow(q,n)/(1-q), and thus the diminishing return for each worker is simply (1-pow(q,n))/(1-q)/n Using q=0.95 gives values not too different from the current diminishing returns as can be seen in the figure rates1.png which shows the integrated rate (summed on all gatherers) as a function of the number of gatherers: red is the current one in svn (with its bad cosine behaviour) and blue the new proposed one (dotted black is without diminishing returns). Then to show how this would change with the value of q, rates2.png gives the same kind of curves for different q values.

[fatherbushido]

I m fully convinced by the formula.

[mimo]

patch for the diminishingReturns (in comment 6 and rate1.png, 0.95 should be in fact 0.90)

[fatherbushido]

I will 'review'(test) it tommorow if you need but I don't see anything wrong. Perhaps you can add (minExclusive and) maxExclusive in the schema ? (One could also perhaps say you can merge the two if L90 and L93 (but then you should write two times this.GetNumGatherers) (so...))

[mimo]

In 18985:

change the diminishingReturns (used for farms) to make it more intuitive and mod-friendly, reviewed by fatherbushido, fixes #4356