Details

Reviewers

bb
wraitii

Commits

rP19410: Show effects of game-start tech modifications in Structure Tree.

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

Lint Not Applicable

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

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.

Harbormaster completed remote builds in B567: Diff 576.Feb 22 2017, 6:59 PM

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

In D154#5956, @fatherbushido wrote:

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?

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

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.

Harbormaster completed remote builds in B1004: Diff 1090.Apr 3 2017, 10:09 PM

In D154#11386, @s0600204 wrote:
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

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.

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.

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.

Harbormaster completed remote builds in B1010: Diff 1097.Apr 4 2017, 9:36 PM

In D154#11386, @s0600204 wrote:

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.)

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

s0600204 mentioned this in D295: Relocate and Rearrange structree code.Apr 6 2017, 4:21 PM

s0600204 mentioned this in D297: Add an in-game Template Details Viewer.Apr 6 2017, 5:28 PM

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.

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.

Harbormaster completed remote builds in B1090: Diff 1208.Apr 11 2017, 3:39 AM

Looks good, thanks for making the changes :) .

Ok wrt to D92.

Closed by commit rP19410: Show effects of game-start tech modifications in Structure Tree. (authored by wraitii). · Explain WhyApr 11 2017, 4:36 PM

This revision was automatically updated to reflect the committed changes.

s0600204 mentioned this in D92: Show entity upgrades in the Structure Tree.Apr 11 2017, 10:58 PM

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1212

ps/trunk/binaries/data/mods/public/globalscripts/Technologies.js

ps/trunk/binaries/data/mods/public/globalscripts/Templates.js

ps/trunk/binaries/data/mods/public/gui/structree/helper.js

ps/trunk/binaries/data/mods/public/gui/structree/load.js

ps/trunk/binaries/data/mods/public/gui/structree/structree.js

ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js

Show effects of game-start tech modifications in Structure TreeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1212

ps/trunk/binaries/data/mods/public/globalscripts/Technologies.js

ps/trunk/binaries/data/mods/public/globalscripts/Templates.js

ps/trunk/binaries/data/mods/public/gui/structree/helper.js

ps/trunk/binaries/data/mods/public/gui/structree/load.js

ps/trunk/binaries/data/mods/public/gui/structree/structree.js

ps/trunk/binaries/data/mods/public/simulation/components/TechnologyManager.js

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

Revision Contents
Changeset List