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.alignLinePixel` 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 3, 2018
1 parent 3cb2d70 commit 6f90b59
Show file tree
Hide file tree
Showing 14 changed files with 135 additions and 94 deletions.
16 changes: 16 additions & 0 deletions src/core/core.helpers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,22 @@ module.exports = function() {
helpers.aliasPixel = function(pixelWidth) {
return (pixelWidth % 2 === 0) ? 0 : 0.5;
};

/**
* Returns the aligned line pixel value to avoid anti-aliasing blur
* @param {Number} linePixel - A line pixel value.
* @param {Number} lineWidth - A line width.
* @param {Boolean} roundDown - if true, the line pixel value is rounded down when its fractional portion is equal to 0.5.
* @returns {Number} The aligned line pixel value.
*/
helpers.alignLinePixel = function(linePixel, lineWidth, roundDown) {
if (Math.floor(lineWidth) !== lineWidth) {
return linePixel;
}
// Adding or subtracting a small value to ensure the number is rounded up or down
return Math.round(linePixel - lineWidth / 2 + (roundDown ? -1e-13 : 1e-13)) + lineWidth / 2;
};

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
148 changes: 81 additions & 67 deletions src/core/core.scale.js
View file
Original file line number Diff line number Diff line change
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 @@ -689,15 +689,19 @@ module.exports = Element.extend({
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 +712,28 @@ 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 = helpers.valueAtIndexOrDefault(gridLines.lineWidth, 0, 0);
var alignedLeft = helpers.alignLinePixel(me.left, axisWidth, position === 'right');
var alignedRight = helpers.alignLinePixel(me.right, axisWidth);
var alignedTop = helpers.alignLinePixel(me.top, axisWidth, position === 'bottom');
var alignedBottom = helpers.alignLinePixel(me.bottom, axisWidth);

var xTickStart, xTickEnd, yTickStart, yTickEnd;
if (isHorizontal) {
if (position === 'top') {
yTickStart = me.bottom - tl;
yTickEnd = alignedBottom - axisWidth / 2;
} else {
yTickStart = alignedTop + axisWidth / 2;
yTickEnd = me.top + tl;
}
} else if (position === 'left') {
xTickStart = me.right - tl;
xTickEnd = alignedRight - axisWidth / 2;
} else {
xTickStart = alignedLeft + axisWidth / 2;
xTickEnd = me.left + tl;
}

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

Expand All @@ -738,66 +759,60 @@ 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;
tx1 = tx2 = x1 = x2 = helpers.alignLinePixel(lineValue, lineWidth, lineValue === chartArea.right);
ty1 = yTickStart;
ty2 = yTickEnd;
y1 = chartArea.top;
y2 = chartArea.bottom + axisWidth;
} else {
var isLeft = options.position === 'left';
var labelXOffset;
y2 = chartArea.bottom;

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 = helpers.alignLinePixel(y1, axisWidth) + axisWidth / 2;
textBaseline = !isRotated ? 'bottom' : 'middle';
textAlign = !isRotated ? 'center' : 'left';
labelY = me.bottom - labelYOffset;
} else {
textAlign = isLeft ? 'right' : 'left';
labelXOffset = tl + tickPadding;
y2 = helpers.alignLinePixel(y2, axisWidth, true) - 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 = xTickStart;
tx2 = xTickEnd;
x1 = chartArea.left;
x2 = chartArea.right + axisWidth;
ty1 = ty2 = y1 = y2 = yLineValue;
x2 = chartArea.right;
ty1 = ty2 = y1 = y2 = helpers.alignLinePixel(lineValue, lineWidth, lineValue === chartArea.bottom);

labelY = me.getPixelForTick(index) + labelOffset;

if (position === 'left') {
x1 = helpers.alignLinePixel(x1, axisWidth) + axisWidth / 2;
textAlign = isMirrored ? 'left' : 'right';
labelX = me.right - labelXOffset;
} else {
x2 = helpers.alignLinePixel(x2, axisWidth, true) - axisWidth / 2;
textAlign = isMirrored ? 'right' : 'left';
labelX = me.left + labelXOffset;
}
}

itemsToDraw.push({
Expand Down Expand Up @@ -867,7 +882,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 +906,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 @@ -916,28 +931,27 @@ module.exports = Element.extend({

if (gridLines.drawBorder) {
// 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 aliasPixel = helpers.aliasPixel(context.lineWidth);
var firstLineWidth = axisWidth;
var lastLineWidth = helpers.valueAtIndexOrDefault(gridLines.lineWidth, ticks.length - 1, 0);
var x1 = helpers.alignLinePixel(me.left, firstLineWidth) - firstLineWidth / 2;
var x2 = helpers.alignLinePixel(me.right, lastLineWidth, true) + lastLineWidth / 2;
var y1 = helpers.alignLinePixel(me.top, firstLineWidth) - firstLineWidth / 2;
var y2 = helpers.alignLinePixel(me.bottom, lastLineWidth, true) + lastLineWidth / 2;

if (isHorizontal) {
y1 = y2 = options.position === 'top' ? me.bottom : me.top;
y1 += aliasPixel;
y2 += aliasPixel;
y1 = y2 = position === 'top' ? alignedBottom : alignedTop;
} else {
x1 = x2 = options.position === 'left' ? me.right : me.left;
x1 += aliasPixel;
x2 += aliasPixel;
x1 = x2 = position === 'left' ? alignedRight : alignedLeft;
}

context.beginPath();
context.moveTo(x1, y1);
context.lineTo(x2, y2);
context.stroke();
if (axisWidth) {
context.lineWidth = axisWidth;
context.strokeStyle = helpers.valueAtIndexOrDefault(gridLines.color, 0);
context.beginPath();
context.moveTo(x1, y1);
context.lineTo(x2, y2);
context.stroke();
}
}
}
});
4 changes: 2 additions & 2 deletions src/scales/scale.category.js
View file
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading
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
Loading
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
Loading
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
Loading
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions test/specs/core.helpers.tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,17 @@ describe('Core helper tests', function() {
});
});

it('should get the aligned line pixel value', function() {
expect(helpers.alignLinePixel(0, 0.5)).toEqual(0);
expect(helpers.alignLinePixel(0, 1)).toEqual(0.5);
expect(helpers.alignLinePixel(0.5, 1)).toEqual(0.5);
expect(helpers.alignLinePixel(0, 2)).toEqual(0);
expect(helpers.alignLinePixel(1, 0.5, true)).toEqual(1);
expect(helpers.alignLinePixel(1, 1, true)).toEqual(0.5);
expect(helpers.alignLinePixel(1.5, 1, true)).toEqual(1.5);
expect(helpers.alignLinePixel(1, 2, true)).toEqual(1);
});

it('should spline curves', function() {
expect(helpers.splineCurve({
x: 0,
Expand Down
8 changes: 4 additions & 4 deletions test/specs/core.scale.tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ describe('Core.scale', function() {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: false,
offset: false,
expected: [0.5, 128.5, 256.5, 384.5, 512.5]
expected: [0.5, 128.5, 256.5, 384.5, 511.5]
}, {
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
Loading

0 comments on commit 6f90b59

Please sign in to comment.