Page MenuHomeWildfire Games

Show all summary values as a graph
ClosedPublic

Authored by Imarok on Feb 14 2017, 10:33 PM.

Details

Summary

Adds a new tab with two graphs which each show one summary value over time.
The value can be selected with three dropdowns.

Test Plan

Play a game, open the summary and the graphs.

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
Imarok added inline comments.Apr 17 2017, 11:46 PM
binaries/data/mods/public/simulation/components/StatisticsTracker.js
388 ↗(On Diff #1269)

just used that, as we use it somewhere else in the code too

Imarok updated this revision to Diff 1317.Apr 18 2017, 12:14 AM

Using count for the two charts

Imarok marked 2 inline comments as done.Apr 18 2017, 12:14 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/802/ for more details.

Thanks for the updates, much nicer code IMO :-)

binaries/data/mods/public/gui/summary/summary.js
123 ↗(On Diff #1317)

Sure you don't want to make it:

let chartsVisible = panel.name == "chartsPanelButton";
generalPanel.hidden =chartsVisible;
chartsPanel.hidden = !chartsVisible;

if (chartsVisible)
	for (let i=0; i<2; ++i)
		updateCategoryDropdown(i);
else
	updatePanelData(g_ScorePanelsData[panel.name.substr(0, panel.name.length - "PanelButton".length)]);

?

173 ↗(On Diff #1317)

Those hardcoded numbers are still meh. The numbers will be in trouble if I find time

183 ↗(On Diff #1317)

potentially early return

27 ↗(On Diff #1315)

The color closing tag can be added if and only if the color element is defined.
Those three elements might fit into a single line if and only if you assume they will never be extended.
This percent item still seems out of place to me, but will have to become more familiar with the code to see what we can do.
Seems a bit odd that " / " is a postfix, isn't it supposed to be a delimiter or something to that effect actually? Like "x / y" instead of "x / y /"?
-1 Tab for the closing braces
The color globals seem quite nice to me and the structure overall looks much better to me IMO

binaries/data/mods/public/gui/summary/summary.xml
194 ↗(On Diff #1317)

Those strings sound like transifex people would report them for ambiguity, so should add a translation comment or context

167 ↗(On Diff #1252)

Looks MUCH better!

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/803/ for more details.

Imarok updated this revision to Diff 1329.Apr 18 2017, 12:47 PM
Imarok marked 4 inline comments as done.

Apply remarks

Imarok added inline comments.Apr 18 2017, 12:48 PM
binaries/data/mods/public/gui/summary/summary.js
123 ↗(On Diff #1317)

Now I get your point :D

27 ↗(On Diff #1315)

The color closing tag can be added if and only if the color element is defined.

What if the postfix should be colored too for a new value some day?

Those three elements might fit into a single line if and only if you assume they will never be extended.

As thats not sure, I'll leave it as is

This percent item still seems out of place to me, but will have to become more familiar with the code to see what we can do.
Seems a bit odd that " / " is a postfix, isn't it supposed to be a delimiter or something to that effect actually? Like "x / y" instead of "x / y /"?

I think thats ok as it is, though it could be postfix isn't the best word for this property.

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/809/ for more details.

elexis added inline comments.Apr 18 2017, 6:51 PM
binaries/data/mods/public/gui/summary/counters.js
464 ↗(On Diff #1329)

calculatePercent?

binaries/data/mods/public/gui/summary/summary.js
12 ↗(On Diff #1329)

Move those color opening and closing tags to the place that uses them, so that we have actual separation of code and data plus duplication reduction

129 ↗(On Diff #1329)

looks unconventional. Would be the first time we do such a thing while all the other code uses for() or map()

27 ↗(On Diff #1315)

Then the slash should explicitly add color tags.

It's really bad to have the opening tag in one place of the code and the closing tag on the other side of the world.
Pairs, no matter if parenthesis, braces, XML tags should always be opened and closed in the same breath

Imarok marked an inline comment as done.Apr 18 2017, 11:40 PM
Imarok added inline comments.
binaries/data/mods/public/gui/summary/counters.js
464 ↗(On Diff #1329)

Nope, as the value already is given as percent.

binaries/data/mods/public/gui/summary/summary.js
12 ↗(On Diff #1329)

That would mean adding > 10 color tags...
So I don't think this is a good idea.

129 ↗(On Diff #1329)

Sure, but why not? ;D
(for() will be much more code and map() is not suited for this usecase)

27 ↗(On Diff #1315)

that's because the colors are not only used together with the postfix.

elexis requested changes to this revision.Apr 22 2017, 5:15 AM

No to the color tags. Add them if and only if a color is defined, close the tag in the same place where you open it, so confirmation that tags that are opened are closed becomes trivial.

binaries/data/mods/public/gui/session/session.js
1444 ↗(On Diff #1329)

Last time we changed something in this function, we broke the bot and the bots database!

See rP19329

So needs to update the ratingbot accordingly and we will have to rename some table or something in the sqlite.db

binaries/data/mods/public/gui/summary/counters.js
25 ↗(On Diff #1329)

What is this thing actually? A postfix? A delimiter?

60 ↗(On Diff #1329)

Since that function is used in only one function, perhaps we can make this ugliness a local variable of that function

15 ↗(On Diff #564)

thx

21 ↗(On Diff #1252)

This still looks broken. g_InfinitySymbol refered to, but g_InfiniteSymbol defined. Consult jshint.

This revision now requires changes to proceed.Apr 22 2017, 5:15 AM
Imarok updated this revision to Diff 1458.Apr 23 2017, 7:20 PM
Imarok edited edge metadata.
Imarok marked an inline comment as done.

Fix InfinitySymbol global.

Build is green

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

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

Imarok added inline comments.Apr 23 2017, 10:36 PM
binaries/data/mods/public/gui/session/session.js
1444 ↗(On Diff #1329)

Why should this break the ratingbot?
The data sent to the bot is the same as before.

binaries/data/mods/public/gui/summary/counters.js
60 ↗(On Diff #1329)

May be used in other functions in the future life of counters.js

21 ↗(On Diff #1252)

:O true...

Imarok updated this revision to Diff 1465.Apr 23 2017, 10:37 PM

Move the color tags

Imarok updated this revision to Diff 1466.Apr 23 2017, 10:38 PM

Fix typo

Build is green

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

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

Build is green

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

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

bb requested changes to this revision.May 5 2017, 4:03 PM
bb added inline comments.
binaries/data/mods/public/gui/session/session.js
1544 ↗(On Diff #1466)

In original code feminisation wasn't filled, but it was created, also feminisation doesn't seem to exist in the statisticTracker, so not sure i this is correct. Also in some code down we use feminization, so make it consistent

binaries/data/mods/public/gui/summary/counters.js
54 ↗(On Diff #1466)

Add whitespaces in the arrays.

binaries/data/mods/public/gui/summary/summary.js
129 ↗(On Diff #1466)

missing whitespace

144–147 ↗(On Diff #1466)

Use a loop?

binaries/data/mods/public/simulation/components/StatisticsTracker.js
216–218 ↗(On Diff #1466)

IIRC we don't have caps after the hyphen, so remove them (some lines below too). If we do use caps there, add them in other comments

binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
160 ↗(On Diff #1466)

Trailling comma, IIRC it was decided that the trailling comma's in the guiInterface shouldn't be removed, so they should be there in the tests too.
But then they should be added everywhere in the tests....

169 ↗(On Diff #1466)

.... Like a missing trailling comma here

255 ↗(On Diff #1466)

And here and more down

This revision now requires changes to proceed.May 5 2017, 4:03 PM
Imarok added inline comments.May 5 2017, 4:26 PM
binaries/data/mods/public/gui/session/session.js
1544 ↗(On Diff #1466)

I know, I think someone forgot to add that. Though, as changing the reportObject bugs the lobby bot I think its better to remove ' feminisation' .

Imarok marked 9 inline comments as done.May 5 2017, 4:48 PM
Imarok updated this revision to Diff 1663.May 5 2017, 4:48 PM
Imarok edited edge metadata.

Apply remarks and fix trainling commas in guiinterface_tests

Vulcan added a comment.May 5 2017, 5:36 PM

Build is green

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

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

bb added inline comments.May 5 2017, 6:14 PM
binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js
142 ↗(On Diff #1663)

?

560 ↗(On Diff #1663)

?

568 ↗(On Diff #1663)

?

581 ↗(On Diff #1663)

?

Imarok updated this revision to Diff 1666.May 5 2017, 6:54 PM
Imarok marked 4 inline comments as done.

Remove trailing commas and put typeColors into an Object

bb accepted this revision.May 5 2017, 7:15 PM

Ignoring all other non quoted object keys and arrow function candidates in the tests, as they are out of scope imo.

Vulcan added a comment.May 5 2017, 8:08 PM

Build is green

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

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

This revision was automatically updated to reflect the committed changes.
elexis added a comment.May 5 2017, 9:43 PM

Thanks for the great feature Imarok, Vladislav and bb!

can u adjust the thickness of the graph lines? blue and red almost invisible on gray background in this thickness. pls xD

can u adjust the thickness of the graph lines? blue and red almost invisible on gray background in this thickness. pls xD

This problem is known, and I thing @vladislavbelov wanted to fix this (or did already?)
(Also wrong diff, as the drawing code was separate from this one.)

Maybe we should also show somehow the civs (icons) there in graphs as im missing this when looking at this :) maybe small icon in front of names or behind