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.

Sign up for GitHub

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

Jump to bottom

Refactor helpers.canvas.drawPoint() #5623

Merged
merged 2 commits into from Jul 29, 2018
Merged

Refactor helpers.canvas.drawPoint() #5623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1596d58
Refactor helpers.canvas.drawPoint()
nagix Jul 8, 2018
c53bd3c
Move fill() call to the end and add moveTo() call to the end of round…
nagix Jul 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 5 additions & 24 deletions src/helpers/helpers.canvas.js
View file
Open in desktop
Expand Up @@ -41,6 +41,8 @@ var exports = module.exports = {
ctx.arcTo(x, y + height, x, y + height - r, r);
ctx.lineTo(x, y + r);
ctx.arcTo(x, y, x + r, y, r);
ctx.closePath();
Copy link
Member

Choose a reason for hiding this comment

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

It is preferable that helpers.canvas.roundedRect() include ctx.closePath() at the end because CanvasRenderingContext2D.rect() closes the subpath [link]

It's also written that rect() "must then create a new subpath with the point (x, y) as the only point in the subpath". Shoud we add ctx.move(x, y) right after closePath() or do you think it's not application to a rounded rectangle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctx.move(x, y) after closePath() makes sense. I will update the code.

ctx.moveTo(x, y);
} else {
ctx.rect(x, y, width, height);
}
Expand All @@ -65,77 +67,61 @@ var exports = module.exports = {
ctx.save();
ctx.translate(x, y);
ctx.rotate(rotation * Math.PI / 180);
ctx.beginPath();
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need closePath() before stroke()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need closePath() before stroke(). [link]

In the image for #5629, you see some lines are darker than others. This is because the last line in a path is rendered twice as a result of closing the subpath. This can be avoided by not calling closePath().
image


switch (style) {
// Default includes circle
default:
ctx.beginPath();
ctx.arc(0, 0, radius, 0, Math.PI * 2);
ctx.closePath();
ctx.fill();
break;
case 'triangle':
ctx.beginPath();
edgeLength = 3 * radius / Math.sqrt(3);
height = edgeLength * Math.sqrt(3) / 2;
ctx.moveTo(-edgeLength / 2, height / 3);
ctx.lineTo(edgeLength / 2, height / 3);
ctx.lineTo(0, -2 * height / 3);
ctx.closePath();
ctx.fill();
break;
case 'rect':
size = 1 / Math.SQRT2 * radius;
ctx.beginPath();
ctx.fillRect(-size, -size, 2 * size, 2 * size);
ctx.strokeRect(-size, -size, 2 * size, 2 * size);
ctx.rect(-size, -size, 2 * size, 2 * size);
break;
case 'rectRounded':
var offset = radius / Math.SQRT2;
var leftX = -offset;
var topY = -offset;
var sideSize = Math.SQRT2 * radius;
ctx.beginPath();

// NOTE(SB) the rounded rect implementation changed to use `arcTo`
// instead of `quadraticCurveTo` since it generates better results
// when rect is almost a circle. 0.425 (instead of 0.5) produces
// results visually closer to the previous impl.
this.roundedRect(ctx, leftX, topY, sideSize, sideSize, radius * 0.425);

ctx.closePath();
ctx.fill();
break;
case 'rectRot':
size = 1 / Math.SQRT2 * radius;
ctx.beginPath();
ctx.moveTo(-size, 0);
ctx.lineTo(0, size);
ctx.lineTo(size, 0);
ctx.lineTo(0, -size);
ctx.closePath();
ctx.fill();
break;
case 'cross':
ctx.beginPath();
ctx.moveTo(0, radius);
ctx.lineTo(0, -radius);
ctx.moveTo(-radius, 0);
ctx.lineTo(radius, 0);
ctx.closePath();
break;
case 'crossRot':
ctx.beginPath();
xOffset = Math.cos(Math.PI / 4) * radius;
yOffset = Math.sin(Math.PI / 4) * radius;
ctx.moveTo(-xOffset, -yOffset);
ctx.lineTo(xOffset, yOffset);
ctx.moveTo(-xOffset, yOffset);
ctx.lineTo(xOffset, -yOffset);
ctx.closePath();
break;
case 'star':
ctx.beginPath();
ctx.moveTo(0, radius);
ctx.lineTo(0, -radius);
ctx.moveTo(-radius, 0);
Expand All @@ -146,22 +132,18 @@ var exports = module.exports = {
ctx.lineTo(xOffset, yOffset);
ctx.moveTo(-xOffset, yOffset);
ctx.lineTo(xOffset, -yOffset);
ctx.closePath();
break;
case 'line':
ctx.beginPath();
ctx.moveTo(-radius, 0);
ctx.lineTo(radius, 0);
ctx.closePath();
break;
case 'dash':
ctx.beginPath();
ctx.moveTo(0, 0);
ctx.lineTo(radius, 0);
ctx.closePath();
break;
}

ctx.fill();
ctx.stroke();
ctx.restore();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any impact (in the calling code) to not call ctx.closePath(); anymore before returning?

},
Expand Down Expand Up @@ -224,5 +206,4 @@ helpers.clear = exports.clear;
helpers.drawRoundedRectangle = function(ctx) {
ctx.beginPath();
exports.roundedRect.apply(exports, arguments);
ctx.closePath();
};
26 changes: 13 additions & 13 deletions test/specs/element.point.tests.js
View file
Open in desktop
Expand Up @@ -232,11 +232,11 @@ describe('Point element tests', function() {
name: 'beginPath',
args: []
}, {
name: 'fillRect',
name: 'rect',
args: [0 - 1 / Math.SQRT2 * 2, 0 - 1 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2]
}, {
name: 'strokeRect',
args: [0 - 1 / Math.SQRT2 * 2, 0 - 1 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2, 2 / Math.SQRT2 * 2]
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down Expand Up @@ -358,8 +358,8 @@ describe('Point element tests', function() {
name: 'lineTo',
args: [2, 0],
}, {
name: 'closePath',
args: [],
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down Expand Up @@ -406,8 +406,8 @@ describe('Point element tests', function() {
name: 'lineTo',
args: [0 + Math.cos(Math.PI / 4) * 2, 0 - Math.sin(Math.PI / 4) * 2],
}, {
name: 'closePath',
args: [],
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down Expand Up @@ -466,8 +466,8 @@ describe('Point element tests', function() {
name: 'lineTo',
args: [0 + Math.cos(Math.PI / 4) * 2, 0 - Math.sin(Math.PI / 4) * 2],
}, {
name: 'closePath',
args: [],
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down Expand Up @@ -508,8 +508,8 @@ describe('Point element tests', function() {
name: 'lineTo',
args: [2, 0],
}, {
name: 'closePath',
args: [],
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down Expand Up @@ -550,8 +550,8 @@ describe('Point element tests', function() {
name: 'lineTo',
args: [2, 0],
}, {
name: 'closePath',
args: [],
name: 'fill',
args: []
}, {
name: 'stroke',
args: []
Expand Down
1 change: 0 additions & 1 deletion test/specs/global.deprecations.tests.js
View file
Open in desktop
Expand Up @@ -113,7 +113,6 @@ describe('Deprecations', function() {

var calls = ctx.getCalls();
expect(calls[0]).toEqual({name: 'beginPath', args: []});
expect(calls[calls.length - 1]).toEqual({name: 'closePath', args: []});
expect(Chart.helpers.canvas.roundedRect).toHaveBeenCalledWith(ctx, 10, 20, 30, 40, 5);
});
});
Expand Down
4 changes: 3 additions & 1 deletion test/specs/helpers.canvas.tests.js
View file
Open in desktop
Expand Up @@ -36,7 +36,9 @@ describe('Chart.helpers.canvas', function() {
{name: 'lineTo', args: [15, 60]},
{name: 'arcTo', args: [10, 60, 10, 55, 5]},
{name: 'lineTo', args: [10, 25]},
{name: 'arcTo', args: [10, 20, 15, 20, 5]}
{name: 'arcTo', args: [10, 20, 15, 20, 5]},
{name: 'closePath', args: []},
{name: 'moveTo', args: [10, 20]}
]);
});
it('should optimize path if radius is 0', function() {
Expand Down