Page MenuHomeWildfire Games

Adding resource counts and population count graphs
ClosedPublic

Authored by toonijn on Jan 17 2021, 12:13 PM.

Details

Reviewers
Freagarach
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24721: Add resource and population counts to the summary screen.
Trac Tickets
#4554
Summary

This patch adds two new graphs

  • By resources, besides gathered and used, there is a new count graph. This displays the amounts of resources available during the game
  • In miscellaneous there is a graph of the population count

Note that this patch does not add extra columns to the summary.

A screenshot of the graphs (me, in blue, vs 3 AI's; I lost)

Test Plan

-

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

toonijn requested review of this revision.Jan 17 2021, 12:13 PM
toonijn created this revision.
Silier added a subscriber: Silier.Jan 17 2021, 1:34 PM

Hello,
thank you for picking this up.
I am writing down some minor things. I did not check this fully, nor read ticket yet.
We are currently in feature freeze so please do not be discouraged by us not commiting this once finished until we start new cycle :)

binaries/data/mods/public/gui/summary/counters.js
20
binaries/data/mods/public/simulation/components/StatisticsTracker.js
184

remove this function if unused

399
if (cmpPlayer)
     return cmpPlayer.GetResourceCounts();

and the rest

Silier added a reviewer: Restricted Owners Package.Jan 17 2021, 1:36 PM
toonijn updated this revision to Diff 15425.Jan 17 2021, 3:39 PM

fixing comments from angen

toonijn marked 3 inline comments as done.Jan 17 2021, 3:40 PM
Stan added a subscriber: Stan.Jan 17 2021, 6:06 PM

Can you upload a picture? You also need to add context to your patch. What did you use to generate it?

binaries/data/mods/public/simulation/components/StatisticsTracker.js
351

*amount. Final dot.

358

Might want to find a better name :)

toonijn updated this revision to Diff 15438.Jan 17 2021, 6:12 PM

Typo fix and better variablename

toonijn marked 2 inline comments as done.Jan 17 2021, 6:13 PM
toonijn edited the summary of this revision. (Show Details)Jan 17 2021, 6:16 PM
Stan edited the summary of this revision. (Show Details)Jan 17 2021, 6:16 PM
Stan set the repository for this revision to rP 0 A.D. Public Repository.
toonijn edited the summary of this revision. (Show Details)Jan 17 2021, 6:18 PM
Stan added inline comments.Jan 17 2021, 6:22 PM
binaries/data/mods/public/gui/summary/counters.js
327โ€“332

sorry linter is not running :)

binaries/data/mods/public/gui/summary/counters.js
330
binaries/data/mods/public/simulation/components/StatisticsTracker.js
397
406
466
Stan added a comment.Jan 17 2021, 6:59 PM

I guess it might run at the next update. (Added to contributors and set the repo so it should be happy)

I did quick test and noticed, that if resource is not used or gathered, total count is not recorded.

Looks good. ๐Ÿ‘
Added some quick comments.
Maybe think of a more specific name then just hide? (I mean it's only hidden in the normal summary and not in the graphs.)

binaries/data/mods/public/gui/summary/layout.js
262

Same for the other occurences.

binaries/data/mods/public/simulation/components/StatisticsTracker.js
403

Could use map

In D3395#150799, @Angen wrote:

I did quick test and noticed, that if resource is not used or gathered, total count is not recorded.

I think this is just a quirk in the visualization. A graph of a constant value is not displayed properly.

toonijn updated this revision to Diff 15447.Jan 17 2021, 11:26 PM

Fixing some linter details

toonijn marked 7 inline comments as done.Jan 17 2021, 11:32 PM

I changed hide to the more informative hideInSummary.

toonijn edited the summary of this revision. (Show Details)Jan 17 2021, 11:32 PM
toonijn edited the summary of this revision. (Show Details)

Build is green

builderr-debug-macos.txt
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/CoreVideo.tbd and library file /System/Library/Frameworks//CoreVideo.framework/CoreVideo are out of sync. Falling back to library file for linking.
ld: warning: text-based stu

See https://jenkins.wildfiregames.com/job/macos-differential/2933/display/redirect for more details.

Stan added inline comments.Jan 18 2021, 1:31 PM
binaries/data/mods/public/gui/summary/counters.js
330

Well linter disagrees :) (Follow the linter)

binaries/data/mods/public/simulation/components/StatisticsTracker.js
398โ€“403

Maybe, might be hard to read. Sucks a bit to have to create such an empty array each time that function is called, but I guess there is no way around it?

466โ€“473
toonijn updated this revision to Diff 15491.Jan 18 2021, 11:17 PM

Fix: comments from Stan

toonijn marked 2 inline comments as done.Jan 18 2021, 11:20 PM

@Stan Good thing about the ternary operator: the branch that will not be picked won't be evaluated. So the new object (with no resources) will only be build when this function is called without a player (which is rare).

> false ? console.log('true') : console.log('false')
false
undefined
> true ? console.log('true') : console.log('false')
true
undefined
toonijn marked an inline comment as done.Jan 18 2021, 11:20 PM

Build is green

builderr-debug-macos.txt
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/CoreVideo.tbd and library file /System/Library/Frameworks//CoreVideo.framework/CoreVideo are out of sync. Falling back to library file for linking.
ld: warning: text-based stu

See https://jenkins.wildfiregames.com/job/macos-differential/2954/display/redirect for more details.

Stan added a comment.Jan 19 2021, 7:27 AM

Yeah that's the point :) You got new linting errors :)

I really like this feature :)

There is one suggestion though: perhaps have a separate Population catagory, such that one can differentiate between Pop count and pop bonuses (e.g. from houses) (perhaps even pop max).
So:
Population:

  • Amount (better name welcome).
  • Capacity (can be introduced in a later patch).
  • Max (idem).
toonijn updated this revision to Diff 15542.Jan 19 2021, 8:37 PM

Build is green

builderr-debug-macos.txt
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreVideo.framework/CoreVideo.tbd and library file /System/Library/Frameworks//CoreVideo.framework/CoreVideo are out of sync. Falling back to library file for linking.
ld: warning: text-based stu

See https://jenkins.wildfiregames.com/job/macos-differential/2979/display/redirect for more details.

@Freagarach
See #5940.
If I find the time I can implement these as well. But first, I think suggestion are welcome.

  • Code is good (minor style issue(s) can be fixed at commit time).
  • Feature is very nice and wanted.
  • Works as advertised.
Freagarach accepted this revision.Jan 20 2021, 9:19 AM
This revision is now accepted and ready to land.Jan 20 2021, 9:19 AM
wraitii accepted this revision.Jan 20 2021, 10:04 AM
wraitii added a subscriber: wraitii.

Seems to work, nice feature -> I'd say go for A24 :)

Thanks for the patch @toonijn!

This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jan 20 2021, 10:24 AM