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 closePath by using PDF command #3304

Merged
merged 9 commits into from
Nov 3, 2021
Merged

Fix closePath by using PDF command #3304

merged 9 commits into from
Nov 3, 2021

Conversation

KurtGokhan
Copy link
Contributor

For context2d.closePath, PDF command h can be used as seen in PDF spec

Fixes: #3293

@Uzlopak
Copy link
Collaborator

Uzlopak commented Oct 25, 2021

When I modified that code long time ago, I realized that it is not preferable to always close the path, e.g. on page breaks...
Should be careful while doing this

@KurtGokhan
Copy link
Contributor Author

I will convert this to draft. This does not to solve the moveTo + arc fill problem. I will also investigate multiple pages.

@KurtGokhan KurtGokhan marked this pull request as draft October 25, 2021 23:07
@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Oct 25, 2021

Fixed the problem mentioned in the issue and added test.

Some HTML references were changed. Those were using rect call and manually closing the path. But now it is using closePath command. There are some subpixel differences, but the new method should be the correct one.

@Uzlopak what is the worst that could happen? Will it break the pdf? How should I test it? The old code was also closing path (using h) for arcs. But it was doing it in a wrong place.

@KurtGokhan KurtGokhan marked this pull request as ready for review October 25, 2021 23:53
@Uzlopak
Copy link
Collaborator

Uzlopak commented Oct 26, 2021

Hi Kurt,

I remember when I refactored context2d API command by API command and it had issues with the closePath. It had this bug because I could not figure out how to solve it properly.

Currently I am just the nagging old maintainer, who just gives hints for potential issues :). HackbrettXXX takes care of the project, so it is on him if there is a bug.

Worst what can happen is that the arcs are broken.

@KurtGokhan
Copy link
Contributor Author

@HackbrettXXX any luck with this?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Oct 28, 2021

@KurtGokhan your Branch is now out of date. Please rebase or merge latest master into your branch.

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

I think it looks mostly good.

Could you add a test case where there is an implicit lineTo from the current position to the arc start? E.g.

ctx.beginPath()
ctx.moveTo(100,100)
ctx.arc(100,100,30,0,2)
ctx.fill()
ctx.stroke()

src/modules/context2d.js Outdated Show resolved Hide resolved
@KurtGokhan
Copy link
Contributor Author

Done. Seemed strange at first but apparently that is how it is supposed to look.

Canvas:

image

PDF:

image

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

Alright, looks good. Thanks for the PR!

@HackbrettXXX HackbrettXXX merged commit 8094918 into parallax:master Nov 3, 2021
@KurtGokhan KurtGokhan deleted the closepath-fix branch November 3, 2021 15:39
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.

context2D closePath on arc closes path to centre x point and not to drawn line start
3 participants