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

Refactor helpers.canvas.drawPoint() #5623

Merged
merged 2 commits into from
Jul 29, 2018
Merged

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Jul 8, 2018

This PR makes the following changes:

  • Bring ctx.beginPath() before switch
  • Replace ctx.fillRect()/ctx.strokeRect() with ctx.rect()/ctx.fill() to make it consistent with the other styles
  • It is preferable that helpers.canvas.roundedRect() include ctx.closePath() at the end because CanvasRenderingContext2D.rect() closes the subpath [link]
  • Get rid of ctx.closePath() for cross, crossRot, star, line and dash because these have no closed path shape, and it should be avoided that ctx.closePath() makes a round-trip path [link]
  • The related tests are fixed

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.

@nagix Do you think we can also move ctx.fill() calls to the end, just before ctx.stroke? I don't know if it makes a difference to fill all paths, even cross, dash, line, etc. I'm asking because it could be interesting to move the path creation in a separate method (e.g. helpers.canvas.symbol()) and only fill and stroke when needed. It would fix #5180 without relying on a transparent stroke but simply skip ctx.stroke() if borderWidth is 0.

@@ -41,6 +41,7 @@ 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.

@@ -65,17 +66,16 @@ 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

@nagix
Copy link
Contributor Author

nagix commented Jul 10, 2018

I think we can move ctx.fill() calls to the end because the shapes like cross, dash and line don't have closed paths, and the fill call shouldn't affect the results. I will try it first, then update the code if there is no problem.

@nagix
Copy link
Contributor Author

nagix commented Jul 10, 2018

Confirmed that there is no problem with the changes above. It's ready to merge.

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?

@nagix
Copy link
Contributor Author

nagix commented Jul 11, 2018

A path itself doesn't have an open/close state, but a subpath does. What ctx.closePath() does are:

  1. Mark the last subpath as closed
  2. Create a new subpath whose first point is the same as the previous subpath's first point
  3. Add this new subpath to the path

The last subpath is always open regardless of whether ctx.closePath() is called or not. This makes no impact on the following rendering unless the code intends to append points to the last subpath (and we don't have such code).

@nagix
Copy link
Contributor Author

nagix commented Jul 11, 2018

If we make sure that the path has the last subpath with the point (x, y) like ctx.rect() and helpers.canvas.roundedRect(), we can add ctx.moveTo(x, y) (which also creates a new subpath leaving the previous subpath open) to the end instead of ctx.closePath().

@simonbrunel simonbrunel merged commit 493eaa8 into chartjs:master Jul 29, 2018
@nagix nagix deleted the issue-5623 branch October 29, 2018 04:32
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Bring ctx.beginPath() before switch and replace ctx.fillRect()/ctx.strokeRect() with ctx.rect()/ctx.fill() to make it consistent with the other styles. It is also preferable that helpers.canvas.roundedRect() include ctx.closePath() at the end because CanvasRenderingContext2D.rect() closes the subpath.

Get rid of ctx.closePath() for cross, crossRot, star, line and dash because these have no closed path shape, and it should be avoided that ctx.closePath() makes a round-trip path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants