Page MenuHomeWildfire Games
Differential D167

Fix CChart scaling edgecases
ClosedPublic
Actions

Authored by Imarok on Feb 26 2017, 2:58 PM.

Details

Reviewers
vladislavbelov
Commits
rP19408: Fix CChart scaling edgecases (constant value and infinity)
Trac Tickets
#3403
Summary

CChart displays nothing, when the series is constant or any value is Infinity.
This patch fixes this by assuming Infinityis 1.5 * the highest value.
The constant series bug is fixed by increasing the value range in this case by 1.

Test Plan

Display a graph with a constant series and one with an infinite value.

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

Imarok created this revision.Feb 26 2017, 2:58 PM
Owners added a subscriber: Restricted Owners Package.Feb 26 2017, 2:58 PM
Vulcan added a subscriber: Vulcan.Feb 26 2017, 3:43 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/414/ for more details.

Harbormaster completed remote builds in B606: Diff 622.Feb 26 2017, 3:43 PM
Imarok updated this revision to Diff 630.Feb 27 2017, 10:15 PM

Style (noticed by bb)

Vulcan added a comment.Feb 28 2017, 1:48 AM

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

Harbormaster completed remote builds in B615: Diff 630.Feb 28 2017, 1:48 AM
echotangoecho added a subscriber: echotangoecho.Mar 10 2017, 5:27 PM

Wouldn't it make more sense for nothing to be displayed in the case of an infinite value?

Imarok added a comment.EditedMar 10 2017, 8:26 PM
In D167#7761, @echotangoecho wrote:

Wouldn't it make more sense for nothing to be displayed in the case of an infinite value?

You mean no graph at all, or displaying a gap?
(a gap could work)

echotangoecho added a comment.Mar 10 2017, 8:28 PM

I mean displaying a gap.

Imarok added a comment.Mar 10 2017, 8:42 PM

I am not against it, but what would be the benefits?
(because the current approach is much easier to implement)

echotangoecho added a comment.Mar 10 2017, 8:47 PM

Well, it seems strange to use some random value for an infinite value, and infinite values should in practice not be displayed on the chart.

vladislavbelov requested changes to this revision.Mar 11 2017, 5:08 PM

X-values should have a check for infinity too, because the chart is a common object, so it could draw not only math functions, but vertical graphics too and any kind of poly-line.
The rest of the patch looks good for me.

This revision now requires changes to proceed.Mar 11 2017, 5:08 PM
Imarok updated this revision to Diff 1173.Apr 9 2017, 3:47 PM
Imarok edited edge metadata.

leave a gap when infinite value and handle X and Y similar

Imarok updated this revision to Diff 1174.Apr 9 2017, 3:50 PM

Remove \n

Harbormaster completed remote builds in B1061: Diff 1174.Apr 9 2017, 3:50 PM
Vulcan added a comment.Apr 9 2017, 4:44 PM

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

Harbormaster completed remote builds in B1060: Diff 1173.Apr 9 2017, 4:44 PM
vladislavbelov requested changes to this revision.Apr 9 2017, 5:06 PM
vladislavbelov added inline comments.
source/gui/CChart.cpp
98 ↗(On Diff #1174)

Should be std::numeric_limits<float>::infinity(), I think.

124 ↗(On Diff #1174)

The same here.

source/gui/CChart.h
67 ↗(On Diff #1174)

Function and vertices could be const, I think.

This revision now requires changes to proceed.Apr 9 2017, 5:06 PM
vladislavbelov added inline comments.Apr 9 2017, 5:06 PM
source/gui/CChart.cpp
136 ↗(On Diff #1174)

Should be if (!vertices.empty()), as wraitii said.

Vulcan added a comment.Apr 9 2017, 5:30 PM

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

Harbormaster completed remote builds in B1061: Diff 1174.Apr 9 2017, 5:31 PM
Imarok updated this revision to Diff 1197.Apr 10 2017, 4:30 PM
Imarok edited edge metadata.

apply remarks

Owners added a subscriber: Restricted Owners Package.Apr 10 2017, 4:30 PM
vladislavbelov requested changes to this revision.Apr 10 2017, 4:44 PM

Perhaps, elexis will want to split it in two commits: adding const and fixing infinity. But in this case I don't mind for single commit.
I like it.

source/gui/CChart.cpp
136 ↗(On Diff #1197)

Not fixed.

This revision now requires changes to proceed.Apr 10 2017, 4:44 PM
Imarok added inline comments.Apr 10 2017, 4:46 PM
source/gui/CChart.cpp
136 ↗(On Diff #1197)

Seems like I forgot to save :/

Imarok added a parent revision: D315: Add some consts to ShaderProgram functions.Apr 10 2017, 4:53 PM

depends on D315

Vulcan added a comment.Apr 10 2017, 5:52 PM

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

Harbormaster completed remote builds in B1079: Diff 1197.Apr 10 2017, 5:52 PM
Imarok updated this revision to Diff 1205.Apr 10 2017, 7:41 PM
Imarok edited edge metadata.

Rebase and add the not saved empty check

vladislavbelov accepted this revision.Apr 10 2017, 7:53 PM

The good one, also this helper really should be as non-static method, because it could be used in future to draw lines in different styles.

This revision is now accepted and ready to land.Apr 10 2017, 7:53 PM
Vulcan added a comment.Apr 10 2017, 10:49 PM

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

Harbormaster completed remote builds in B1087: Diff 1205.Apr 10 2017, 10:50 PM
Closed by commit rP19408: Fix CChart scaling edgecases (constant value and infinity) (authored by Imarok). · Explain WhyApr 11 2017, 2:28 PM
This revision was automatically updated to reflect the committed changes.
elexis added a subscriber: elexis.Apr 14 2017, 2:13 PM
In D167#12283, @vladislavbelov wrote:

Perhaps, elexis will want to split it in two commits: adding const and fixing infinity

Splitting the kind of unrelated Shader changes to D315 was a good decision IMO.
Thanks for taking care of those edge cases without using the workaround hack!

Revision Contents
Changeset List

PathSize
ps/
trunk/
source/
gui/
22 lines
54 lines

Diff 1210

View Options

ps/trunk/source/gui/CChart.h

Loading...
View Options

ps/trunk/source/gui/CChart.cpp

Loading...
Wildfire Games · Phabricator