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

Image stroke #2926

Merged
merged 4 commits into from
May 1, 2016
Merged

Image stroke #2926

merged 4 commits into from
May 1, 2016

Conversation

asturur
Copy link
Member

@asturur asturur commented May 1, 2016

I will post screenshot to check i did not broke anything, but looks like that we have some mess in the stroke rendering under certain conditions.

@@ -164,12 +164,12 @@
* @param {CanvasRenderingContext2D} ctx Context to render on
*/
_stroke: function(ctx) {
ctx.save();
this._setStrokeStyles(ctx);
Copy link
Member Author

Choose a reason for hiding this comment

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

_stroke is called by the _render function, _setStrokeStyle has been already done.

ctx.lineTo(w, -h);
ctx.lineTo(w, h);
ctx.lineTo(-w, h);
ctx.lineTo(-w, -h);
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an un-optimization, but in my opinion it simplify the rendering flow for the dev.

@asturur
Copy link
Member Author

asturur commented May 1, 2016

The point is that every shape do its path in the _render function and then we call .fill(); and .stroke();

Image was the only one that was doing differently, it was drawing the eventual border inside the renderStroke with strokeRect function and then not doing the ctx.stroke();

I think that getting 4 lineTo instead of strokeRect is slower, but we get a cleaner pipeline.

Oh, also we had a very very rare bug:

If someone would stroke with dash Array and gradient with gradientTransform, well, gradientTransform was not applied... no one notice in 2 years so not so popular combination

@asturur
Copy link
Member Author

asturur commented May 1, 2016

Also look this code:

// add red rectangle
var rect = new fabric.Rect({
  width: 50,
  height: 50,
  left: 50,
  top: 50,
});
rect.setGradient('fill', { type:'linear',
                        x1: 0, 
                        y1: 0, 
                        x2: 0, 
                        y2: rect.height,
                        colorStops: { 0 : 'rgba(0,255,0,1)', 1 : 'rgba(0,0,255,1)'} });

// add line
var line = new fabric.Line([10,10,10,60], {strokeWidth: 50});
line.setGradient('stroke', { type:'linear',
                        x1: 0, 
                        y1: 0, 
                        x2: 0, 
                        y2: rect.height,
                        colorStops: { 0 : 'rgba(0,255,0,1)', 1 : 'rgba(0,0,255,1)'} });
canvas.add(rect);
canvas.add(line);

look results:

image

left rectangle, right a chubby line. We are not translating the gradient for stroke ( and so patterns!!!)

@asturur
Copy link
Member Author

asturur commented May 1, 2016

new SVG IMPORT
image

real SVG

image

current SVG IMPORT

image

@asturur asturur merged commit 6fd1f8a into fabricjs:master May 1, 2016
@asturur asturur deleted the imageStroke branch May 2, 2016 06:33
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.

1 participant