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

Properly render gradients when strokeScaling = false #40

Merged
merged 2 commits into from
Jul 28, 2020
Merged

Properly render gradients when strokeScaling = false #40

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
f6212cd
Properly render gradients when strokeScaling=false
adroitwhiz Jun 9, 2020
b25ce19
Add test for strokeScaling gradients
adroitwhiz Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/item/Item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4279,16 +4279,16 @@ new function() { // Injection scope for hit-test functions shared with project
* Not defined in Path as it is required by other classes too,
* e.g. PointText.
*/
_setStyles: function(ctx, param, viewMatrix) {
_setStyles: function(ctx, param, viewMatrix, strokeMatrix) {
// We can access internal properties since we're only using this on
// items without children, where styles would be merged.
var style = this._style,
matrix = this._matrix;
if (style.hasFill()) {
ctx.fillStyle = style.getFillColor().toCanvasStyle(ctx, matrix);
ctx.fillStyle = style.getFillColor().toCanvasStyle(ctx, matrix, strokeMatrix);
}
if (style.hasStroke()) {
ctx.strokeStyle = style.getStrokeColor().toCanvasStyle(ctx, matrix);
ctx.strokeStyle = style.getStrokeColor().toCanvasStyle(ctx, matrix, strokeMatrix);
ctx.lineWidth = style.getStrokeWidth();
var strokeJoin = style.getStrokeJoin(),
strokeCap = style.getStrokeCap(),
Expand Down
2 changes: 1 addition & 1 deletion src/item/Shape.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ var Shape = Item.extend(/** @lends Shape# */{
ctx.closePath();
}
if (!dontPaint && (hasFill || hasStroke)) {
this._setStyles(ctx, param, viewMatrix);
this._setStyles(ctx, param, viewMatrix, strokeMatrix);
if (hasFill) {
ctx.fill(style.getFillRule());
ctx.shadowColor = 'rgba(0,0,0,0)';
Expand Down
2 changes: 1 addition & 1 deletion src/path/CompoundPath.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ var CompoundPath = PathItem.extend(/** @lends CompoundPath# */{
children[i].draw(ctx, param, strokeMatrix);

if (!param.clip) {
this._setStyles(ctx, param, viewMatrix);
this._setStyles(ctx, param, viewMatrix, strokeMatrix);
var style = this._style;
if (style.hasFill()) {
ctx.fill(style.getFillRule());
Expand Down
2 changes: 1 addition & 1 deletion src/path/Path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2321,7 +2321,7 @@ new function() { // Scope for drawing
if (!dontPaint && (hasFill || hasStroke)) {
// If the path is part of a compound path or doesn't have a fill
// or stroke, there is no need to continue.
this._setStyles(ctx, param, viewMatrix);
this._setStyles(ctx, param, viewMatrix, strokeMatrix);
if (hasFill) {
ctx.fill(style.getFillRule());
// If shadowColor is defined, clear it after fill, so it
Expand Down
20 changes: 17 additions & 3 deletions src/style/Color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,8 +899,11 @@ var Color = Base.extend(new function() {
+ components.join(',') + ')';
},

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 •
edited
Loading

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 •
edited
Loading

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.

// can't use a cached gradient here.
var strokeMayChange = this._type === 'gradient' && strokeMatrix;
if (this._canvasStyle && !strokeMayChange)
return this._canvasStyle;
// Normal colors are simply represented by their CSS string.
if (this._type !== 'gradient')
Expand All @@ -923,6 +926,12 @@ var Color = Base.extend(new function() {
if (highlight)
highlight = inverse._transformPoint(highlight);
}
if (strokeMatrix) {
origin = strokeMatrix._transformPoint(origin);
destination = strokeMatrix._transformPoint(destination);
if (highlight)
highlight = strokeMatrix._transformPoint(highlight);
}
if (gradient._radial) {
var radius = destination.getDistance(origin);
if (highlight) {
Expand All @@ -948,7 +957,12 @@ var Color = Base.extend(new function() {
offset == null ? i / (l - 1) : offset,
stop._color.toCanvasStyle());
}
return this._canvasStyle = canvasGradient;
// Don't store gradients that may change in the cache.
// If we cached a gradient that was transformed by strokeMatrix
// then set strokeScaling to true, then the transformed gradient
// could get "stuck" in the cache.
if (!strokeMayChange) this._canvasStyle = canvasGradient;
return canvasGradient;
},

/**
Expand Down
35 changes: 35 additions & 0 deletions test/tests/Color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,41 @@ test('Gradients with applyMatrix', function() {
comparePixels(path, shape);
});

test('Gradients with strokeScaling: false', function() {
var topLeft = [100, 100];
var bottomRight = [400, 400];
var gradientColor = {
gradient: {
stops: ['yellow', 'red', 'blue']
},
origin: topLeft,
destination: bottomRight
}

var path = new Shape.Rectangle({
topLeft: topLeft,
bottomRight: bottomRight,
fillColor: gradientColor,
strokeScaling: true
});

var shape = new Shape.Rectangle({
topLeft: topLeft,
bottomRight: bottomRight,
fillColor: gradientColor,
strokeScaling: false
});

comparePixels(path, shape);

path.scale(2);
path.rotate(45);
shape.scale(2);
shape.rotate(45);

comparePixels(path, shape);
})

test('Modifying group.strokeColor for multiple children', function() {
var item = new Group(new Path(), new Path());
item.strokeColor = 'red';
Expand Down