Fix gradient parsing #5836
Fix gradient parsing #5836
Conversation
|
I will offer a code review momentarily, but I strongly suggest you get another code review from someone who understands the internals of Fabric (because I don't). |
| * @type Array[Number] | ||
| * @default null | ||
| */ | ||
| gradientTransform: null, |
There was a problem hiding this comment.
- painting
- behaves differently how? Describe the layout and the members of this matrix, please, enough to describe them for people coming along in the future.
/**
* | x1 x2 0 0
* | ... |
* ...
* x1: a description of x1
* ...
*/Yes, I know, JavaDoc is probably insufficient for this (I've never seen a 2-D array defined in JavaDoc in detail), but some documentation of the members is better than none.
Even better would be to make this an instance of an explicit unit-testable matrix class. I realize that would be a significant refactor, but I'd be very surprised if you didn't have a need for it by now elsewhere in the code. I do see there's a fabric.ColorMatrix class, but that appears to be something else.
src/gradient.class.js
Outdated
|
|
||
| /** | ||
| * coordinates units for coords. | ||
| * If `pixels`, the number of cords are in the same unit of width/ height. |
src/gradient.class.js
Outdated
| * @param {Number} [options.coords.x2] | ||
| * @param {Number} [options.coords.y2] | ||
| * @param {Number} [options.coords.r1] | ||
| * @param {Number} [options.coords.r2] |
There was a problem hiding this comment.
options.coords should probably be next to options.coords.x1. Move colorstops up one line?
src/util/misc.js
Outdated
| @@ -165,9 +165,15 @@ | |||
| /** | |||
| * Returns coordinates of points's bounding rectangle (left, top, width, height) | |||
| * @param {Array} points 4 points array | |||
| * @param {Array} [transform] 6 number trasnform matrix | |||
There was a problem hiding this comment.
transform (typo on the second use). Also mark it optional?
Is this array 1x6, 6x1, 2x3, 3x2? :-)
There was a problem hiding this comment.
All the matrices are 2 rows, 3 columns and are the standard geometric affine trasformations you use in 2d graphic. the ones you can pass to ctx.transform
src/util/misc.js
Outdated
| @@ -658,7 +664,7 @@ | |||
| }, | |||
|
|
|||
| /** | |||
| * Decomposes standard 2x2 matrix into transform componentes | |||
| * Decomposes standard 2x3 matrix into transform componentes | |||
| @@ -307,7 +344,7 @@ | |||
| * @see http://www.w3.org/TR/SVG/pservers.html#LinearGradientElement | |||
| * @see http://www.w3.org/TR/SVG/pservers.html#RadialGradientElement | |||
| */ | |||
| fromElement: function(el, instance, opacityAttr) { | |||
| fromElement: function(el, instance, opacityAttr, svgOptions) { | |||
There was a problem hiding this comment.
You might as well add a javadoc line for svgOptions.
There was a problem hiding this comment.
sure, i m not done yet, svg export is now broken and needs to be adequate to the new logic.
src/util/misc.js
Outdated
| if (options.skewY) { | ||
| scaleMatrix = fabric.util.multiplyTransformMatrices( | ||
| scaleMatrix, | ||
| [1, Math.tan(fabric.util.degreesToRadians(options.skewY)), 0, 1], |
There was a problem hiding this comment.
Just a little consistency question here. Your code style uses fabric.util.cos, fabric.util.sin, but then you abruptly switch to Math.tan. Maybe you should just define fabric.util.tan as well.
There was a problem hiding this comment.
Math.tan is good enough to be used as it is.
cos and sin never return 0 or 1 when they are at multiples of 90 degrees. That makes number go crazy, like 0.0000009999 instead of 0. That is why we have util.cos and util.sin, to return 0 and 1 when needed.
src/util/misc.js
Outdated
| * @param {Number} [options.translateY] | ||
| * @return {Array[Number]} transform matrix | ||
| */ | ||
| componeMatrix: function(options) { |
There was a problem hiding this comment.
i think is 'compose'.
I used it in a precedent implementation of the parsing, i do not need to keep it.
i ll remove it.
|
While you're at it, see if this fixes #3665 as well. |
Added gradientUnit to the gradient class Rewrote the SVG import and export Added a fallback with patterns for gradients that cannot be represented easily applied to stroke
close #5810
close #3665
This PR rewrite fabricJS gradient parsing and fixes those use cases:
It also enable the following feature:
gradientUnitsthat can be pixels or percentage and allows you to express the gradient properties in percentage relative to the object utilizing it.