Histogram bug fixes

[odmir]

• The implementation of drawSolidPolygon and drawPlotLines for each renderer now checks for the number of points passed in before doing any drawing.
• Under some circumstances the x labels would be shown with many decimal places, this was due to numeric errors and is now fixed by eliminating one divide followed by a multiplication by the same number. (We should eventually have some way of controlling the number of decimal places for the labels with a sensible default)
• We were rounding the value of the barWidth which in histograms with a lot of bins could lead to significant offsets for each bar relative to the x markers. Removing this rounding introduces another bug though: introduction of white lines of variable width in between the bars due to antialiasing and painter's algorithm used by some renderers.
• Refactor drawData and fix white lines by naïvely drawing polygons with all of the points of each series, the series being drawn on top of each other, with most of the surface being redrawn each time. This leads the colours from series in the back to mix with the colours of series overlaid on top, at the edges of bars.
• Implement a better algorithm for .bar that "tucks" the series on the back slightly behind the ones in front, by flooring or ceiling the points coordinates at the right places. This fixes white lines between different series and the colour of background series bleeding through the edges of foreground series.
• Improve algorithm for .step by removing one "instruction" and reducing the times other instructions were being issued to the bare minimum without modifying the output files.

Difference between before and after fixing the rounded barWidth and the rest:

The difference between the before and after fixing the colour bleeding can be seen here (its easier to see on the right side where there are lots of different series of different colours side by side):

[KarthikRIyer]

Looks good to me but I'll wait if @karwa has any suggestions...

[karwa]

The implementation of drawSolidPolygon and drawPlotLines for each renderer now checks for the number of points passed in before doing any drawing.

Hmm... I'm not sure if we should just silently return without drawing anything if the number of points are invalid. It might be worth making these in to precondition(...) checks, which crash on invalid input.

In general it's not a good idea to silently return without doing anything - they can be difficult to debug or mask other errors. We told the renderer to draw, so it should draw (or fail loudly, telling me why it couldn't draw).

Under some circumstances the x labels would be shown with many decimal places, this was due to numeric errors and is now fixed by eliminating one divide followed by a multiplication by the same number. (We should eventually have some way of controlling the number of decimal places for the labels with a sensible default)

Thanks very much for tracking this down! This looks much better! 😍

• We were rounding the value of the barWidth which in histograms with a lot of bins could lead to significant offsets for each bar relative to the x markers. Removing this rounding introduces another bug though: introduction of white lines of variable width in between the bars due to antialiasing and painter's algorithm used by some renderers.

• Refactor drawData and fix white lines by naïvely drawing polygons with all of the points of each series, the series being drawn on top of each other, with most of the surface being redrawn each time. This leads the colours from series in the back to mix with the colours of series overlaid on top, at the edges of bars.

• Implement a better algorithm for .bar that "tucks" the series on the back slightly behind the ones in front, by flooring or ceiling the points coordinates at the right places. This fixes white lines between different series and the colour of background series bleeding through the edges of foreground series.

OK, so there are 2 parts to this: the bar/marker alignment, and aliasing. The first is an accuracy issue, and the latter is the an image quality issue.

As for the first part: it's definitely worth fixing, so thanks! 🙂 However, rather than removing the rounding, have you tried making the markers round in the same way as the bars? In general, we would like to round to integer coordinates, which leads me on the second issue.

Personally, I would prefer that we deferred tackling the aliasing issue until after GCI. This is an issue which affects basically everything that we draw, and there are several things to consider for a complete solution. It's not enough to just collapse everything in to a polygon and not use rectangles ever - sometimes we need to draw individual rectangles, and we need them to be sharp whenever we do it. But aliasing sucks - for many plots, it totally ruins the output. So we should try to avoid it by rounding to integer coordinates and sizes whenever possible. (Also, we always want to round down - there's no point drawing 2 pixels/bar if you don't even have that much space).

The only time when it isn't possible to round is when you're plotting a huge amount of data in a very small plot, so each element gets less than 1 pixel to display in. This leads to another issue - let's say I have 800 bars and a plot width of 900 pixels. Rounding would give us 1 pixel/bar, with a massive 100 pixels (11% of the plot width) completely empty. And it gets worse - if you increased the plot width to 1500 pixels, the data would still only occupy 800 pixels, and we'd need to figure out what to do with the remaining 700 (47% of the plot).

There are 2 options for how to use that space:

1. Add margins to the data (inside the plot), so it is at least centred. We already add some margins, so it shouldn't be too difficult to increase the size of them a bit.
2. Shrink the plot size. Some plots (e.g. heatmaps) like to go right up to the axes, without any margins.

So to summarise, there are 3 ways to lay out the plot:

• don't round (and accept aliasing)
• round down and add margins
• round down and shrink plot size

We should give the user control over which mode they prefer. We might also have some kind of adaptive option which resolves to one of those 3 based on some inspection of the data and image size.

But again, this affects every plot, and is a sufficiently large issue that I think we should wait until after GCI. For now I think we should continue to use rectangles for the histogram's bar mode, even if it means the output can suffer from aliasing, and then later we'll add some control over the rounding/layout mode and fix all of the plots, gridlines, etc to use it.

Improve algorithm for .step by removing one "instruction" and reducing the times other instructions were being issued to the bare minimum without modifying the output files.

