Page MenuHomeWildfire Games

Add axes to CChart
ClosedPublic

Authored by vladislavbelov on May 11 2017, 12:33 AM.

Details

Reviewers
elexis
Imarok
s0600204
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP21429: Adds axes to the CChart and the summary screen.
Trac Tickets
#4892
#3403
Summary

Added axises and axises values for CChart.

Test Plan
  1. Run game
  2. Open replays, open summary
  3. Open chart tab

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 a comment.Jun 4 2017, 6:48 PM
In D474#20271, @Imarok wrote:

Bug

Actually it's not the bug, just format_y = "%0.f", so 1.00 and 1.05 are rounded to 1.

Huh? The same graph twice with different axis labels is a bug.

In D474#24789, @Imarok wrote:
In D474#20271, @Imarok wrote:

Bug

Actually it's not the bug, just format_y = "%0.f", so 1.00 and 1.05 are rounded to 1.

Huh? The same graph twice with different axis labels is a bug.

I meant, it's not the bug of the chart.

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!
Checking XML files...

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

In D474#24789, @Imarok wrote:
In D474#20271, @Imarok wrote:

Bug

Actually it's not the bug, just format_y = "%0.f", so 1.00 and 1.05 are rounded to 1.

Huh? The same graph twice with different axis labels is a bug.

I meant, it's not the bug of the chart.

It's a bug of the axes right?

In D474#26276, @Imarok wrote:

It's a bug of the axes right?

I don't think so, why? Let's imagine if someone set a text color alpha to 0.0, then no one won't see this text. But it's not a problem of a CText.

In D474#26276, @Imarok wrote:

It's a bug of the axes right?

I don't think so, why? Let's imagine if someone set a text color alpha to 0.0, then no one won't see this text. But it's not a problem of a CText.

Because the same graphs produce different axes labels. How should this be a correct behaviour?

vladislavbelov added a comment.EditedJun 17 2017, 12:49 AM
In D474#26296, @Imarok wrote:
In D474#26276, @Imarok wrote:

It's a bug of the axes right?

I don't think so, why? Let's imagine if someone set a text color alpha to 0.0, then no one won't see this text. But it's not a problem of a CText.

Because the same graphs produce different axes labels. How should this be a correct behaviour?

About same graphs: it's fixed in this patch already, and it's not the problem with axes.

I meant, that the bug with two 1 on the left chart isn't the chart issue.

fabio added a subscriber: fabio.Jun 17 2017, 8:34 AM

What about using a hours:minutes:seconds time format for the x axis? I think it would be cleaner rather than using only seconds with high values.
Also, scores on y axis should just be integers, no need for decimal values?

In D474#26298, @fabio wrote:

What about using a hours:minutes:seconds time format for the x axis? I think it would be cleaner rather than using only seconds with high values.
Also, scores on y axis should just be integers, no need for decimal values?

I agree, about format, it'd be nice, but how to implement it without hacks? In case we do the chart element independent of the summary, we could show any values there, so X-axis could be meters, seconds, bytes, etc.

About decimal, there is an image, which describes why we needs decimal, it has multiple equal axis numbers.

Else looks good functionality-wise

