Page MenuHomeWildfire Games

Show effects of game-start tech modifications in Structure Tree
ClosedPublic

Authored by s0600204 on Feb 22 2017, 6:13 PM.

Details

Summary

Currently, the Structure Tree (structree) doesn't take into account any of the civbonus/penalties or the unit promotion techs when calculating the values it displays.

As the civbonuses/penalties and unit promotion techs are in-place from game-start and do not require a player to research them, the effects should be taken into account.

To minimise unnecessary repetition, code from the in-game TechnologyManager component has been relocated into globalscripts and adapted so that both the simulation and the structree can use it.

Originally reported on trac, #3747 and #3801.

Test Plan

Without the patch applied, open the Structure Tree and take note of the health points of the Civic Centre for the athen and brit civs. Also note the health and attack strength of the rome spearman ("Triarius") as trainable from the Army Camp.

With the patch applied, open the Structure Tree and take note that the health of the athen Civic Centre has increased and the health of the brit Civic Centre has gone down. These are part of the effects of their respective civbonus/penalties.

Also take note that the health, attack strength, and gather rates of the spearman in the Army Camp have changed. This is due to it being trained at Advanced rank. (If it helps, the spearman trainable from the Barracks is the same unit, but at Basic rank. You can use this to compare the two.)

As a further test, run a game session (or two). The modifiers should still be applied correctly in game, despite the relocated code.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
Owners added a subscriber: Restricted Owners Package.Feb 22 2017, 6:13 PM
Vulcan added a subscriber: Vulcan.Feb 22 2017, 6:59 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/387/ for more details.

There is also a related design question: should the tech tree display the tech modified value while in game?

There is also a related design question: should the tech tree display the tech modified value while in game?

If you mean, "should the structree detect the presence of a running game session and apply the effects of a player's currently-researched technologies on the values it displays", that is beyond the scope of this ticket and would require detailed discussion which would be better located on the forums.

Yes that's it.
The way of doing things depends of that too, isn't it?

bb requested changes to this revision.Mar 5 2017, 2:24 PM
bb added a subscriber: bb.

There are some rounding issues with mace walls/palisades.
Gather rates on skiritai incorrect.