Cool, sounds good 👍

[odmir]

Hmm... I'm not sure if we should just silently return without drawing anything if the number of points are invalid. It might be worth making these in to precondition(...) checks, which crash on invalid input.

In general it's not a good idea to silently return without doing anything - they can be difficult to debug or mask other errors. We told the renderer to draw, so it should draw (or fail loudly, telling me why it couldn't draw).

I agree and will change it to a precondition with a helpful message. The reason I had changed it to bail out was because the algorithms assumed they could ask to draw without enough points as a way to reduce the complexity of the rules, but honestly it doesn't make much of a difference to the rules now that I am checking them and there were not many cases where the algorithm did this.
I will adjust the rules for the .step case so this definitely does not happen after having the preconditions.

As for the first part: it's definitely worth fixing, so thanks! 🙂 However, rather than removing the rounding, have you tried making the markers round in the same way as the bars? In general, we would like to round to integer coordinates, which leads me on the second issue.

No, I have not tried that, I started thinking that that would lead to us having to adjust everything else as you say afterwards so I though this would be the better option. But the way it is now the grid lines and the markers will almost never be crisp nor will the bar edges, so what you say makes sense.

But again, this affects every plot, and is a sufficiently large issue that I think we should wait until after GCI. For now I think we should continue to use rectangles for the histogram's bar mode, even if it means the output can suffer from aliasing, and then later we'll add some control over the rounding/layout mode and fix all of the plots, gridlines, etc to use it.

Should I then leave the barWidth un-rounded and we tackle making it rounded, and making the grid lines and markers match it, at a later time, leaving the aliasing white lines there, or should I revert the barWidth to being rounded, so that for the time being we don't need to live with the white lines?

So these are the changes I will make for now:

• guard -> precondition, with helpful messages.
• Change .step to use rules that make it impossible to ask to draw un-drawable lines.
• Revert .bar to use rectangles.
• For now revert barWidth to being rounded as it was before.

[odmir]

Apparently I already did step 2:

Improve algorithm for .step by removing one "instruction" and reducing the times other instructions were being issued to the bare minimum without modifying the output files.

and forgot that it also implied not being possible to ask to draw lines with less than 2 points.

[odmir]

Ok, finished with the changes for now. Thank you for your time reviewing!

[odmir]

@karwa, I was thinking about this aside you talked about:

(Also, we always want to round down - there's no point drawing 2 pixels/bar if you don't even have that much space)

And thought that maybe you were referring to my use of .rounded(.up) and .rounded(.down).
If so then maybe I should clarify that the algorithm does not change the width of the viewable parts of the bars (if they should have 1 px width then they will all be 1 px and draw without any antialiasing).
What I mean by this is that the rounding up or down is only used in the places where the bar of one series is side by side with the bar of another series. Like this:

Blue: foreground series.
Yellow: Background series.
Green: Pixel boundaries.

As you can see it only rounds up or down the correct points, that will be hidden behind the series in the foreground, while keeping the visible width of both series the exact same.

Now that I think more about this, you said the following:

So to summarise, there are 3 ways to lay out the plot:

• don't round (and accept aliasing)
• round down and add margins
• round down and shrink plot size

We should give the user control over which mode they prefer. We might also have some kind of adaptive option which resolves to one of those 3 based on some inspection of the data and image size.

I was thinking that maybe the first option could be handled by this algorithm with the benefit that there will be no aliasing effects visible (by this I mean the white lines, not that the edges will not be aliased). The other two options could be handled by using simple rects or this algorithm as the result would be the exact same, its just a question of rounding down the barWidth, the only difference would probably be in the performance characteristics, but these were not measured, and in the file size for formats like SVG, in which case this algorithm could do significantly better as it "merges" the rectangles leading to less points being specified.
Now, this would be a solution (to the white lines in the first mode) specialised for a Histogram, but the logic to control in which mode we are is something that is probably general to all the other plot types as well.
I'm not saying we should implement those modes now, just that when they exist, this algorithm would do the exact same as the one using rects except in the first mode where it will fare better.

I wanted to clear up a little bit about what the algorithm does in case you tough it was working in another way and influencing your feedback.

Once again thank you for taking the time to read this!

[odmir]

Also I can answer any questions about how the algorithm works if it helps make a more informed decision.

[odmir]

@karwa I think you may have missed the last couple of comments in this PR, can I ask you to check them out when you're available?

[karwa]

And thought that maybe you were referring to my use of .rounded(.up) and .rounded(.down)

I was just referring to the sizing calculations. Both are required to really fix the aliasing issue, since it otherwise leads to an incorrect scale and unaligned offsets.

I this this PR is fine, I don't have anything to add ✅

[odmir]

Ok, I have fixed the merge conflicts, but I was a bit out of the loop lately so I may have made some mistakes that do not manifest in the tests due to recent changes. It would be nice if someone could have a quick look at the drawData method of the Histogram since there is where I'm less sure I didn't screw up something. The tests are passing and I don't see anything weird so I am assuming it is fine.

If everything is alright then this can be merged! @KarthikRIyer @karwa

[KarthikRIyer]

This looks fine. Now that GCI is over I think this can be merged.
Thanks a lot for your work @odmir . Appreciated!