binaries/data/mods/public/gui/summary/summary.xml
198 ↗(On Diff #2427)

why not `format_x="%.0fs"?

vladislavbelov added inline comments.Jul 21 2017, 7:06 PM
binaries/data/mods/public/gui/summary/summary.xml
198 ↗(On Diff #2427)

Because @elexis suggested to use %.02f to prevent equal values on axes.

leper added inline comments.
source/gui/CChart.cpp
241 ↗(On Diff #1833)

But why do we use format strings?

What if someone does strange things (there is no limit on format anywhere) with the passed format string (which does come from JS/XML)? Eg using %n, not that having access to JS and XML wouldn't already be an issue, but why add code that just begs to be abused?

elexis added inline comments.Jul 21 2017, 8:37 PM
source/gui/CChart.cpp
241 ↗(On Diff #1833)

That could become a buffer overflow vulnerability if we have input from untrusted sources, i.e. if people could collaboratively style the format of charts and have format being sent across the network.

If we don't trust gui/summary/, but source/, then we should exclusively add support for few well defined formats, like printing a number with a user defined amount of decimals, percent numbers (to get the % character) and maybe ingame-time for events?

leper added inline comments.Jul 22 2017, 10:17 PM
source/gui/CChart.cpp
241 ↗(On Diff #1833)

It currently is one, however it also is a format string issue (which is why I mentioned %n, there's quite some literature about that in case you are bored).

I wouldn't say we don't trust the gui, but I'd rather not add code that relies on us always trusting it when there are alternatives where we don't have to. Always assume that you will get input you don't expect, so handle things to cope with those (even if the result is something that isn't useful, at least it shouldn't be harmful).

elexis added inline comments.Aug 12 2017, 11:57 AM
source/gui/CChart.cpp
226 ↗(On Diff #2427)
// unneeded comment
// TODO Vladslav: Don't duplicate TODOs from 2004 needlessly
241 ↗(On Diff #1833)

Just provide a helper function for each hardcoded format string. We really need axis labels.

source/gui/CChart.cpp
226 ↗(On Diff #2427)

Why do you think it's unneeded?

elexis added inline comments.Aug 12 2017, 12:18 PM
source/gui/CChart.cpp
226 ↗(On Diff #2427)

Why?

IMO The first line is unneeded because the code is equally easy to read.

IMO The second line is needless because we have that comment at least once already. Someone caring about this has to search the code for all occurrences of the bugged code (not for all occurrences of the comment).

vladislavbelov added inline comments.Aug 12 2017, 2:43 PM
source/gui/CChart.cpp
226 ↗(On Diff #2427)

Exactly by this reason I leave it as is, because it's easier to find it here. It will look strange if we would have this comment in many places, but in the one - not.

s0600204 requested changes to this revision.Aug 28 2017, 8:52 PM
s0600204 added a subscriber: s0600204.

I'm agreeing with leper and elexis on the topic of format strings. Why do we have something that can be misused or mistyped very easily, and not something like a choice between predefined formats: "time", "integer", "percentage", etc.?

I mean, if you can write a format string (%0.2f or whatever) without having to lookup a guide or reference to such things first, then good for you! But consider that not everyone can. I certainly can't. And to require our modders, who operate predominately in the world of XML & JS, to lookup a guide on string formatting for c++ if they wish to change how numbers appear... No. This is not something they should have to do.

And the fact it's not validated or checked for correctness before being used...

source/gui/CChart.cpp
108 ↗(On Diff #2427)

Why do the axes form a box, when just two sides are sufficient?

source/gui/CChart.h
79 ↗(On Diff #2427)

This function is only used to draw the axes lines, right? So why is it not part of the DrawAxes function?

My point is that it is unclear from the function name what this is used for, and there's no accompanying comment to explain why a line-graph-chart would want or need to draw a strip of triangles. And as this private function is only called in one place in this class, then why not merge it? Neither function is particularly long.

82 ↗(On Diff #2427)

AddFormattedValue

This revision now requires changes to proceed.Aug 28 2017, 8:52 PM
vladislavbelov requested review of this revision.Aug 29 2017, 12:02 AM

I'm agreeing with leper and elexis on the topic of format strings. Why do we have something that can be misused or mistyped very easily, and not something like a choice between predefined formats: "time", "integer", "percentage", etc.?

How do you want to set precision, add new types, add dynamic formats?

I mean, if you can write a format string (%0.2f or whatever) without having to lookup a guide or reference to such things first, then good for you! But consider that not everyone can. I certainly can't. And to require our modders, who operate predominately in the world of XML & JS, to lookup a guide on string formatting for c++ if they wish to change how numbers appear... No. This is not something they should have to do.

We already use the same standard for JS. So nothing is new for modders. But you suggest to limit modders who knows about this format. They won't be able to change something. To make the "time", "integer", "percentage", ... list moddable, you need to create additional files and modder should know how to edit it and know the format (still should know).

So why we need so much pain. If we could add a format, add styles like ModernPercentageChart, ModernIntegerChart. It's the easy & familiar way for all modders. It's really flexible.

source/gui/CChart.cpp
108 ↗(On Diff #2427)

Two sides looks ugly. Mostly RPG/RTS uses boxes. Because UI has colored/imaged background.

source/gui/CChart.h
79 ↗(On Diff #2427)

Because, as @elexis many time suggested to split it for the easy stack trace parsing.

I agree, that comment would be useful.

82 ↗(On Diff #2427)

True.

I'm agreeing with leper and elexis on the topic of format strings. Why do we have something that can be misused or mistyped very easily, and not something like a choice between predefined formats: "time", "integer", "percentage", etc.?

How do you want to set precision, add new types, add dynamic formats?

setup.xml
<?xml version="1.0" encoding="utf-8"?>
<setup>
    <NumberFormat name="percentage"
        maxDecimalPlaces="2"
        suffix="%"
    />
    <NumberFormat name="integer"
        maxDecimalPlaces="0"
    />
    <NumberFormat name="fictionalCurrency"
        maxDecimalPlaces="2"
        prefix="₡"
    />
    <NumberFormat name="increasedPrecisionDecimal"
        maxDecimalPlaces="8"
        minDecimalPlaces="4"
    />
</setup>

(And "time" would probably be a format defined on the c++ side.) I'm not asking you to implement this exact thing, it's just an idea of an alternate possibility. Easier for non-programmers to comprehend, and can potentially be used elsewhere in the gui.

I would also think that different datasets would use different number formats for the y axis. For instance, map control/exploration and vegetarianism would use percentage, treasures collected and buildings captured would be integers. It's not a one-fits-all thing. But the chart would have a default y-axis format for when one isn't provided with a dataset.

Just a thought.

Of course, we could leave modder-friendly formats for a later patch, in which case at the very least write some validation checks on the format string, and address the other remarks (by leper and elexis, not just mine).

source/gui/CChart.cpp
108 ↗(On Diff #2427)

Four sides looks ugly, and it's not clear they are axes and not just sides of a box.

Most graphs and charts use two axes (or three if there are two sets of overlaid data with different y scales). If gui designers want a box around the chart then they can add one via the use of <object type="image" sprite="BoxSpriteOfTheirChoiceAndOrDesign"/>. (And it should remain an option to have one or not.)

And what does the gui background have to do with the number of chart axes?

setup.xml
<?xml version="1.0" encoding="utf-8"?>
<setup>
    <NumberFormat name="percentage"
        maxDecimalPlaces="2"
        suffix="%"
    />
    <NumberFormat name="integer"
        maxDecimalPlaces="0"
    />
    <NumberFormat name="fictionalCurrency"
        maxDecimalPlaces="2"
        prefix="₡"
    />
    <NumberFormat name="increasedPrecisionDecimal"
        maxDecimalPlaces="8"
        minDecimalPlaces="4"
    />
</setup>

We'll have a lot of hardcoded and not flexible things then. Because now it's the XML attributes. What's the different from styles?

(And "time" would probably be a format defined on the c++ side.) I'm not asking you to implement this exact thing, it's just an idea of an alternate possibility. Easier for non-programmers to comprehend, and can potentially be used elsewhere in the gui.

I agree, it's not the best solution. But I don't want to introduce hardcoded and not flexible parts. I prefer to use simple way (formats), because it's short and flexible, it's easier to change/improve in the future, when someone will create an elegant way to do such things.

I would also think that different datasets would use different number formats for the y axis. For instance, map control/exploration and vegetarianism would use percentage, treasures collected and buildings captured would be integers. It's not a one-fits-all thing. But the chart would have a default y-axis format for when one isn't provided with a dataset.

I thought, that the best way would be a JS callback, but it's not supported.

source/gui/CChart.cpp
108 ↗(On Diff #2427)

Sides != Axes. There are 2 axes, but 4 sides.

First results from Google:

AoE II:

Starcraft:

MATLAB:

All of them use 4 sides, but 2 axes. Charts should be visible on any background. So we need 4 sides.

s0600204 added inline comments.Aug 29 2017, 2:50 AM
source/gui/CChart.cpp
108 ↗(On Diff #2427)

And the point of this revision is to add axes to charts. Not sides. Of which there are, as you pointed out, two.

And just to prove that I can use images too, a nice labeled image of x- and y-axes, courtesy of wikipedia:

And the number of sides (or axes) does not make a chart any more or less visible.

vladislavbelov added inline comments.Aug 29 2017, 9:46 AM
source/gui/CChart.cpp
108 ↗(On Diff #2427)

To add visible axes I add sides. Sides make charts much visible, because you see borders, you could see (not here, but at all) values on both sides if needed. Also all elements in our GUI are symmetric, so I don't want to break it.

From wiki too (Chart):

Does not need to be done in this patch, but what about adding some indicators on both axes showing the current coordinates of the mouse(if the mouse is within the chart)?

Or maybe also add some crosshairs like this:

In D474#33689, @Imarok wrote:

Does not need to be done in this patch, but what about adding some indicators on both axes showing the current coordinates of the mouse(if the mouse is within the chart)?
Or maybe also add some crosshairs ...:

Yeah, I thought about it. It's possible and I can add it, if it's really needed.

ffffffff added a subscriber: ffffffff.EditedSep 14 2017, 12:54 AM

i think having the y-axis values on right is more convenient or having on both sides

also crosshair very nice (pls make it white thin)

Is this just hold back by the axes/sides discussion?
(Then we maybe can just settle down to call the thing sides?)

elexis updated the Trac tickets for this revision.Dec 6 2017, 7:46 PM

@vladislavbelov pleeeeease get this fix into a23 :-)

ffffffff added a comment.EditedJan 31 2018, 4:59 PM

units missing points/seconds/minutes/hours at the axes for orientation

Uses fixed format types to prevent security issues with printf-like functions.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/differential/65/display/redirect

Tested the patch and I like what I see!

Decimal places:
About the axis label format, the score is typically in the tens of thousands, so we don't need the 2 digits after the comma.
It might be easier to read if we had space all 3 digits or print it like 40k. But that can be improved later.
For resources we even have millions, so it can be hard to read if those numbers are hundreds of thousands or millions.
Buildings and unit numbers are also integers, so there should be no numbers after the comma either.
Kill-death-ratio is a good use case for decimal places.

Units: I wonder if the labels should have a unit attached to it, like an m for minutes.
Of course that is now annoying to when we can't pass a printf format due to the buffer overflow possibility and
just concatenating that unit to the number likely won't work for some language, so it's meh/tricky.

binaries/data/mods/mod/gui/gui.rnc
60–61 ↗(On Diff #5872)

Wouldn't be surprised if it's not supported, but it would be good to verify the formats.

source/gui/CChart.cpp
35 ↗(On Diff #5872)

axis_font, axis_format_x/y? I suspect we might want to add other labels later.

175 ↗(On Diff #5872)

range based loop?

310 ↗(On Diff #5872)

++?

108 ↗(On Diff #2427)

Gentlemen, it should be the XML file deciding how things should be styled, so just add an option to allow both?
When it comes to the style, I would agree that a rectangle looks probably better.
Ideally this could be done with a sprite.

source/gui/CChart.h
79 ↗(On Diff #2427)

I don't recall saying anything about this function in particular, but it's not bad to have it split IMO.

s0600204 requested changes to this revision.Feb 27 2018, 7:25 PM

Using fixed format types is an improvement over the old way. Whilst having them hardcoded in cpp is not ideal, dehardcoding them is something that can be left for another day in another revision. (As is potentially localising number formatting (which is something we don't do anywhere currently).)

I am still concerned that you're still not drawing axis, but sides. As you put it so eloquently, sides != axis (which was and still is my point). The aim of this revision is to draw axis (it's even stated in the revision title). So please do so. Once again: if we really want a box drawn around the charts, then as both myself and @elexis have pointed out, there is already a preferred way of doing that.

And finally: how easy would it be to add captions to the axis? For instance, for the x-axis a caption that says "Time Elapsed"; and for the y-axis, a caption that changes depending on what's selected ("Percentage of Map Controlled" or "Quantity of Food Gathered" or whatever). This would be something to pass with the datasets. (The x-axis caption is achievable via XML, but the y-axis caption would preferably be rotated counter-clockwise 90 degrees, which isn't.) That said, I'm happy to leave these captions until A24.

binaries/data/mods/mod/gui/gui.rnc
242 ↗(On Diff #5872)

Watch your indenting please.

source/gui/CChart.cpp
108 ↗(On Diff #2427)
@elexis said,

it should be the XML file deciding how things should be styled
Ideally this could be done with a sprite.

Exactly. As I wrote months ago:

@s0600204 said,

If gui designers want a box around the chart then they can add one via the use of [xml snippet]. (And it should remain an option to have one or not.)

This revision now requires changes to proceed.Feb 27 2018, 7:25 PM

The important part is the labeling. A graph loses half the information if there aren't labels, which is why it was a must-really-have feature for a22, then it wasn't finished, so I was happy to see this patch being updated half a year later after crying a bit more :P
I'd even be ok with neither drawing axes nor sides if that solves the problem of adding code you don't prefer.
The hardcoding of the format is not too handy indeed, but the problem is that allowing someone to pass the sprintf format allows buffer overflows.

Adds @elexis and @s0600204 notes.

source/gui/CChart.cpp
35 ↗(On Diff #5872)

Do we really want to have other format_*?

175 ↗(On Diff #5872)

You already suggested it and declined it by yourself :)

We can't use range based loop here.

310 ↗(On Diff #5872)

It's not really good looking for decimal. I think, += 1.f is better.

source/gui/CChart.h
79 ↗(On Diff #2427)

There was something in the CinemaManager.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 511| 511| 	Engine.GetGUIObjectByName("summaryText").caption =
| 512| 512| 		g_GameData.gui.isInGame ?
| 513| 513| 			translate("Current Scores") :
| 514|    |-		g_GameData.gui.isReplay ?
|    | 514|+			g_GameData.gui.isReplay ?
| 515| 515| 			translate("Scores at the end of the game.") :
| 516| 516| 		g_GameData.gui.disconnected ?
| 517| 517| 			translate("You have been disconnected.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 512| 512| 		g_GameData.gui.isInGame ?
| 513| 513| 			translate("Current Scores") :
| 514| 514| 		g_GameData.gui.isReplay ?
| 515|    |-			translate("Scores at the end of the game.") :
|    | 515|+				translate("Scores at the end of the game.") :
| 516| 516| 		g_GameData.gui.disconnected ?
| 517| 517| 			translate("You have been disconnected.") :
| 518| 518| 		!assignedState ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 513| 513| 			translate("Current Scores") :
| 514| 514| 		g_GameData.gui.isReplay ?
| 515| 515| 			translate("Scores at the end of the game.") :
| 516|    |-		g_GameData.gui.disconnected ?
|    | 516|+				g_GameData.gui.disconnected ?
| 517| 517| 			translate("You have been disconnected.") :
| 518| 518| 		!assignedState ?
| 519| 519| 			translate("You have left the game.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 514| 514| 		g_GameData.gui.isReplay ?
| 515| 515| 			translate("Scores at the end of the game.") :
| 516| 516| 		g_GameData.gui.disconnected ?
| 517|    |-			translate("You have been disconnected.") :
|    | 517|+					translate("You have been disconnected.") :
| 518| 518| 		!assignedState ?
| 519| 519| 			translate("You have left the game.") :
| 520| 520| 		assignedState.state == "won" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 515| 515| 			translate("Scores at the end of the game.") :
| 516| 516| 		g_GameData.gui.disconnected ?
| 517| 517| 			translate("You have been disconnected.") :
| 518|    |-		!assignedState ?
|    | 518|+					!assignedState ?
| 519| 519| 			translate("You have left the game.") :
| 520| 520| 		assignedState.state == "won" ?
| 521| 521| 			translate("You have won the battle!") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 516| 516| 		g_GameData.gui.disconnected ?
| 517| 517| 			translate("You have been disconnected.") :
| 518| 518| 		!assignedState ?
| 519|    |-			translate("You have left the game.") :
|    | 519|+						translate("You have left the game.") :
| 520| 520| 		assignedState.state == "won" ?
| 521| 521| 			translate("You have won the battle!") :
| 522| 522| 		assignedState.state == "defeated" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 517| 517| 			translate("You have been disconnected.") :
| 518| 518| 		!assignedState ?
| 519| 519| 			translate("You have left the game.") :
| 520|    |-		assignedState.state == "won" ?
|    | 520|+						assignedState.state == "won" ?
| 521| 521| 			translate("You have won the battle!") :
| 522| 522| 		assignedState.state == "defeated" ?
| 523| 523| 			translate("You have been defeated…") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 518| 518| 		!assignedState ?
| 519| 519| 			translate("You have left the game.") :
| 520| 520| 		assignedState.state == "won" ?
| 521|    |-			translate("You have won the battle!") :
|    | 521|+							translate("You have won the battle!") :
| 522| 522| 		assignedState.state == "defeated" ?
| 523| 523| 			translate("You have been defeated…") :
| 524| 524| 			translate("You have abandoned the game.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 519| 519| 			translate("You have left the game.") :
| 520| 520| 		assignedState.state == "won" ?
| 521| 521| 			translate("You have won the battle!") :
| 522|    |-		assignedState.state == "defeated" ?
|    | 522|+							assignedState.state == "defeated" ?
| 523| 523| 			translate("You have been defeated…") :
| 524| 524| 			translate("You have abandoned the game.");
| 525| 525| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 520| 520| 		assignedState.state == "won" ?
| 521| 521| 			translate("You have won the battle!") :
| 522| 522| 		assignedState.state == "defeated" ?
| 523|    |-			translate("You have been defeated…") :
|    | 523|+								translate("You have been defeated…") :
| 524| 524| 			translate("You have abandoned the game.");
| 525| 525| 
| 526| 526| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 521| 521| 			translate("You have won the battle!") :
| 522| 522| 		assignedState.state == "defeated" ?
| 523| 523| 			translate("You have been defeated…") :
| 524|    |-			translate("You have abandoned the game.");
|    | 524|+								translate("You have abandoned the game.");
| 525| 525| 
| 526| 526| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 527| 527| 		translate("Game time elapsed: %(time)s"), {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 526| 526| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 527| 527| 		translate("Game time elapsed: %(time)s"), {
| 528| 528| 			"time": timeToString(g_GameData.sim.timeElapsed)
| 529|    |-	});
|    | 529|+		});
| 530| 530| 
| 531| 531| 	let mapType = g_Settings.MapTypes.find(type => type.Name == g_GameData.sim.mapSettings.mapType);
| 532| 532| 	let mapSize = g_Settings.MapSizes.find(size => size.Tiles == g_GameData.sim.mapSettings.Size || 0);

Link to build: https://jenkins.wildfiregames.com/job/differential/134/display/redirect

s0600204 accepted this revision.Mar 3 2018, 4:37 AM

Much better, thank you.

Feature wise, I think this is pretty much done.

Code-style wise (and I'm no expert when it comes to c++, so I'm going by the coding conventions) it looks ok.


Just so we have it attached to this revision, link to the screenshot you posted on #0ad-dev: https://i.imgur.com/9bkTjsg.jpg

binaries/data/mods/public/gui/summary/summary.js
339 ↗(On Diff #6002)

" rather than ' is more commonly used.

Wondering if units should be included (ie. "Time elapsed (minutes:seconds)"). Meh, it should be clear enough.

elexis added a comment.Mar 3 2018, 4:48 AM

Thanks Vladislav, the patch looks good and the feature is great!

binaries/data/mods/public/gui/summary/summary.js
337 ↗(On Diff #6002)

Engine.GetGUIObjectByName("chart[" + number + "]").format = g_ScorePanelsData[category].headings[itemNumber + 1].format || "INTEGER";

339 ↗(On Diff #6002)

and xAxisLabel can be inlined

binaries/data/mods/public/gui/summary/summary.xml
198 ↗(On Diff #2427)

Typically we have those indices at the end of the property name, right?
So chartXAxisLabel[k] might be more expected

source/gui/CChart.cpp
35 ↗(On Diff #5872)

Maybe not formats, but fonts don't seem too unlikely.
But can keep as is as long as we have only one font option.

175 ↗(On Diff #5872)

(I almost had suggested it again)

elexis added a comment.Mar 3 2018, 4:52 AM

I do count 3 people wondering about the units however and in scientific presentations we also displays the units too.
But the units are usually not part of the title but of the values, i.e. 3s, 3m 20s, 3h 20m.
So maybe we could add a time format in a follow-up patch. (equally the 100k score format)

vladislavbelov added inline comments.Mar 3 2018, 10:16 AM
binaries/data/mods/public/gui/summary/summary.xml
198 ↗(On Diff #2427)

No, all above are in the same style, i.e. chart[k]TypeLabel.

Style fixes.

Vulcan added a comment.Mar 3 2018, 1:54 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 3 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 507| 507| 	Engine.GetGUIObjectByName("summaryText").caption =
| 508| 508| 		g_GameData.gui.isInGame ?
| 509| 509| 			translate("Current Scores") :
| 510|    |-		g_GameData.gui.isReplay ?
|    | 510|+			g_GameData.gui.isReplay ?
| 511| 511| 			translate("Scores at the end of the game.") :
| 512| 512| 		g_GameData.gui.disconnected ?
| 513| 513| 			translate("You have been disconnected.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 508| 508| 		g_GameData.gui.isInGame ?
| 509| 509| 			translate("Current Scores") :
| 510| 510| 		g_GameData.gui.isReplay ?
| 511|    |-			translate("Scores at the end of the game.") :
|    | 511|+				translate("Scores at the end of the game.") :
| 512| 512| 		g_GameData.gui.disconnected ?
| 513| 513| 			translate("You have been disconnected.") :
| 514| 514| 		!assignedState ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 4 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 509| 509| 			translate("Current Scores") :
| 510| 510| 		g_GameData.gui.isReplay ?
| 511| 511| 			translate("Scores at the end of the game.") :
| 512|    |-		g_GameData.gui.disconnected ?
|    | 512|+				g_GameData.gui.disconnected ?
| 513| 513| 			translate("You have been disconnected.") :
| 514| 514| 		!assignedState ?
| 515| 515| 			translate("You have left the game.") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 510| 510| 		g_GameData.gui.isReplay ?
| 511| 511| 			translate("Scores at the end of the game.") :
| 512| 512| 		g_GameData.gui.disconnected ?
| 513|    |-			translate("You have been disconnected.") :
|    | 513|+					translate("You have been disconnected.") :
| 514| 514| 		!assignedState ?
| 515| 515| 			translate("You have left the game.") :
| 516| 516| 		assignedState.state == "won" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 5 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 511| 511| 			translate("Scores at the end of the game.") :
| 512| 512| 		g_GameData.gui.disconnected ?
| 513| 513| 			translate("You have been disconnected.") :
| 514|    |-		!assignedState ?
|    | 514|+					!assignedState ?
| 515| 515| 			translate("You have left the game.") :
| 516| 516| 		assignedState.state == "won" ?
| 517| 517| 			translate("You have won the battle!") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 512| 512| 		g_GameData.gui.disconnected ?
| 513| 513| 			translate("You have been disconnected.") :
| 514| 514| 		!assignedState ?
| 515|    |-			translate("You have left the game.") :
|    | 515|+						translate("You have left the game.") :
| 516| 516| 		assignedState.state == "won" ?
| 517| 517| 			translate("You have won the battle!") :
| 518| 518| 		assignedState.state == "defeated" ?
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 6 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 513| 513| 			translate("You have been disconnected.") :
| 514| 514| 		!assignedState ?
| 515| 515| 			translate("You have left the game.") :
| 516|    |-		assignedState.state == "won" ?
|    | 516|+						assignedState.state == "won" ?
| 517| 517| 			translate("You have won the battle!") :
| 518| 518| 		assignedState.state == "defeated" ?
| 519| 519| 			translate("You have been defeated…") :
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 514| 514| 		!assignedState ?
| 515| 515| 			translate("You have left the game.") :
| 516| 516| 		assignedState.state == "won" ?
| 517|    |-			translate("You have won the battle!") :
|    | 517|+							translate("You have won the battle!") :
| 518| 518| 		assignedState.state == "defeated" ?
| 519| 519| 			translate("You have been defeated…") :
| 520| 520| 			translate("You have abandoned the game.");
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 7 tabs but found 2.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 515| 515| 			translate("You have left the game.") :
| 516| 516| 		assignedState.state == "won" ?
| 517| 517| 			translate("You have won the battle!") :
| 518|    |-		assignedState.state == "defeated" ?
|    | 518|+							assignedState.state == "defeated" ?
| 519| 519| 			translate("You have been defeated…") :
| 520| 520| 			translate("You have abandoned the game.");
| 521| 521| 
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 516| 516| 		assignedState.state == "won" ?
| 517| 517| 			translate("You have won the battle!") :
| 518| 518| 		assignedState.state == "defeated" ?
| 519|    |-			translate("You have been defeated…") :
|    | 519|+								translate("You have been defeated…") :
| 520| 520| 			translate("You have abandoned the game.");
| 521| 521| 
| 522| 522| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 8 tabs but found 3.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 517| 517| 			translate("You have won the battle!") :
| 518| 518| 		assignedState.state == "defeated" ?
| 519| 519| 			translate("You have been defeated…") :
| 520|    |-			translate("You have abandoned the game.");
|    | 520|+								translate("You have abandoned the game.");
| 521| 521| 
| 522| 522| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 523| 523| 		translate("Game time elapsed: %(time)s"), {
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/gui/summary/summary.js
| 522| 522| 	Engine.GetGUIObjectByName("timeElapsed").caption = sprintf(
| 523| 523| 		translate("Game time elapsed: %(time)s"), {
| 524| 524| 			"time": timeToString(g_GameData.sim.timeElapsed)
| 525|    |-	});
|    | 525|+		});
| 526| 526| 
| 527| 527| 	let mapType = g_Settings.MapTypes.find(type => type.Name == g_GameData.sim.mapSettings.mapType);
| 528| 528| 	let mapSize = g_Settings.MapSizes.find(size => size.Tiles == g_GameData.sim.mapSettings.Size || 0);

Link to build: https://jenkins.wildfiregames.com/job/differential/142/display/redirect

elexis added inline comments.Mar 3 2018, 1:57 PM
binaries/data/mods/public/gui/summary/summary.js
335 ↗(On Diff #6014)

ack, if it still works

vladislavbelov added inline comments.Mar 3 2018, 2:11 PM
binaries/data/mods/public/gui/summary/summary.js
335 ↗(On Diff #6014)

What do you mean?

elexis added inline comments.Mar 3 2018, 4:10 PM
binaries/data/mods/public/gui/summary/summary.js
335 ↗(On Diff #6014)

Acknowledged, good change, go commit :-)

vladislavbelov added inline comments.Mar 3 2018, 4:27 PM
binaries/data/mods/public/gui/summary/summary.js
335 ↗(On Diff #6014)

Will you accept it?

elexis added inline comments.Mar 3 2018, 4:40 PM
binaries/data/mods/public/gui/summary/summary.js
335 ↗(On Diff #6014)

s0600204 did the review, I only added a style remark and some other comments.
I don't want to accept because I didn't review the entire code in every detail and don't want to be primarily liable for bugfixes.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2018, 1:44 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Mar 4 2018, 1:44 PM