rework a bit D964
ClosedPublic
Actions

Authored by mimo on Oct 21 2017, 12:23 PM.

Details

Reviewers

elexis

Commits

rP20323: rework a bit D964

Summary

Trying to implement D879 with the changes from D964 showed that this last patch was not fully adequate:

it is better to rename the new globalscript NormalizedTradeGain to a TradeGain function, and move the normalization on 100m to the TradeGainNormalization
if we want to add some saturation on the gain at high distances, the TradeGain function should also depend on mapsize

So here it is, hopefully the last design change before implementing D879

Test Plan

Check that the new structure is indeed better.
Nothing should be changed on the output.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mimo created this revision.Oct 21 2017, 12:23 PM

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2132/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 398| »   »   »   let·ratioMax·=·0.70·+·randFloat(0.,·0.1);
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '0.'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 403| »   »   »   »   ratioMax·=·0.85·+·randFloat(0.,·0.1);
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '0.'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1455| »   »   if·(highLevel·==·0·||·lowLevel·>·1)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|2385| »   »   if·(gameState.ai.playedTurn·%·4·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...

http://jenkins-master:8080/job/phabricator_lint/603/ for more details.

Harbormaster completed remote builds in B3422: Diff 3910.Oct 21 2017, 1:12 PM

Seems odd that it's comment says something about 100m when that number was moved to the other function.
Since TradeGain only returns the identity element, one can't predict well what the best structure for TradeGain and it's call would be until attempting to implement it.

Perhaps it might be easier to read if TradeGainNormalization would return x instead of 1/x, so that it would be clear on before matching the two lines that its distance(d) / distance(100m), like

let tradeGainDefaultDistance = TradeGainNormalDistance(mapSize);
gain.traderGain = TradeGain(distanceSq, mapSize) / tradeGainDefaultDistance;

But that again depends on wheather we want a a multiplier or a function.
Other than that, until the trade gain actually changed, the structure change doesn't provide anything to complain about I can find.

In D967#37674, @elexis wrote:

Seems odd that it's comment says something about 100m when that number was moved to the other function.

Sure, it should have been removed.

Since TradeGain only returns the identity element, one can't predict well what the best structure for TradeGain and it's call would be until attempting to implement it.

Typically, with implementing the last version of D879, it would be

let distance = Math.sqrt(distanceSq);
return mapSize * distance * Math.tanh(distance/mapSize);

Although i'm now leaning towards something simpler as distanceSq / (1. + 0.25 * Math.sqrt(distanceSq)) which has globally the same caracteristics as the previous one in the range of distances we are interested in, but is less scary to some people. But that discussion should be resumed in D879 when these preparatory changes are done.

Perhaps it might be easier to read if TradeGainNormalization would return x instead of 1/x, so that it would be clear on before matching the two lines that its distance(d) / distance(100m), like
let tradeGainDefaultDistance = TradeGainNormalDistance(mapSize);
gain.traderGain = TradeGain(distanceSq, mapSize) / tradeGainDefaultDistance;
But that again depends on wheather we want a a multiplier or a function.
Other than that, until the trade gain actually changed, the structure change doesn't provide anything to complain about I can find.

I've no strong opinion if the norm should be such that we multiply or divide by it. It's just a convention: if we want to scale by sqrt(mapsize) as proposed in D879, the function would become

return Math.sqrt(1024 / mapScale) / TradeGain(10000, mapSize);

and because of the first term, it makes sense that it is multiplicative, although i agree that the presence of the second term could be better if divided! So they could be separated in two different functions and have a TradeGainNormalDistance as you proposed, but as the ai may have to call it several times, it is better for performances to group all non variable part in a single function only called once.

fix comment

Build is green

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/2135/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 398| »   »   »   let·ratioMax·=·0.70·+·randFloat(0.,·0.1);
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '0.'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
| 403| »   »   »   »   ratioMax·=·0.85·+·randFloat(0.,·0.1);
|    | [NORMAL] JSHintBear:
|    | A trailing decimal point can be confused with a dot: '0.'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|1455| »   »   if·(highLevel·==·0·||·lowLevel·>·1)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/simulation/ai/petra/headquarters.js
|2385| »   »   if·(gameState.ai.playedTurn·%·4·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...

http://jenkins-master:8080/job/phabricator_lint/606/ for more details.

Harbormaster completed remote builds in B3425: Diff 3913.Oct 21 2017, 3:48 PM

elexis accepted this revision.Oct 21 2017, 4:27 PM

elexis added inline comments.