Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the rounding issue of floating point numbers in category scale #5880

Merged
merged 1 commit into from
Dec 9, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Dec 3, 2018

This PR fixes following problems.

  1. Points on the right edge sometimes disappear when using a category scale for x axis (mentioned in Add error margin for detecting if a point or line is in the chartArea #5790).
  2. There is sometimes 1 pixel width gap between filled areas and the rightmost grid line when using a category scale for x axis in a line chart ([BUG] Gap at the right end in an Area chart #5797).
  3. A line with many points looks bumpy when using a category scale for x axis in a line chart (Line Graph line not smooth / Updating max tick limit #5297)
  4. A stepped line with many points is sometimes not drawn when using a category scale for x axis in a line chart ([BUG] Stepped line graph, thin lines disappear on certian screen widths #5576)
  5. 1px spaces are seen between bars with barPercentage and categoryPercentage set to 1 (Bar Graph Render Issue in 2.5.0 #3916, [BUG] Spacing between vertical bars with percentages set to 1 #3964)
  6. Grid lines are blurred in comparison with v2.6.0 ([BUG] Sometimes the axes lose their sharpness #4640, Blurred lines after updating from 2.6.0 to 2.7.2 #5431, Blurred bar border after updating from 2.6.0 to 2.7.2 #5532).
  7. Tick lines are not supposed to across or to be off the axis border (by Don't draw tick across axis/border #5178), but this doesn’t work when the width is more than 1px.
  8. Gridlines intersect with the axis border (mentioned in Don't draw tick across axis/border #5178 but not resolved).
  9. An axis border is drawn even if its width is set to 0.

The following changes are made in this PR.

  • For 1, 2, 3, 4 and 5: Math.round in the category scale code is removed. In my opinion, rounding should be done not in getPixelForTick but in drawing functions to avoid detection errors.
  • 6: The position of grid/tick/axis border lines is aligned in order to avoid anti-aliasing blur when its width is an integer.
  • 7 and 8: the calculation for line positions is fixed.
  • 9: The width of the axis border is checked.

Master: https://jsfiddle.net/nagix/52o0qn47/

screen shot 2018-12-04 at 10 07 35 pm

This PR: https://jsfiddle.net/nagix/upv21qto/

screen shot 2018-12-04 at 10 07 11 pm

Fixes #3916
Fixes #3964
Fixes #4640
Fixes #5297
Fixes #5431
Fixes #5532
Fixes #5576
Fixes #5797

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nagix, it looks good.

This scale.draw method is quite complex, we would need to clean it deeper (not in this PR of course). Still, a few suggestions that I think would make the impacted code simpler.

* @param {Number} lineWidth - A line width.
* @returns {Number} The aligned line pixel value.
*/
helpers.alignLinePixel = function(chart, linePixel, lineWidth) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would declare this method private for sure to not have to deal with deprecation if we realize we don't need it anymore or if we want to change its signature, and maybe rename it for something more generic and shorter:

helpers._alignPixel = function(chart, pixel, width) {
    // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

var alignedTop = helpers.alignLinePixel(chart, me.top, axisWidth);
var alignedBottom = helpers.alignLinePixel(chart, me.bottom, axisWidth);

var xTickStart, xTickEnd, yTickStart, yTickEnd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not wrong, depending on the scale orientation, only half of these variables (including the new aligned*) are effectively used so I think we could simplify for something like:

var axisWidth = helpers.valueAtIndexOrDefault(gridLines.lineWidth, 0, 0);
var alignPixel = helpers._alignPixel;
var aligned, tickStart, tickEnd;

if (position === 'top') {
    aligned = alignPixel(chart, me.bottom, axisWidth);
    tickStart = me.bottom - tl;
    tickEnd = aligned - axisWidth / 2;
} else if (position === 'bottom') {
    aligned = alignPixel(chart, me.top, axisWidth);
    tickStart = aligned + axisWidth / 2;
    tickEnd = me.top + tl;
} else if (position === 'left') {
    aligned = alignPixel(chart, me.right, axisWidth);
    tickStart = me.right - tl;
    tickEnd = aligned - axisWidth / 2;
} else {
    aligned = alignPixel(chart, me.left, axisWidth);
    tickStart = aligned + axisWidth / 2;
    tickEnd = me.left + tl;
}

Feel free to rename aligned for whatever you think it's better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this simplification. I renamed it to borderValue as it stores the position of the axis border.

y1 = y2 = options.position === 'top' ? me.bottom : me.top;
y1 += aliasPixel;
y2 += aliasPixel;
y1 = y2 = position === 'top' ? alignedBottom : alignedTop;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are computing too many values here and based on my previous comment, I think we can rewrite this part as:

var x1, x2, y1, y2;
if (isHorizontal) {
    x1 = alignPixel(chart, me.left, firstLineWidth) - firstLineWidth / 2;
    x2 = alignPixel(chart, me.right, lastLineWidth) + lastLineWidth / 2;
    y1 = y2 = aligned;
} else {
    y1 = alignPixel(chart, me.top, firstLineWidth) - firstLineWidth / 2;
    y2 = alignPixel(chart, me.bottom, lastLineWidth) + lastLineWidth / 2;
    x1 = x2 = aligned;
}

context.moveTo(x1, y1);
context.lineTo(x2, y2);
context.stroke();
if (axisWidth) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! do we also need to compute all the code above if at the end we draw nothing? or should we break earlier, for example at line 933 (if (gridLines.drawBorder && axisWidth))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized axisWidth has to be set to 0 if gridLines.drawBorder is false, so I moved this logic at the beginning of draw.

- Remove `Math.round` in the category scale code
- Add `helpers._alignPixel` to align grid/tick/axis border lines
- Fix grid/tick/axis border line calculation
- Add a check of the width of the axis border
- Refactor core.scale code
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@simonbrunel simonbrunel merged commit 40495ec into chartjs:master Dec 9, 2018
@nagix nagix deleted the issue-5880 branch December 9, 2018 16:14
@yangwonlohengramm
Copy link

Hi, I'm wondering what version of Chart.js this is? Reason being I need to use a version of angular-chart that contains this fix and I'm hoping there is a corresponding version there.

Thanks!

@benmccann
Copy link
Contributor

This PR is in Chart.js 2.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment