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

Properly render gradients when strokeScaling = false #40

Merged

Conversation

adroitwhiz
Copy link

Description

This was a fun bug 🙂 I like paper.js 🙂

See paperjs#1822 for more info. This is necessary to make bitmap shape tools work properly with gradients, otherwise this will happen:
Peek 2020-06-08 14-30

Related issues

Checklist

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the JSHint rules (npm run jshint passes)

@adroitwhiz adroitwhiz requested a review from fsih June 9, 2020 01:10
@adroitwhiz adroitwhiz changed the title Fix gradient stroke scaling Properly render gradients when strokeScaling = false Jun 9, 2020
toCanvasStyle: function(ctx, matrix) {
if (this._canvasStyle)
toCanvasStyle: function(ctx, matrix, strokeMatrix) {
// strokeMatrix can change without triggering _changed here, so we
Copy link

Choose a reason for hiding this comment

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

Is this fixable by correctly triggering the _changed event when strokeMatrix changes?
It seems brittle to keep track of exceptions around caching

Copy link
Author

@adroitwhiz adroitwhiz Jun 16, 2020

Choose a reason for hiding this comment

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

I could manually trigger _changed in this block but that seems more fragile to me-- the color code would no longer be responsible for ensuring its own caching behavior.

Copy link
Author

@adroitwhiz adroitwhiz Jun 16, 2020

Choose a reason for hiding this comment

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

And come to think of it that would suffer from the same problem as commented below-- if strokeMatrix stopped being passed in, _changed would stop being called and the gradient would retain its previous incorrect transform

Copy link

Choose a reason for hiding this comment

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

I'm working on understanding the paper.js code, and it looks like the Color class already doesn't keep track of its own matrix; it's relying on the parent item to call _transform on the color any time the item's matrix changes, otherwise if you call toCanvasStyle twice in a row with different matrices you'll get the wrong result.

What if the color stored what was in the matrix and strokeMatrix, and used that to determine whether its cached style was still valid?

Are we assuming that any time a strokeMatrix is passed in, it implies that we're coloring a stroke?

Copy link
Author

Choose a reason for hiding this comment

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

strokeMatrix, confusingly, actually refers to (IIRC) the matrix by which path points are transformed to obtain the view transformation. It's applied to the fill and stroke when strokeScaling is false.

The problem is that it would also need to be invalidated whenever the view transform changes.

@fsih fsih assigned adroitwhiz and unassigned fsih Jun 16, 2020
@adroitwhiz
Copy link
Author

@fsih Did we determine that this is OK to merge?

Copy link

@fsih fsih left a comment

Choose a reason for hiding this comment

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

Oops, yes I think this looks good to merge!

@adroitwhiz adroitwhiz merged commit 16d5ff0 into scratchfoundation:develop Jul 28, 2020
@jkozniewski
Copy link

jkozniewski commented Jan 12, 2022

So there is a good working pr from 2020 and still not fixed in 2022 ?
Any chance for it to be actually merged into main paperjs branch anytime soon ?

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