Skip to content

Commit

Permalink
Fix the rounding issue of floating point numbers in category scale
Browse files Browse the repository at this point in the history 
- 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
  • Loading branch information
@nagix
nagix committed Dec 6, 2018
1 parent 3cb2d70 commit 9e7a4cc
Show file tree
Hide file tree
Showing 16 changed files with 134 additions and 94 deletions.
22 changes: 22 additions & 0 deletions src/core/core.helpers.js
View file
Expand Up @@ -214,9 +214,31 @@ module.exports = function() {
helpers.distanceBetweenPoints = function(pt1, pt2) {
return Math.sqrt(Math.pow(pt2.x - pt1.x, 2) + Math.pow(pt2.y - pt1.y, 2));
};

/**
* Provided for backward compatibility, not available anymore
* @function Chart.helpers.aliasPixel
* @deprecated since version 2.8.0
* @todo remove at version 3
*/
helpers.aliasPixel = function(pixelWidth) {
return (pixelWidth % 2 === 0) ? 0 : 0.5;
};

/**
* Returns the aligned pixel value to avoid anti-aliasing blur
* @param {Chart} chart - The chart instance.
* @param {Number} pixel - A pixel value.
* @param {Number} width - The width of the element.
* @returns {Number} The aligned pixel value.
* @private
*/
helpers._alignPixel = function(chart, pixel, width) {
var devicePixelRatio = chart.currentDevicePixelRatio;
var halfWidth = width / 2;
return Math.round((pixel - halfWidth) * devicePixelRatio) / devicePixelRatio + halfWidth;
};

helpers.splineCurve = function(firstPoint, middlePoint, afterPoint, t) {
// Props to Rob Spencer at scaled innovation for his post on splining between points
// http://scaledinnovation.com/analytics/splines/aboutSplines.html
Expand Down
147 changes: 79 additions & 68 deletions src/core/core.scale.js
View file
Expand Up @@ -580,7 +580,7 @@ module.exports = Element.extend({
pixel += tickWidth / 2;
}

var finalVal = me.left + Math.round(pixel);
var finalVal = me.left + pixel;
finalVal += me.isFullWidth() ? me.margins.left : 0;
return finalVal;
}
Expand All @@ -598,7 +598,7 @@ module.exports = Element.extend({
var innerWidth = me.width - (me.paddingLeft + me.paddingRight);
var valueOffset = (innerWidth * decimal) + me.paddingLeft;

var finalVal = me.left + Math.round(valueOffset);
var finalVal = me.left + valueOffset;
finalVal += me.isFullWidth() ? me.margins.left : 0;
return finalVal;
}
Expand Down Expand Up @@ -683,21 +683,26 @@ module.exports = Element.extend({
return;
}

var chart = me.chart;
var context = me.ctx;
var globalDefaults = defaults.global;
var optionTicks = options.ticks.minor;
var optionMajorTicks = options.ticks.major || optionTicks;
var gridLines = options.gridLines;
var scaleLabel = options.scaleLabel;
var position = options.position;

var isRotated = me.labelRotation !== 0;
var isMirrored = optionTicks.mirror;
var isHorizontal = me.isHorizontal();

var ticks = optionTicks.autoSkip ? me._autoSkip(me.getTicks()) : me.getTicks();
var tickFontColor = helpers.valueOrDefault(optionTicks.fontColor, globalDefaults.defaultFontColor);
var tickFont = parseFontOptions(optionTicks);
var majorTickFontColor = helpers.valueOrDefault(optionMajorTicks.fontColor, globalDefaults.defaultFontColor);
var majorTickFont = parseFontOptions(optionMajorTicks);
var tickPadding = optionTicks.padding;
var labelOffset = optionTicks.labelOffset;

var tl = gridLines.drawTicks ? gridLines.tickMarkLength : 0;

Expand All @@ -708,11 +713,27 @@ module.exports = Element.extend({

var itemsToDraw = [];

var axisWidth = helpers.valueAtIndexOrDefault(me.options.gridLines.lineWidth, 0);
var xTickStart = options.position === 'right' ? me.left : me.right - axisWidth - tl;
var xTickEnd = options.position === 'right' ? me.left + tl : me.right;
var yTickStart = options.position === 'bottom' ? me.top + axisWidth : me.bottom - tl - axisWidth;
var yTickEnd = options.position === 'bottom' ? me.top + axisWidth + tl : me.bottom + axisWidth;
var axisWidth = gridLines.drawBorder ? helpers.valueAtIndexOrDefault(gridLines.lineWidth, 0, 0) : 0;
var alignPixel = helpers._alignPixel;
var borderValue, tickStart, tickEnd;

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

var epsilon = 0.0000001; // 0.0000001 is margin in pixels for Accumulated error.

Expand All @@ -738,66 +759,58 @@ module.exports = Element.extend({
}

// Common properties
var tx1, ty1, tx2, ty2, x1, y1, x2, y2, labelX, labelY;
var textAlign = 'middle';
var tx1, ty1, tx2, ty2, x1, y1, x2, y2, labelX, labelY, textAlign;
var textBaseline = 'middle';
var tickPadding = optionTicks.padding;
var lineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);

if (isHorizontal) {
var labelYOffset = tl + tickPadding;

if (options.position === 'bottom') {
// bottom
textBaseline = !isRotated ? 'top' : 'middle';
textAlign = !isRotated ? 'center' : 'right';
labelY = me.top + labelYOffset;
} else {
// top
textBaseline = !isRotated ? 'bottom' : 'middle';
textAlign = !isRotated ? 'center' : 'left';
labelY = me.bottom - labelYOffset;
}

var xLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (xLineValue < me.left - epsilon) {
if (lineValue < me.left - epsilon) {
lineColor = 'rgba(0,0,0,0)';
}
xLineValue += helpers.aliasPixel(lineWidth);

labelX = me.getPixelForTick(index) + optionTicks.labelOffset; // x values for optionTicks (need to consider offsetLabel option)

tx1 = tx2 = x1 = x2 = xLineValue;
ty1 = yTickStart;
ty2 = yTickEnd;
y1 = chartArea.top;
y2 = chartArea.bottom + axisWidth;
} else {
var isLeft = options.position === 'left';
var labelXOffset;
tx1 = tx2 = x1 = x2 = alignPixel(chart, lineValue, lineWidth);
ty1 = tickStart;
ty2 = tickEnd;
labelX = me.getPixelForTick(index) + labelOffset; // x values for optionTicks (need to consider offsetLabel option)

if (optionTicks.mirror) {
textAlign = isLeft ? 'left' : 'right';
labelXOffset = tickPadding;
if (position === 'top') {
y1 = alignPixel(chart, chartArea.top, axisWidth) + axisWidth / 2;
y2 = chartArea.bottom;
textBaseline = !isRotated ? 'bottom' : 'middle';
textAlign = !isRotated ? 'center' : 'left';
labelY = me.bottom - labelYOffset;
} else {
textAlign = isLeft ? 'right' : 'left';
labelXOffset = tl + tickPadding;
y1 = chartArea.top;
y2 = alignPixel(chart, chartArea.bottom, axisWidth) - axisWidth / 2;
textBaseline = !isRotated ? 'top' : 'middle';
textAlign = !isRotated ? 'center' : 'right';
labelY = me.top + labelYOffset;
}
} else {
var labelXOffset = (isMirrored ? 0 : tl) + tickPadding;

labelX = isLeft ? me.right - labelXOffset : me.left + labelXOffset;

var yLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (yLineValue < me.top - epsilon) {
if (lineValue < me.top - epsilon) {
lineColor = 'rgba(0,0,0,0)';
}
yLineValue += helpers.aliasPixel(lineWidth);

labelY = me.getPixelForTick(index) + optionTicks.labelOffset;
tx1 = tickStart;
tx2 = tickEnd;
ty1 = ty2 = y1 = y2 = alignPixel(chart, lineValue, lineWidth);
labelY = me.getPixelForTick(index) + labelOffset;

tx1 = xTickStart;
tx2 = xTickEnd;
x1 = chartArea.left;
x2 = chartArea.right + axisWidth;
ty1 = ty2 = y1 = y2 = yLineValue;
if (position === 'left') {
x1 = alignPixel(chart, chartArea.left, axisWidth) + axisWidth / 2;
x2 = chartArea.right;
textAlign = isMirrored ? 'left' : 'right';
labelX = me.right - labelXOffset;
} else {
x1 = chartArea.left;
x2 = alignPixel(chart, chartArea.right, axisWidth) - axisWidth / 2;
textAlign = isMirrored ? 'right' : 'left';
labelX = me.left + labelXOffset;
}
}

itemsToDraw.push({
Expand Down Expand Up @@ -867,7 +880,7 @@ module.exports = Element.extend({
if (helpers.isArray(label)) {
var lineCount = label.length;
var lineHeight = tickFont.size * 1.5;
var y = me.isHorizontal() ? 0 : -lineHeight * (lineCount - 1) / 2;
var y = isHorizontal ? 0 : -lineHeight * (lineCount - 1) / 2;

for (var i = 0; i < lineCount; ++i) {
// We just make sure the multiline element is a string here..
Expand All @@ -891,11 +904,11 @@ module.exports = Element.extend({

if (isHorizontal) {
scaleLabelX = me.left + ((me.right - me.left) / 2); // midpoint of the width
scaleLabelY = options.position === 'bottom'
scaleLabelY = position === 'bottom'
? me.bottom - halfLineHeight - scaleLabelPadding.bottom
: me.top + halfLineHeight + scaleLabelPadding.top;
} else {
var isLeft = options.position === 'left';
var isLeft = position === 'left';
scaleLabelX = isLeft
? me.left + halfLineHeight + scaleLabelPadding.top
: me.right - halfLineHeight - scaleLabelPadding.top;
Expand All @@ -914,26 +927,24 @@ module.exports = Element.extend({
context.restore();
}

if (gridLines.drawBorder) {
if (axisWidth) {
// Draw the line at the edge of the axis
context.lineWidth = helpers.valueAtIndexOrDefault(gridLines.lineWidth, 0);
context.strokeStyle = helpers.valueAtIndexOrDefault(gridLines.color, 0);
var x1 = me.left;
var x2 = me.right + axisWidth;
var y1 = me.top;
var y2 = me.bottom + axisWidth;
var firstLineWidth = axisWidth;
var lastLineWidth = helpers.valueAtIndexOrDefault(gridLines.lineWidth, ticks.length - 1, 0);
var x1, x2, y1, y2;

var aliasPixel = helpers.aliasPixel(context.lineWidth);
if (isHorizontal) {
y1 = y2 = options.position === 'top' ? me.bottom : me.top;
y1 += aliasPixel;
y2 += aliasPixel;
x1 = alignPixel(chart, me.left, firstLineWidth) - firstLineWidth / 2;
x2 = alignPixel(chart, me.right, lastLineWidth) + lastLineWidth / 2;
y1 = y2 = borderValue;
} else {
x1 = x2 = options.position === 'left' ? me.right : me.left;
x1 += aliasPixel;
x2 += aliasPixel;
y1 = alignPixel(chart, me.top, firstLineWidth) - firstLineWidth / 2;
y2 = alignPixel(chart, me.bottom, lastLineWidth) + lastLineWidth / 2;
x1 = x2 = borderValue;
}

context.lineWidth = axisWidth;
context.strokeStyle = helpers.valueAtIndexOrDefault(gridLines.color, 0);
context.beginPath();
context.moveTo(x1, y1);
context.lineTo(x2, y2);
Expand Down
8 changes: 4 additions & 4 deletions src/core/core.tooltip.js
View file
Expand Up @@ -124,8 +124,8 @@ var positioners = {
}

return {
x: Math.round(x / count),
y: Math.round(y / count)
x: x / count,
y: y / count
};
},

Expand Down Expand Up @@ -619,8 +619,8 @@ var exports = module.exports = Element.extend({
model.footer = me.getFooter(tooltipItems, data);

// Initial positioning and colors
model.x = Math.round(tooltipPosition.x);
model.y = Math.round(tooltipPosition.y);
model.x = tooltipPosition.x;
model.y = tooltipPosition.y;
model.caretPadding = opts.caretPadding;
model.labelColors = labelColors;
model.labelTextColors = labelTextColors;
Expand Down
4 changes: 2 additions & 2 deletions src/scales/scale.category.js
View file
Expand Up @@ -90,7 +90,7 @@ module.exports = function() {
widthOffset += (valueWidth / 2);
}

return me.left + Math.round(widthOffset);
return me.left + widthOffset;
}
var valueHeight = me.height / offsetAmt;
var heightOffset = (valueHeight * (index - me.minIndex));
Expand All @@ -99,7 +99,7 @@ module.exports = function() {
heightOffset += (valueHeight / 2);
}

return me.top + Math.round(heightOffset);
return me.top + heightOffset;
},
getPixelForTick: function(index) {
return this.getPixelForValue(this.ticks[index], index + this.minIndex, null);
Expand Down
4 changes: 2 additions & 2 deletions src/scales/scale.radialLinear.js
View file
Expand Up @@ -477,8 +477,8 @@ module.exports = function(Chart) {
var me = this;
var thisAngle = me.getIndexAngle(index) - (Math.PI / 2);
return {
x: Math.round(Math.cos(thisAngle) * distanceFromCenter) + me.xCenter,
y: Math.round(Math.sin(thisAngle) * distanceFromCenter) + me.yCenter
x: Math.cos(thisAngle) * distanceFromCenter + me.xCenter,
y: Math.sin(thisAngle) * distanceFromCenter + me.yCenter
};
},
getPointPositionForValue: function(index, value) {
Expand Down
Binary file modified test/fixtures/controller.line/point-style.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/controller.radar/point-style.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.scale/tick-drawing.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/core.tooltip/opacity.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/scale.radialLinear/border-dash.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/scale.radialLinear/indexable-gridlines.png
View file
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 3 additions & 3 deletions test/specs/core.scale.tests.js
View file
Expand Up @@ -29,7 +29,7 @@ describe('Core.scale', function() {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: false,
offset: true,
expected: [51.5, 154.5, 256.5, 358.5, 461.5]
expected: [51.5, 153.5, 256.5, 358.5, 460.5]
}, {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: true,
Expand All @@ -39,7 +39,7 @@ describe('Core.scale', function() {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: true,
offset: true,
expected: [0, 103, 205.5, 307.5, 410]
expected: [-0.5, 102.5, 204.5, 307.5, 409.5]
}, {
labels: ['tick1'],
offsetGridLines: false,
Expand Down Expand Up @@ -146,7 +146,7 @@ describe('Core.scale', function() {
chart.draw();

expect(yScale.ctx.getCalls().filter(function(x) {
return x.name === 'moveTo' && x.args[0] === 0;
return x.name === 'moveTo' && x.args[0] === 1;
}).map(function(x) {
return x.args[1];
})).toEqual(test.expected);
Expand Down
6 changes: 3 additions & 3 deletions test/specs/core.tooltip.tests.js
View file
Expand Up @@ -145,7 +145,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.x).toBeCloseToPixel(267);
expect(tooltip._view.y).toBeCloseToPixel(155);
});

Expand Down Expand Up @@ -343,7 +343,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.x).toBeCloseToPixel(267);
expect(tooltip._view.y).toBeCloseToPixel(312);
});

Expand Down Expand Up @@ -576,7 +576,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.x).toBeCloseToPixel(267);
expect(tooltip._view.y).toBeCloseToPixel(155);
});

Expand Down
7 changes: 7 additions & 0 deletions test/specs/global.deprecations.tests.js
View file
Expand Up @@ -24,6 +24,13 @@ describe('Deprecations', function() {
});
});
});

describe('Chart.helpers.aliasPixel', function() {
it('should be defined and a function', function() {
expect(Chart.helpers.aliasPixel).toBeDefined();
expect(typeof Chart.helpers.aliasPixel).toBe('function');
});
});
});

describe('Version 2.7.3', function() {
Expand Down

0 comments on commit 9e7a4cc

Please sign in to comment.