Page MenuHomeWildfire Games

Fixing the build time tooltip
ClosedPublic

Authored by temple on May 20 2017, 2:15 AM.

Details

Summary

Currently, the tooltip for build time construction is only valid for units that have build rate = 1. For example, a worker elephant (build rate = 2) starting a barracks will see the message "This foundation will be completed in 150 seconds", but he will actually finish it in half the time. Also, currently the formula uses an integer percentage for the building progress rather than the actual value, which makes the countdown jerky rather than smooth. For example, instead of going 10, 9, 8, it'll stay at 10 for two seconds before jumping to 8.

To correct this we need to keep track of the building rates of every builder: for example, women will have rate 1 while men with female inspiration with have rate 1.1, and I believe some heroes affect everyone's building rate. When we add a worker to the foundation we start them with rate = 1, and after their first second of work we update their rate to the actual value. At any time later if their rate changes (for example, if a woman walks past a man and adds +10% to his build rate), we update the stored value.

The total build rate is the sum of their rates, although with multiple builders there is an additional penalty. For example, ten women build at a rate of 10^0.7 = 5.01 instead of 10, half the speed. (That's why building with one unit is the most efficient.)

There are actually two tooltips, "time to completion" and "time speed-up" (which means how much quicker we'd finish the construction if we added another woman builder). I moved both calculations to Foundation.js and corrected the math by using the total build rate rather than number of builders (the two are equal only if every unit has rate = 1).

See also D572 and D573.

Test Plan

Test that the tooltip matches the actual build time.

It seemed that foundation.progress was only used here, so I simply changed it to be GetBuildProgress rather than GetBuildPercentage. It didn't make sense to me to multiply by 100 only to divide by 100 later. But if it is used somewhere else then obviously we will need to change it back and define a new variable.

This is my first time coding in javascript so it's possible I've made some basic errors.

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
fatherbushido added inline comments.May 26 2017, 9:36 AM
binaries/data/mods/public/simulation/components/tests/test_Foundation.js
133 ↗(On Diff #2213)

Don't forget spaces in object

temple updated this revision to Diff 2224.May 26 2017, 6:53 PM

Updated per fatherbushido's comments. Moved Math.ceil() to selection_details.js.

temple marked 8 inline comments as done.May 26 2017, 6:55 PM

You point out that the computation in the tooltip was wrong. But I have to think a bit more :)

It's not obvious, so I should explain it more.
If there was no build penalty then ten women would build at a combined rate of 10.
But there is a build penalty, and that makes them only build at a combined rate of 10^0.7 = 5.01.
Put another way, each unit builds at 10^0.7 / 10 = 0.501 times their normal rate.
So they'll take about twice as long to finish a building as they would without the penalty.

The question is what happens when you have units that build at a different rate? For example, infantry in a woman's aura build 10% faster, and elephants build twice as fast as women.
The actual calculation is done in Build(), called on each unit once every second:

var deltaHP = Math.max(work, Math.min(maxHealth, work * this.GetBuildRate() * this.buildMultiplier));
work = the unit's build rate (1 for women, 1.1 for men, 2 for elephant, etc.)
this.GetBuildRate() = the building's hp/time rate (for example, a civic center has 3000 hp and takes 500s to build, so 3000/500 = 6 hp/s)
this.buildMultiplier = the build penalty per unit (10^0.7 / 10 = 0.501 in our example)

Notice that this.GetBuildRate() and this.buildMultiplier are the same for every unit. For the civic center, each woman adds deltaHP = 1 * 6 * 10^0.7/10 = 3.01 hp a second, and together they add 10 * 6*10^0.7/10 = 30.1 hp a second.
If there were nine women and one man, the build multiplier would be the same, so the women would add the same hitpoints as in the previous case, but the man would instead add deltaHP = 1.1 * 6 * 10^0.7/10 = 3.31 hp a second. And combined they'd add (9*1 + 1.1) * 6*10^0.7/10 = 30.4 hp each second.
If you had an elephant, one woman, and eight men, all together they'd add (2 + 1 + 8*1.1) * 6*10^0.7/10 = 35.5 hp each second.
That's how the calculation is actually done. (I didn't change it, besides removing "floor" in that other patch.)

The "time remaining" tooltip currently uses:
let speed = Math.pow(entState.foundation.numBuilders, 0.7);
But in light of the above discussion about how building works, the correct formula should be speed = total build rate * build multiplier.
The two formulas are identical if all the builders have rate = 1. For example, with ten women, speed = 10 * 10^0.7/10 = 10^0.7 = 5.01, and the numbers match.
But with an elephant, one woman, and eight men, the correct number is speed = (2 + 1 + 8*1.1) * 10^0.7/10 = 5.91.

fatherbushido requested changes to this revision.May 28 2017, 3:10 PM

Well I am really annoyed by the GetTotalBuilderRate in the Foundation component.
As the way it works is mainly the converse. So all the relevant information are already *given* to that component and it sounds a bit redundant to *get* them again. So instead, as we have an array for the builders entity, I woud recommand to have a second one for the matching builders rate. (A map for both is perhaps better). Then that one will be update in the Build method (at the same place where you add a builder).
Then your GetTotalBuilderRate() method will just compute the rate from that array (or map).

This revision now requires changes to proceed.May 28 2017, 3:10 PM
temple updated this revision to Diff 2281.May 28 2017, 6:35 PM
temple edited edge metadata.

Changed the builder array to a map storing their build rates, as fatherbushido suggested. Also changed totalBuilderRate to a variable.

elexis added a subscriber: elexis.May 28 2017, 6:44 PM
elexis added inline comments.
binaries/data/mods/public/gui/session/selection_details.js
254 ↗(On Diff #2281)
Engine.GetGUIObjectByName("resourceCarryingIcon").tooltip = sprintf(
	translatePlural(
		"Number of builders.\nTasking another to this foundation would speed construction up by %(speedup)s second.",
		"Number of builders.\nTasking another to this foundation would speed construction up by %(speedup)s seconds.",
		Math.ceil(entState.foundation.timeSpeedup)),
	{
		"speedup": Math.ceil(entState.foundation.timeSpeedup)
	});

?

Search for other occurances of translatePlural that have \n in them that ended up in the po files to see that extractors.py gets it

fatherbushido added inline comments.May 28 2017, 7:18 PM
binaries/data/mods/public/simulation/components/Foundation.js
124 ↗(On Diff #2281)

You could get rid of calling again GetRate (and so the Modification function...)
For example, add here a second argument 'rate'.
I guess AddBuilder is only called in Build
and 'rate' is exactly 'work'.

fatherbushido added inline comments.May 28 2017, 7:28 PM
binaries/data/mods/public/simulation/components/Foundation.js
124 ↗(On Diff #2281)

Not exactly that as you explained to me in irc.
Adding the key here (builderEnt) and adding the value in Build

temple updated this revision to Diff 2286.May 28 2017, 8:19 PM

Used default builder rate = 1 (it's replaced by the correct value in Build()).

temple marked 2 inline comments as done.May 28 2017, 8:20 PM
temple updated this revision to Diff 2288.May 28 2017, 8:26 PM

Separated the speed-up tooltip into two strings.

temple updated this revision to Diff 2303.May 29 2017, 6:23 AM

Cleaned up the sprintf/translate formatting. Replaced buildTimePenalty with 0.7 since it never changes. I'll rewrite the summary.

temple edited the summary of this revision. (Show Details)May 29 2017, 6:47 AM
temple added inline comments.May 29 2017, 6:55 AM
binaries/data/mods/public/gui/session/selection_details.js
254 ↗(On Diff #2281)

I couldn't find any other cases of translatePlural using "\n", but I'm not sure where all I'd have to look. There are some with plain translate, but I don't know about the po files, etc. It's probably better to have someone who knows what they're doing look at it. Anyway, I've moved the "\n" outside the translate here.

temple edited the summary of this revision. (Show Details)May 29 2017, 7:03 AM

Well I read all, I added some comments, not all of them are to adressed.

binaries/data/mods/public/simulation/components/Foundation.js
17 ↗(On Diff #2303)

Previous comment wasn't so wrong, was it?
I am not convinced that we should use the word penalty here, the previous comment seemed more general.

20 ↗(On Diff #2303)

uhm ok for removing this.buildTimePenalty = 0.7;
(That could be discussed, I have pro and cons)

126 ↗(On Diff #2303)

Add a comment here explaining that we use 1.0 as a default (but that the actual value will be given when actually performing the build in Build).
As we discussed together in irc, it is also tempting to call it from unitAI with the right value as parameter (grabed in UnitAI) but it would also be an approximation... (I am lazy to write more :p)

127 ↗(On Diff #2303)

Here we have a choice to add a function to compute that each time
something like this.builders.Values().reduce((a, b) => a + b, 0) or to compute it step by step like you do (which result in less operations).
So ok.
++this.totalBuilderRate is ok too in that specific case even if it seems less clear.

156 ↗(On Diff #2303)

Previous comments seemed not so bad.

157 ↗(On Diff #2303)

It's ok as you use it in two places.
I don't know if the name shouldn't be more explicit.
(as with a verb)
It's good to have the formula only at one place now.

171 ↗(On Diff #2303)

speed could be misleading here (rate sounds perhaps better)

182 ↗(On Diff #2303)

We could perhaps avoid duplication but it's ok like that.

304 ↗(On Diff #2303)

ok

temple updated this revision to Diff 2319.May 30 2017, 9:11 PM

Combined the tooltips. Added back buildTimePenalty and made a few other changes per fatherbushido's suggestions.

temple updated this revision to Diff 2324.May 30 2017, 10:32 PM

Separated the GUI change again to D572.

fatherbushido added a comment.EditedMay 30 2017, 11:27 PM

Test with AI, with serialization test, with auras, with techs. All seems working well.
See remarks.
(also may you upload patch with 'context')

binaries/data/mods/public/simulation/components/Foundation.js
19 ↗(On Diff #2324)

you can remove the tab on that whiteline

172 ↗(On Diff #2324)

It's fine to not add a cost check to detect some unexpected bugs.

binaries/data/mods/public/simulation/components/GuiInterface.js
356 ↗(On Diff #2324)

Well I wonder if those two ones should be here.
Perhaps they should be moved to extendEntityState? (they you could get rid of those mirage things below).

Could you check all that?

binaries/data/mods/public/simulation/components/Mirage.js
83 ↗(On Diff #2324)

Perhaps we could use newlines, though it's already done like that.
I wonder if those one are really usefull. See my comments in GuiInterface.

temple updated this revision to Diff 2327.May 31 2017, 1:53 AM

Moved the two times to extendedEntityState, and removed the changes to Mirage.js.
(Also uploaded with context, sorry for not doing that previously.)

Don't forget to check the tooltips (with or without detailed option).

binaries/data/mods/public/simulation/components/GuiInterface.js
542 ↗(On Diff #2327)

I guess we need to udpate the GuiInterface test.

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
2 ↗(On Diff #2327)

It seems we can remove it since the last versions?

135 ↗(On Diff #2327)

And so that too

temple updated this revision to Diff 2329.May 31 2017, 5:13 PM

Fixed tests.

fatherbushido resigned from this revision.Jul 29 2017, 11:28 AM
bb requested changes to this revision.Aug 7 2017, 3:37 PM
bb added a subscriber: bb.

Add some tests for the CalculateBuildMultiplier() function

binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

Notice this behaviour looks odd, but actually is correct, since meanwhile the value can change due to a tech or cheat, so we need to update the value on every build() call. But wouldn't it be better to set the value already correctly here and don't wait for the build update (by querying the builder component and making a getter there)

169–186 ↗(On Diff #2329)

wondering whether one of these functions will be called without calling the other, if no there is a lot of duplicate execution, probably good to merge the two function and returning an object like { "timeRemaining": ... , "timeSpeedup": ... } but no strong opinion

172–173 ↗(On Diff #2329)

both variables only used once so can be inlined, strictly the cmpCost could also be inlined, but no strong opinion on that

301 ↗(On Diff #2329)

wondering if this check is good for performance reasons, otherwise the check is not necessary (then the code below doesn't do anything)

This revision now requires changes to proceed.Aug 7 2017, 3:37 PM
elexis added inline comments.Aug 7 2017, 4:52 PM
binaries/data/mods/public/gui/session/selection_details.js
265 ↗(On Diff #2329)

(Spreading translatePlural over multiple lines is odd, but we have it in menu.js and both string variants are found in l10n.js, so it's working. One could look at extractors.py for the best proof)

binaries/data/mods/public/simulation/components/Foundation.js
136 ↗(On Diff #2329)
temple updated this revision to Diff 3025.Aug 8 2017, 3:08 AM
temple edited edge metadata.

Updated per suggestions.

temple added inline comments.Aug 8 2017, 3:12 AM
binaries/data/mods/public/gui/session/selection_details.js
265 ↗(On Diff #2329)

I plan on changing some gui stuff with D572 after this is done, so for now I'll move the newline back inside the translate as it was.

binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

I had a builder rate getter in an earlier diff, but the previous reviewer didn't like it.

301 ↗(On Diff #2329)

I don't know much about performance, but I removed the if anyway.

fatherbushido added inline comments.Aug 8 2017, 8:08 AM
binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

Are you sure it was about liking?
As far as I remember, it was about working together on something.

Meh.

136 ↗(On Diff #2329)

(he doesn't mean you had to change it...)

bb requested changes to this revision.Aug 8 2017, 12:17 PM
bb added inline comments.
binaries/data/mods/public/gui/session/selection_details.js
260 ↗(On Diff #3025)

was wondering whether we should add "builder" between "another" and "to" in those strings, but better ask a native.

265 ↗(On Diff #2329)

ok (but comment was more about the arguments of translate to be spread over multiple lines of code, which is allowed, than using multiple translate calls)

binaries/data/mods/public/simulation/components/Builder.js
69 ↗(On Diff #3025)

use the new getter here

binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #3025)

I guess it is correct to assume such an entity has a builder component.

178 ↗(On Diff #3025)

semicolomn

136 ↗(On Diff #2329)

From grepping the public mod code: array.from() is used once in garrisonholder (in slightly another context), [...foo] is used in multiple files all over the mod. But meh

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
148–155 ↗(On Diff #3025)

with TS_ASSERT_UNEVAL_EQUALS one can test if two objects are the same

This revision now requires changes to proceed.Aug 8 2017, 12:17 PM
temple added inline comments.Aug 8 2017, 3:00 PM
binaries/data/mods/public/gui/session/selection_details.js
260 ↗(On Diff #3025)

Yes, I'll make that kind of change in D572.

binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

I remember you didn't want me calling the builder component at all in foundation, which I never really understood (but there's lots I don't understand). I thought bb should be aware of that, if he didn't read all the previous discussion (some was in irc and left out). Maybe he would have the same concerns or maybe not.

136 ↗(On Diff #2329)

(It's dangerous to comment on my code.)

temple updated this revision to Diff 3046.Aug 8 2017, 3:02 PM
temple edited edge metadata.
fatherbushido added inline comments.Aug 8 2017, 3:05 PM
binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

We mostly discussed of that in private. It was also late and I like to sleep early.

bb accepted this revision.Aug 9 2017, 12:29 PM
bb added inline comments.
binaries/data/mods/public/simulation/components/Foundation.js
174–177 ↗(On Diff #3046)

notice that when rate =0 (what can happen) these return infinity which is correct ofc. imo other code should handle that if it is not wanted to display that, as is done

binaries/data/mods/public/simulation/components/tests/test_Foundation.js
169 ↗(On Diff #3046)

some newlines could be added before each add/remove, but no strong opinion

This revision is now accepted and ready to land.Aug 9 2017, 12:29 PM
This revision was automatically updated to reflect the committed changes.
fatherbushido added inline comments.Aug 9 2017, 5:33 PM
binaries/data/mods/public/simulation/components/Foundation.js
127 ↗(On Diff #2329)

(but who cares?)

elexis added inline comments.Aug 9 2017, 6:06 PM
binaries/data/mods/public/simulation/components/Foundation.js
136 ↗(On Diff #2329)

(ES5 and ES6 offer features that have the potential to make the code nicer but were not present when the code was created originaly.
[...x]looks like it wouldn't do anything useful before being aware that x is a Map, while it's self-evident that x isn't an array when using Array.from.
Array.from also reveals it's intent while the reader has to resolve what the two operations yield to figure out what it does.
If we later decide to use Array.from, then it can be changed in all occurrences.
I'm perfectly fine with both proposals in this patch.)