binaries/data/mods/public/globalscripts/Technologies.js
47 ↗(On Diff #576)

.

50 ↗(On Diff #576)

Improper JsDocs, should be @param {Object} techTemplate The technology template to derive the modifications from. and @return {Object} containing the relevant modifications..

77 ↗(On Diff #576)

Doesn't need overline split but OK.

78 ↗(On Diff #576)

Newline

binaries/data/mods/public/globalscripts/Templates.js
88 ↗(On Diff #576)

This will crash when the template and tech paths does not correspond, but I guess thats ok.

91 ↗(On Diff #576)

Wondering what this call is (trying to) do...

358 ↗(On Diff #576)

Merge this with ret declaration above.

359 ↗(On Diff #576)

Trailling comma.

365 ↗(On Diff #576)

Shouldn't that be undefined (instead on null)?
Imo ternary was better.

376 ↗(On Diff #576)

Improper JsDocs (same as above).

binaries/data/mods/public/gui/structree/helper.js
54 ↗(On Diff #576)

Nuke path and make it global.

59 ↗(On Diff #576)

Some comment about the -5 wouldn't hurt.

71 ↗(On Diff #576)

Name confusing as we are only looping over autoresearched techs.

binaries/data/mods/public/gui/structree/structree.js
14 ↗(On Diff #576)

Perhaps move this to helper file where it is used.
I believe it is good to have this global, so we don't recalculate on civ selection.

31 ↗(On Diff #576)

I guess this can be merged with variable definition.

37 ↗(On Diff #576)

sry have to notice this ugliness, no change needed in this patch.

This revision now requires changes to proceed.Mar 5 2017, 2:24 PM
s0600204 updated this revision to Diff 1090.Apr 3 2017, 9:24 PM
s0600204 edited edge metadata.
s0600204 marked 12 inline comments as done.

Changes in response to @bb's comments on Phabricator

  • Improved JSDoc comment blocks
  • New const globals
  • Rearranging some minor code

In addition, some changes from D92 with regard to handling tech-modification paths have also been included. (affects globalscripts/Templates only)

In D154#7363, @bb wrote:

There are some rounding issues with mace walls/palisades.

I suspect the rounding issues have something to do with how spidermonkey handles numbers...

1 * 1.1 = 1.1
2 * 1.1 = 2.2
3 * 1.1 = 3.3000000000000003
4 * 1.1 = 4.4
5 * 1.1 = 5.5
6 * 1.1 = 6.6000000000000005
7 * 1.1 = 7.700000000000001
8 * 1.1 = 8.8
9 * 1.1 = 9.9
10 * 1.1 = 11
11 * 1.1 = 12.100000000000001
...
etc

Not sure how to fix or work round this (in an elegant/clean/efficient manner). Or even if, as it also occurs in the session gui and will no doubt need a more universal fix.

In D154#7363, @bb wrote:

Gather rates on skiritai incorrect.

In what way?

The filename of the Spartan Skiritai swordsman implies that it should be a Champion, but it inherits from a citizen-soldier-type template, so perhaps there's something else wrong here.

binaries/data/mods/public/globalscripts/Templates.js
88 ↗(On Diff #576)

It should not crash. It may give the wrong value under certain specific circumstances, but it should not crash.

If you can prove evidence or a test case where it does crash, I'll look into it.

91 ↗(On Diff #576)

The same thing that the call does in the pre-revision state of the code.

If called from within a running game session, it returns the appropriately modified value by running it through the simulation state, which is more complete and includes player-specific modifications unlike the call below which only applies global or game-start modifications.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/688/ for more details.

bb requested changes to this revision.Apr 4 2017, 2:41 PM
In D154#7363, @bb wrote:

There are some rounding issues with mace walls/palisades.

I suspect the rounding issues have something to do with how spidermonkey handles numbers...

1 * 1.1 = 1.1
2 * 1.1 = 2.2
3 * 1.1 = 3.3000000000000003
4 * 1.1 = 4.4
5 * 1.1 = 5.5
6 * 1.1 = 6.6000000000000005
7 * 1.1 = 7.700000000000001
8 * 1.1 = 8.8
9 * 1.1 = 9.9
10 * 1.1 = 11
11 * 1.1 = 12.100000000000001
...
etc

Not sure how to fix or work round this (in an elegant/clean/efficient manner). Or even if, as it also occurs in the session gui and will no doubt need a more universal fix.

This needs a round at the appropriate place, not sure if an universal fix is possible, since some values have are rounded on whole numbers, some have 1 more digit.

In D154#7363, @bb wrote:

Gather rates on skiritai incorrect.

In what way?
The filename of the Spartan Skiritai swordsman implies that it should be a Champion, but it inherits from a citizen-soldier-type template, so perhaps there's something else wrong here.

Skiri's are elite swordsman, so they have better stats in attacking (as is shown is structree correctly), but they have lesser gather stats due to promotion. The gather rates shown in structree are the same as those of normal swordsman, thus wrong.

binaries/data/mods/public/globalscripts/Templates.js
83–84 ↗(On Diff #1090)

periods

354 ↗(On Diff #1090)

Never seen that hyphen in comments, nuke

355 ↗(On Diff #1090)

requirements seem to also be based on civ

372 ↗(On Diff #1090)

See above

91 ↗(On Diff #576)

Then I think it's broken, tested with the corral ups for cav speed...

probably out of scope of this diff

binaries/data/mods/public/gui/structree/helper.js
8 ↗(On Diff #1090)

period

33 ↗(On Diff #1090)

variable unneeded.

47 ↗(On Diff #1090)

Also unneeded

59 ↗(On Diff #1090)

unneeded variable

64 ↗(On Diff #1090)

good

68 ↗(On Diff #1090)

As there only 1 statement follows in the loop, I would invert the cases and so remove the continue.

This revision now requires changes to proceed.Apr 4 2017, 2:41 PM
s0600204 planned changes to this revision.Apr 4 2017, 7:24 PM
s0600204 marked 8 inline comments as done.

Skiri's are elite swordsman, so they have better stats in attacking (as is shown is structree correctly), but they have lesser gather stats due to promotion. The gather rates shown in structree are the same as those of normal swordsman, thus wrong.

Ok, I see the problem. ResourceGatherer/BaseSpeed was not being taken into consideration. Problem also affected Rome Triarius.

binaries/data/mods/public/globalscripts/Templates.js
83–84 ↗(On Diff #1090)

...are what lady-folk get roughly once a month. I'll add in some full-stops instead.

*ahem* ok, levity over. Back to work.

354 ↗(On Diff #1090)

Then with due respect, you're not looking very hard.

Hint: run ack --type=js "\* \@param \{" in binaries/data/mods/public and you'll find that more frequently than not, a hyphen is included.

binaries/data/mods/public/gui/structree/helper.js
33 ↗(On Diff #1090)

But arguably nicer to read. Changing anyway,

59 ↗(On Diff #1090)

True, but would make the for statement more unpleasant to read, imo.

s0600204 updated this revision to Diff 1097.Apr 4 2017, 8:51 PM
s0600204 edited edge metadata.
s0600204 marked 3 inline comments as done.
s0600204 edited the test plan for this revision. (Show Details)

Further modifications as requested bu @bb

binaries/data/mods/public/gui/structree/helper.js
59 ↗(On Diff #1090)

Meh, it's not too bad. Changed it anyway.

Vulcan added a comment.Apr 4 2017, 9:36 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/694/ for more details.

In D154#7363, @bb wrote:

There are some rounding issues with mace walls/palisades.

I suspect the rounding issues have something to do with how spidermonkey handles numbers...

Well it has something to do with how IEEE 754 works. Or rather how some fractions cannot be represented accurately as sums of powers of 2.

One such example would be the 1.1 used below. which can be represented as 1*1+0*0.5+0*0.25+0*0.125+1*0.0625+.... but that will never be exactly 1.1 (but close enough for our purposes with the available precision). In a few of the below examples you just get lucky that the result is truncated in a nice place, or there is some rounding going on somewhere (I suspect the former though).

1 * 1.1 = 1.1
2 * 1.1 = 2.2
3 * 1.1 = 3.3000000000000003
4 * 1.1 = 4.4
5 * 1.1 = 5.5
6 * 1.1 = 6.6000000000000005
7 * 1.1 = 7.700000000000001
8 * 1.1 = 8.8
9 * 1.1 = 9.9
10 * 1.1 = 11
11 * 1.1 = 12.100000000000001
...
etc

Not sure how to fix or work round this (in an elegant/clean/efficient manner). Or even if, as it also occurs in the session gui and will no doubt need a more universal fix.

Round the numbers and only display some significant digits (if we even care about anything after the decimal seperator). Or punch whoever thought using tons of floating point numbers in the simulation was a good idea. (Maybe do both.)

bb accepted this revision.Apr 5 2017, 12:08 PM

Those whitespaces doesn't seem important enough for requesting changes...

binaries/data/mods/public/globalscripts/Templates.js
77 ↗(On Diff #1097)

One could add spaces in the commented object for consistency, meh.
Committer can add them too

94 ↗(On Diff #1097)

I don't suspect any stat needs to be displayed with more than 8 decimals, so seems fine

This revision is now accepted and ready to land.Apr 5 2017, 12:08 PM
wraitii requested changes to this revision.Apr 9 2017, 3:48 PM
wraitii added a subscriber: wraitii.

Should have seen this revision earlier...

This is overall a really good idea (would be useful for D302), it just needs a few small changes so that we don't revert code needlessly later. See inline comments.

Re:

There is also a related design question: should the tech tree display the tech modified value while in game?

What this code should be adapted to do is take an arbitrary list of researched technologies and return modified values, not only the auto-researched ones. See inline comments.

binaries/data/mods/public/globalscripts/Templates.js
85 ↗(On Diff #1097)

This function should be at the top scope instead. It will be useful for D302 and there's no reason not to put it there in general.

This function should not used g_CurrentModifiers but get a list of modifiers as a parameter, so that it becomes pure. See comments below about deriveAutoresearchedModifications.

Also, I'm not entirely sure why you add mod_key here, the description is unclear and it doesn't actually seem used?

96 ↗(On Diff #1097)

I like adding "else" for incompatible ifs anyhow.

binaries/data/mods/public/gui/structree/helper.js
72 ↗(On Diff #1097)

This should be put in Technologies.js.

Do not use g_AutoResearchTechList, pass a list of technologies (ideally template names, otherwise loaded templates) as a parameter instead.

Do not change g_CurrentModifiers, return the object instead.

(basically make this a pure function in Technologies.js)

This will make it possible to call getEntityValue (see comment above) with an arbitrary modifier object, which will be useful for D302 and the future in general.

This revision now requires changes to proceed.Apr 9 2017, 3:48 PM
s0600204 marked 2 inline comments as done.Apr 11 2017, 2:54 AM
s0600204 added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
85 ↗(On Diff #1097)

Also, I'm not entirely sure why you add mod_key here, the description is unclear and it doesn't actually seem used?

See D92#5200.

Unless you meant why added in this commit. Hypothetically to reduce possibility of merge conflict with D92, which also changes code in this area, and looked to be closer to being committed at the time.

s0600204 updated this revision to Diff 1208.Apr 11 2017, 2:54 AM
s0600204 edited edge metadata.

Move certain code into a higher scope for @wraitii

This revision is now accepted and ready to land.Apr 11 2017, 2:54 AM
s0600204 requested review of this revision.Apr 11 2017, 2:55 AM
s0600204 edited edge metadata.
This revision is now accepted and ready to land.Apr 11 2017, 2:55 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/744/ for more details.

wraitii accepted this revision.Apr 11 2017, 8:18 AM

Looks good, thanks for making the changes :) .

Ok wrt to D92.

This revision was automatically updated to reflect the committed changes.