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

Fix gradient parsing #5836

Merged
merged 19 commits into from
Aug 12, 2019
Merged

Fix gradient parsing #5836

merged 19 commits into from
Aug 12, 2019

Conversation

asturur
Copy link
Member

@asturur asturur commented Aug 10, 2019

close #5810
close #3665

This PR rewrite fabricJS gradient parsing and fixes those use cases:

  • better support for gradient transform
  • better support for ObjectBoundingBox gradient units
  • better support for percentage coords on UserSpaceOnUse ( use the svg viewbox as reference )

It also enable the following feature:

  • now gradients have a property gradientUnits that can be pixels or percentage and allows you to express the gradient properties in percentage relative to the object utilizing it.

@ajvincent
Copy link
Contributor

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,
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. painting
  2. 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.


/**
* coordinates units for coords.
* If `pixels`, the number of cords are in the same unit of width/ height.
Copy link
Contributor

Choose a reason for hiding this comment

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

coords

* @param {Number} [options.coords.x2]
* @param {Number} [options.coords.y2]
* @param {Number} [options.coords.r1]
* @param {Number} [options.coords.r2]
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

transform (typo on the second use). Also mark it optional?

Is this array 1x6, 6x1, 2x3, 3x2? :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

components

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You might as well add a javadoc line for svgOptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

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],
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

componentMatrix?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@ajvincent
Copy link
Contributor

While you're at it, see if this fixes #3665 as well.

@asturur asturur merged commit 620e956 into master Aug 12, 2019
@asturur asturur mentioned this pull request Aug 19, 2019
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
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
@asturur asturur deleted the fix-gradient-parsing branch January 15, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants