Merge lp:~widelands-dev/widelands/cleanup-statistics-plotter into lp:widelands

[Toni Förster]

Clean-up statistics' plotter:

- add ticks at 1/4 and 3/4 on the y-axes
- move y-axes ticks to the right
- draw max_value atop the y-axes
- draw a 0 instead of -0
- draw min value instead of 0
- make min_value green

The old plot was a little hard to read, so I did some tweaking.

Here are some comparison pictures:

https://fosuta.org/pics/old_stats_1.png
https://fosuta.org/pics/new_stats_1.png

https://fosuta.org/pics/old_stats_2.png
https://fosuta.org/pics/new_stats_2.png

https://fosuta.org/pics/old_stats_2.png
https://fosuta.org/pics/new_stats_3.png

Since it there a no breaking changes, it would be nice if it could
be part of b20. Assumed you agree with the changes.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4654. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/511761507.
Appveyor build 4441. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cleanup_statistics_plotter-4441.

[hessenfarmer (stephan-lutz)]

I could agree with the changes except the min value of the y axis (warestatistics) plotted below the x-Axis. There it overwrites the "0" of the x-axis which looks weird. (see newstats_1).

I would vote against having it for b20 as it is no bugfix and we don't want to end up in discussions which branch is small enough to make it into b20. I think we should be equally strict to every branch / developer. For me we should only fix real showstoppers now and having b20 asap. Showstoppers means in my opinion severe bugs that happen frequently. Everything else should be postponed.

[Toni Förster (stonerl)]

> I could agree with the changes except the min value of the y axis
> (warestatistics) plotted below the x-Axis. There it overwrites the
> "0" of the x-axis which looks weird. (see newstats_1).

I did this on purpose. The 0 doesn't get overwritten, it gets replaces
by the min_value. But let me explain why I came to this design.

The Economy Health Graph is very unintuitive and hard to read. I
noticed this many times when playing with friends, where they didn't
really understand the graph.

The reasons are manifold. First of all the min and max value are inside
the quadrant and therefore are hard to read, because many times they
are overwritten by the plot lines. Secondly the min value is in red and
next to the red zero of the x-axes. Especially for small values the min
value could be mistaken for an erroneously drawn x-axes value.

Since I moved the max value atop the y-axes, the min value was still
in the lower right corner of the quadrant and now it looked even weirder.
Also we would still have the problem that the graph could overwrite the
min value and make it unreadable.

So I decided to remove the 0 from the x-axes and draw the min value
instead. It looked better IMHO but it was still red and easily to be
confused with the time units from the x-axes. Therefore, I decided to give
it the same colour as the max value. This should visually indicate that
both values belong to each other. It also makes it look more symmetrical.

I can provide some pictures with the min value in it original position,
but it really looks out of place there.

> I would vote against having it for b20 as it is no bugfix[...]

That's fine with me.

[GunChleoc (gunchleoc)]

Since we're down to 1 bug to fix for Build 20, feature freeze needs to be taken very seriously. Any innocent-looking line of code can potentially introduce a breaking bug that would force us to postpone Build 20.

Code LGTM :)

I think having the min value in the statistics from newstats_1 is a good idea, because the bottom value is not 0. Since it bothers hessenfarmer, maybe there is a way to display both?

[hessenfarmer (stephan-lutz)]

Did you try to display the y-values right of the y-axis instead of centered above and below. this would look familiar to me.

[Toni Förster (stonerl)]

> maybe there is a way to display both?

I tried this as well, but there isn't enough space. We cannot move the
y-axes label to the right-hand side, because the labels value can be a
anything from a one digit number up to a five digit number. To meet
these requirements, the space to the right-hand side would have to be
either variable or 50 maybe 70 pixel wide.

Either solution would look very awkward.

The numbers are centred atop and beneath the y-axes for single digit
numbers. All additional decimal numbers are prefixed

Since the code is shared between all plots, I think the way it is now is
the best compromise. Aesthetically pleasing, informative, easy to
understand and it fits all graphs.

Furthermore, the economy health graph is the only differential graph and
therefore, needs to be handled a little different from the others.

[GunChleoc (gunchleoc)]

I think we should make more space to the right.

We could also make the code even more generic. I have been thinking of a function that will take a vector of labels, the line length and whether it's to be plotted vertically or horizontally. Then iterate through the vector and calculate the position according to size of the vector + plot length to draw both the tick and the label text. Some of the labels would be empty.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4688. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/517020546.
Appveyor build 4474. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cleanup_statistics_plotter-4474.

[GunChleoc (gunchleoc)]

Do you want to try hour hand at the refactoring that I suggested above before we merge this?

[Toni Förster (stonerl)]

I'd say we merge it as it is now. I'll look into this when I find more time.

[GunChleoc (gunchleoc)]

OK, let's have it then :)

@bunnybot merge

review: Approve

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4780. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/523885621.
Appveyor build 4536. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_cleanup_statistics_plotter-4536.

[bunnybot (widelandsofficial)]

Refusing to merge, since Travis is not green. Use @bunnybot merge force for merging anyways.

Travis build 4780. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/523885621.

[GunChleoc (gunchleoc)]

Transient failure on Travis

@bunnybot merge force

=== modified file 'src/wui/general_statistics_menu.cc'
--- src/wui/general_statistics_menu.cc	2019-02-23 11:00:49 +0000
+++ src/wui/general_statistics_menu.cc	2019-04-24 08:25:42 +0000
@@ -42,7 +42,7 @@
 
 using namespace Widelands;
 
-#define PLOT_HEIGHT 130
+#define PLOT_HEIGHT 145
 #define NR_BASE_DATASETS 11
 
 GeneralStatisticsMenu::GeneralStatisticsMenu(InteractiveGameBase& parent,

=== modified file 'src/wui/plot_area.cc'
--- src/wui/plot_area.cc	2019-02-23 11:00:49 +0000
+++ src/wui/plot_area.cc	2019-04-24 08:25:42 +0000
@@ -245,7 +245,7 @@
 	   kAxisLineColor, kAxisLinesWidth);
 
 	//  Y Axis
-	dst.draw_line_strip({Vector2f(inner_w - kSpaceRight, kSpacing),
+	dst.draw_line_strip({Vector2f(inner_w - kSpaceRight, kSpacing * 3),
 	                     Vector2f(inner_w - kSpaceRight, inner_h - kSpaceBottom)},
 	                    kAxisLineColor, kAxisLinesWidth);
 	//  No Arrow here, since this doesn't continue.
@@ -260,7 +260,7 @@
 
 		// The space at the end is intentional to have the tick centered
 		// over the number, not to the left
-		if (how_many_ticks != 0) {
+		if (how_many_ticks != 0 && i != 0) {
 			std::shared_ptr<const UI::RenderedText> xtick = UI::g_fh->render(
 			   xtick_text_style((boost::format("-%u ") % (max_x / how_many_ticks * i)).str()));
 			Vector2i pos(posx, inner_h - kSpaceBottom + 10);
@@ -271,13 +271,29 @@
 		posx -= sub;
 	}
 
-	//  draw yticks, one at full, one at half
-	dst.draw_line_strip(
-	   {Vector2f(inner_w - kSpaceRight, kSpacing), Vector2f(inner_w - kSpaceRight - 3, kSpacing)},
-	   kAxisLineColor, kAxisLinesWidth);
-	dst.draw_line_strip(
-	   {Vector2f(inner_w - kSpaceRight, kSpacing + ((inner_h - kSpaceBottom) - kSpacing) / 2),
-	    Vector2f(inner_w - kSpaceRight - 3, kSpacing + ((inner_h - kSpaceBottom) - kSpacing) / 2)},
+	//  draw yticks, one at full, one at three-quarter, one at half, one at quarter & 0
+	dst.draw_line_strip(
+	   {Vector2f(inner_w - kSpaceRight + 3 , kSpacing * 3), Vector2f(inner_w - kSpaceRight - 3, kSpacing * 3)},
+	   kAxisLineColor, kAxisLinesWidth);
+
+	dst.draw_line_strip(
+	   {Vector2f(inner_w - kSpaceRight + 2, kSpacing * 3 + ((((inner_h - kSpaceBottom) + kSpacing * 3) / 2) - kSpacing * 3) / 2),
+	    Vector2f(inner_w - kSpaceRight, kSpacing * 3 + ((((inner_h - kSpaceBottom) + kSpacing * 3) / 2) - kSpacing * 3) / 2)},
+	   kAxisLineColor, kAxisLinesWidth);
+
+	dst.draw_line_strip(
+	   {Vector2f(inner_w - kSpaceRight + 3, ((inner_h - kSpaceBottom) + kSpacing * 3) / 2),
+	    Vector2f(inner_w - kSpaceRight, ((inner_h - kSpaceBottom) + kSpacing * 3) / 2)},
+	   kAxisLineColor, kAxisLinesWidth);
+
+	dst.draw_line_strip(
+	   {Vector2f(inner_w - kSpaceRight + 2, inner_h - kSpaceBottom - (inner_h - kSpaceBottom - ((inner_h - kSpaceBottom) + kSpacing * 3) / 2) / 2),
+	    Vector2f(inner_w - kSpaceRight, inner_h - kSpaceBottom - (inner_h - kSpaceBottom - ((inner_h - kSpaceBottom) + kSpacing * 3) / 2) / 2)},
+	   kAxisLineColor, kAxisLinesWidth);
+
+	dst.draw_line_strip(
+	   {Vector2f(inner_w - kSpaceRight + 3, inner_h - kSpaceBottom),
+	    Vector2f(inner_w - kSpaceRight, inner_h - kSpaceBottom)},
 	   kAxisLineColor, kAxisLinesWidth);
 
 	//  print the used unit
@@ -303,7 +319,7 @@
      needs_update_(true),
      lastupdate_(0),
      xline_length_(get_inner_w() - kSpaceRight - kSpacing),
-     yline_length_(get_inner_h() - kSpaceBottom - kSpacing),
+     yline_length_(get_inner_h() - kSpaceBottom - kSpacing * 3),
      time_ms_(0),
      highest_scale_(0),
      sub_(0),
@@ -463,6 +479,9 @@
 	dst.tile(Recti(Vector2i::zero(), get_inner_w(), get_inner_h()), g_gr->images().get(BG_PIC),
 	         Vector2i::zero());
 	draw_plot(dst, get_inner_h() - kSpaceBottom, std::to_string(highest_scale_), highest_scale_);
+	// Print the 0
+	draw_value((boost::format("%u") % (0)).str(), RGBColor(255, 0, 0),
+	           Vector2i(get_inner_w() - kSpaceRight + 3, get_inner_h() - kSpaceBottom + 10), dst);
 }
 
 void WuiPlotArea::draw_plot(RenderTarget& dst,
@@ -484,7 +503,8 @@
 
 	//  print the maximal value into the top right corner
 	draw_value(yscale_label, RGBColor(60, 125, 0),
-	           Vector2i(get_inner_w() - kSpaceRight - 3, kSpacing + 2), dst);
+	           Vector2i(get_inner_w() - kSpaceRight + 3, kSpacing + 2), dst);
+
 }
 
 /**
@@ -677,7 +697,7 @@
 	         Vector2i::zero());
 
 	// yoffset of the zero line
-	float const yoffset = kSpacing + ((get_inner_h() - kSpaceBottom) - kSpacing) / 2;
+	float const yoffset = ((get_inner_h() - kSpaceBottom) + kSpacing * 3) / 2;
 
 	// draw zero line
 	dst.draw_line_strip({Vector2f(get_inner_w() - kSpaceRight, yoffset),
@@ -688,8 +708,8 @@
 	draw_plot(dst, yoffset, std::to_string(highest_scale_), 2 * highest_scale_);
 
 	// Print the min value
-	draw_value((boost::format("-%u") % (highest_scale_)).str(), RGBColor(125, 0, 0),
-	           Vector2i(get_inner_w() - kSpaceRight - 3, get_inner_h() - kSpacing - 23), dst);
+	draw_value((boost::format("-%u") % (highest_scale_)).str(), RGBColor(60, 125, 0),
+	           Vector2i(get_inner_w() - kSpaceRight + 3, get_inner_h() - kSpaceBottom + 10), dst);
 }
 
 /**

=== modified file 'src/wui/ware_statistics_menu.cc'
--- src/wui/ware_statistics_menu.cc	2019-02-23 11:00:49 +0000
+++ src/wui/ware_statistics_menu.cc	2019-04-24 08:25:42 +0000
@@ -34,7 +34,7 @@
 #include "wui/plot_area.h"
 #include "wui/waresdisplay.h"
 
-constexpr int kPlotHeight = 130;
+constexpr int kPlotHeight = 145;
 constexpr int kPlotWidth = 250;
 constexpr int kSpacing = 5;