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

Add setTransform method to CanvasPattern #1623

Merged
merged 11 commits into from
Jul 15, 2021

Conversation

samizdatco
Copy link
Contributor

As noted in #1411, CanvasPattern objects should accept a transformation matrix to be applied before the canvas's CTM. The one wrinkle is that cairo's conception of a pattern matrix is that it transforms from user- to pattern-space while canvas's is the reverse. As a result, the DOMMatrix passed to the method is inverted before being applied to the cairo pattern.

  • Have you updated CHANGELOG.md?

@asturur
Copy link
Contributor

asturur commented Jul 19, 2020

I would suggest you add a small visual example to the list of examples available here:

https://github.com/Automattic/node-canvas/blob/master/test/public/tests.js#L433

they are not official tests, but helps to compare how things go compared to browsers.

@@ -389,6 +389,7 @@ Context2d::fill(bool preserve) {
cairo_set_source(_context, new_pattern);
cairo_pattern_destroy(new_pattern);
} else {
cairo_pattern_set_filter(state->fillPattern, state->patternQuality);
Copy link
Contributor

Choose a reason for hiding this comment

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

patterQuality isn't really a canvas feature.
I think, but i may be wrong, it is used to simulate imageSmoothingEnabled property.

In canvas pattern quality is always best, there is no other option.

In case this change is ok, then it should be applied also to the stroke part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now applied to the stroke as well

@samizdatco
Copy link
Contributor Author

image

@SLKnutson
Copy link

Is there any timeline on getting this merged? This would be very nice to have.

@kmcclellan
Copy link

@asturur, @zbjornson What's preventing this from being merged? Anything we can help with?

Copy link
Collaborator

@LinusU LinusU left a comment

Choose a reason for hiding this comment

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

Looks good to me, sorry for the delay in getting this reviewed!

@LinusU LinusU merged commit d7ebfce into Automattic:master Jul 15, 2021
@wxfred
Copy link

wxfred commented Jul 20, 2021

@LinusU Need upgrading on npm, pls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants