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(polyline): stroke bounding rect calculation #8083

Closed
wants to merge 42 commits into from

Conversation

luizzappa
Copy link
Contributor

@luizzappa luizzappa commented Jul 26, 2022

Related Issues

Proposed changes

  • Created acos function in fabric.util to ensure the value is in the range [-1, 1] and the calcAngleBetweenVectors function is using it. This was causing a bug (see comment on commit bc93beb)
  • In calcAngleBetweenVectors function, replaces Math.hypot(x, y) with Math.sqrt(x*x + y*y) to ensure equal results in both engines: Firefox and Chrome. (see comment on commit bc93beb)
  • In getBisector function, checks if ro is close to zero instead of being strictly equal.
  • In calcAngleBetweenVectors function, calculate angle between two vector using atan2 instead of dot product. So eliminated the need to check the angle direction in getBisector function,
  • Changed the _setPositionDimensions function in the polyline class to adjust the width/height by the scale in case of uniform stroke.
  • Changed the projectStrokeOnPoints function to cover all stroke line join cases and also to uniform stroke. This function deserves further comment:

In the case of open path, the stroke is rendered differently, so I calculated with the vectors orthogonal to the stroke.
image

In the case of the bevel, they are the points formed by the orthogonal to the vertex
image

In the case of the round, if the angle is less than or equal to 90°, the projection is calculated with two vectors parallel to the X and Y axes. In the case of larger angles, it is calculated using the orthogonals.
image

In the case of the miter, is the same calculation commented here: #8025

For a better view, see this link (you can change the vectors direction by holding the red circles) : https://hypertolosana.github.io/efficient-webgl-stroking/index.html

Screenshots

Working example here: https://codesandbox.io/s/project-stroke-points-with-context-to-trace-b8jc4j?file=/src/index.js

image

The red dotted vector is the bisector returned by the getBisector function. The green dotted vector is in the opposite direction and is what is used to determine which side to plot the projections. The projections are the colored vectors, one color for each point.
To make debugging easier, you can configure the parameters (stroke line join type, strokeUniform, miterLimit, etc..)

openPath = false and strokeUniform = true

Angle Miter Round Bevel
less than 90° image image image
greater than 90° image image image
equal to 180° image image image

Copy link
Contributor Author

@luizzappa luizzappa left a comment

Choose a reason for hiding this comment

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

I don't know if this can generate a bug in case of very small angles

@luizzappa luizzappa changed the title fix(polyline): bounding rect for all stroke line join and stroke uniform fix(polyline): project stroke width on points Jul 26, 2022
@luizzappa luizzappa changed the title fix(polyline): project stroke width on points fix(polyline): bounding rect calculation Jul 26, 2022
…pathOffset on _setPositionDimension function for polyline
@luizzappa luizzappa marked this pull request as ready for review July 26, 2022 04:32
@ShaMan123 ShaMan123 changed the title fix(polyline): bounding rect calculation fix(polyline): stroke bounding rect calculation Jul 26, 2022
@ShaMan123 ShaMan123 self-assigned this Jul 26, 2022
@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 26, 2022

Amazing work.
I will review it later on today.
Meanwhile, we can rethink calcAngleBetweenVectors and use atan2 diff instead of dot product. That will give use the exact angle with the correct direction, eliminating the need to check the direction ourselves, hence resolving the ro issue.
How strange that acos gets values out of bounds.

@luizzappa
Copy link
Contributor Author

Amazing work. I will review it later on today. Meanwhile, we can rethink calcAngleBetweenVectors and use atan2 diff instead of dot product. That will give use the exact angle with the correct direction, eliminating the need to check the direction ourselves, hence resolving the ro issue. How strange that acos gets values out of bounds.

Great idea! I just committed this change.

firefox is crazy enough to calculate this wrong
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

This is excellent work. Thorough and precise.
Most of my comments are related to styling.
I would like to eliminate the use of duplicate code as much as possible and utilize fabric.Point methods such as multiply scalarMultiply making the code more readable.

I will commit visual tests with the examples you have created.
Once I do that it will be easy for you to add more. We should cover all cases and as many edge cases as possible.
I will explain how to run the tests as well.

I would never have figured this out:

In the case of the round, if the angle is less than or equal to 90°, the projection is calculated with two vectors parallel to the X and Y axes. In the case of larger angles, it is calculated using the orthogonals.

Thank you for this awesome contribution!

src/util/misc.js Outdated Show resolved Hide resolved
src/shapes/polyline.class.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
src/util/misc.js Outdated Show resolved Hide resolved
@ShaMan123 ShaMan123 mentioned this pull request Jul 27, 2022
@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 27, 2022

@luizzappa I have created a PR #8087 with tests that is forked from this one.
I don't want to merge it here to avoid the mess of so many files.

To run tests on fabric:

  1. npm run build -- -w
  2. npm test: select visual/generic_rendering

This will run visual tests on node. Which isn't enough.

To run in the browser you need to clone fabricjs.com. See the contribution guide for how to.

And don't hesitate to reach out if it isn't clear enough.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 27, 2022

Visual testing has exposed a bug in strokeMiterLimit

@ShaMan123
Copy link
Contributor

https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-miterlimit

image

I wonder what is the inner corner of the miter

The ratio of miter length (distance between the outer tip and the inner corner of the miter) to stroke-width

@luizzappa
Copy link
Contributor Author

@luizzappa I have created a PR #8087 with tests that is forked from this one. I don't want to merge it here to avoid the mess of so many files.

To run tests on fabric:

  1. npm run build -- -w
  2. npm test: select visual/generic_rendering

This will run visual tests on node. Which isn't enough.

To run in the browser you need to clone fabricjs.com. See the contribution guide for how to.

And don't hesitate to reach out if it isn't clear enough.

I didn't quite understand what you're comparing to in these tests. Is the percentage of the object that is outside the blue rectangle?

Running on node there were no failures.

image

I must be doing something wrong to run in the browser. I cloned fabricjs.com, copied the tests, but they don't show up.

image

@luizzappa
Copy link
Contributor Author

luizzappa commented Jul 28, 2022

Visual testing has exposed a bug in strokeMiterLimit

Would you mind reproducing a buggy case here: https://codesandbox.io/s/fabricjs-project-stroke-working-forked-gem8z9?file=/src/index.js

I tried to reproduce but I didn't find the bug. The behavior is different, even the miter limit is at 4, the stroke doesn't become a bevel. I must be missing some parameter.

Golden:
image

Codesandbox:
image

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 28, 2022

Forgive me, I forgot a command... To start fabricjs.com in dev mode run from fabric.js npm start
Then it should show up.

The tests run and compare the output with an image ref stored in test/visual/golden/**.
The percentage is the pixel diff between the two.
There will be no diff because I generated the refs from the current state of the code. Meaning it will output the same image.
Navigate to test/visual/golden/stroke-projection/miter/ and see that there are refs that have a wrong bbox due to the miter limit calculation.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 28, 2022

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jul 28, 2022

My bad for the second time!
I have discovered a bug in node canvas miter limit calculation, not on our side.
As you can see the left side image is the ref image I produced (node) and the right side is the image generated by the browser test.
I will update the refs

luizzappa and others added 2 commits July 29, 2022 00:28
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
@ShaMan123
Copy link
Contributor

@lovegnep you should provide an explanation and a summary, no one should guess why you posted this code here

@ShaMan123
Copy link
Contributor

@luizzappa ?

@luizzappa
Copy link
Contributor Author

luizzappa commented Sep 24, 2022

@ShaMan123 , I'm working on it. It's more difficult than I initially imagined...

I got the miter and bevel cases, but the round is still missing.

When skew is applied, the semicircle becomes a segment of an ellipse, and we need to find the farthest points from that ellipse.

Picture1

I'm going to take this weekend to review some concepts of geometry and calculus, I believe that with the ellipse equation we can take the derivative and find the tangents parallel to the x-axis and y-axis that are candidates for more distant points.

@ShaMan123
Copy link
Contributor

Alright
I am thinking we could merge it with skew broken and return to that (something I learned lately from @asturur...)
This PR, your work, is a huge leap forward in bbox calculations and accuracy.
It is important to feel accomplishment as well and merging is a good and decent way to do that. It will give more motivation to tackle the next issue.
I am more focused on updating from master, es6, types etc. It is more important IMO at this point.
If you'd like I can migrate it and produce a diff for you to merge into the PR (you deserve the credit)
Open to your thoughts and will.

@ShaMan123
Copy link
Contributor

Just airing some thoughts regarding round + skew
Your code assumes that both axis are orthogonal. That is why it is enough to add the radius of each axis to a point, getting 2 projected points covering the intersection of the tangent lines of the curve.
And that is the problem when skew is applied, making the axis not orthogonal, am I right so far?

@ShaMan123
Copy link
Contributor

ShaMan123 commented Sep 24, 2022

If that is the case why don't we intersect the 2 vectors after they are transformed by the plane?
Or before and transform the intersection by the plane?

@ShaMan123
Copy link
Contributor

Ok, now I understand why you want to look for the axis tangents.
Intersection isn't accurate enough... but the tangents are.

Isn't the bisector orthogonal to the "tip" of the semicircle?
Could it mean that the "tip" point is our point of interest regardless of the shear transform?
If this is correct (I am quite positive I have a huge hole in my reasoning) once we transform that point with the shear matrix we should get the farthest point.

Another direction:
Look at the path util function getBoundsOfCurve. We can use that (calculating the control point doesn't sound too complex)

@luizzappa
Copy link
Contributor Author

Alright I am thinking we could merge it with skew broken and return to that (something I learned lately from @asturur...) This PR, your work, is a huge leap forward in bbox calculations and accuracy. It is important to feel accomplishment as well and merging is a good and decent way to do that. It will give more motivation to tackle the next issue. I am more focused on updating from master, es6, types etc. It is more important IMO at this point. If you'd like I can migrate it and produce a diff for you to merge into the PR (you deserve the credit) Open to your thoughts and will.

I'm in the flow, give me a few more days, if I can't solve this problem, I focus on refactoring and PR. I will appreciate your help, I have little experience in typescript

@luizzappa
Copy link
Contributor Author

luizzappa commented Sep 24, 2022

Ok, now I understand why you want to look for the axis tangents. Intersection isn't accurate enough... but the tangents are.

Isn't the bisector orthogonal to the "tip" of the semicircle? Could it mean that the "tip" point is our point of interest regardless of the shear transform? If this is correct (I am quite positive I have a huge hole in my reasoning) once we transform that point with the shear matrix we should get the farthest point.

I tried using the point that is in the direction of the bisector before the transformation and then applying the skew (I tried the opposite too), but as the new position of the X depends on the Y height the point is at, it will not necessarily be the solution. Take a look:

skew with bisector

Another direction: Look at the path util function getBoundsOfCurve. We can use that (calculating the control point doesn't sound too complex)

I'll take a look, thanks! Mathematically I've already calculated the points that have tangents parallel to the X and Y axes. Now it's translating, I believe I'm close to solving

image

@luizzappa
Copy link
Contributor Author

luizzappa commented Sep 30, 2022

I adjusted the round line join with skew. However, it only works when skewX and skewY are applied separately. If the two are applied together, it's still buggy. But I wonder what must be happening. Let's stop here, continue with the PR and then I'll think about this case again

image

Just commenting on the solution.. It's a little different than what I initially imagined. The semicircle does not become the segment of an ellipse. The logic behind it is to work with the equation that generates the position of the point after the skew is applied. For example, skewX, the final point $X_f$ will be:

$X_f = X + Y * Tan(skewX)$

Limited by the equation (the circle of the round line join):

$Y^2 = r^2 - X^2$

Substituting the second equation into the first, taking the derivative and equating to zero to find the maximum/minimum, we find:

$X = \pm \frac{r}{\sqrt{1 + Tan^2(skewX)}}$

This is the value of X before applying skewX which will be the furthest distance from origin after applying skewX. The same logic follows for skewY.

The problem when skewX and skewY are applied at the same time is that skewY influences the position calculation with skewX. The new Y should be this:

$(Y + X * Tan(skewY))^2 = r^2 - X^2$

So, $Y = \pm (\sqrt{r^2 - X^2} - X * Tan(skewY))$

Then substitute into the $X_f$ equation and calculate the derivative. This should give the furthest distance when skewX and skewY are applied at the same time. After opening the new PR, I go back to work on that case.

@luizzappa
Copy link
Contributor Author

luizzappa commented Sep 30, 2022

My code is a mess. Tomorrow I will adjust it, and commit to this PR to have a valid version. Then I open a new PR by cloning from the master and work with the new structure in typescript

@ShaMan123
Copy link
Contributor

ShaMan123 commented Oct 1, 2022

Outstanding.

I will take the advantage and use this PR for learning a bit if you don't mind.
So I will ask a few question to see if I understand.

Are you looking for a derivative for 2 variables?

Is it correct to assume that when d/dy equals 0 it is an asymptote for d/dx and vice versa? If so that can simplify calculations

Correction
Is it correct to assume that when d/dy equals 0 it is an asymptote for the function plotting x as well as to d/dx and vice versa? If so that can simplify calculations since finding the asymptote sounds pretty straight forward in a root function.

Are these functions inverse to each other?

@luizzappa
Copy link
Contributor Author

Outstanding.

I will take the advantage and use this PR for learning a bit if you don't mind. So I will ask a few question to see if I understand.

Are you looking for a derivative for 2 variables?

Is it correct to assume that when d/dy equals 0 it is an asymptote for d/dx and vice versa? If so that can simplify calculations

Correction Is it correct to assume that when d/dy equals 0 it is an asymptote for the function plotting x as well as to d/dx and vice versa? If so that can simplify calculations since finding the asymptote sounds pretty straight forward in a root function.

Are these functions inverse to each other?

No, the equation is one-variable. I don't use implicit differentiation.

$f(X) = X \pm \sqrt{r^2 - X^2} * Tan(skewX)$

And are inverse:

image

@luizzappa
Copy link
Contributor Author

luizzappa commented Oct 1, 2022

I'll take the opportunity to detail it better for future documentation.

Consider this case:
image

There is a point $(X_i, Y_i)$ that will be the furthest X distance from the origin after applying skewX. This is our point of interest.
image

We have:

[eq.1] $X_{afterSkewX} = X_i + Y_i * Tan(skewX)$

And we know that $Y_i$ is a function of $X_i$:

$Y_i^2 + X_i^2 = r^2$
[eq.2] $Y_i = \pm \sqrt{r^2 - X_i^2}$

Substituting eq.2 in eq.1 we have:

$X_{afterSkewX} = X_i \pm \sqrt{r^2 - X_i^2} * Tan(skewX)$

So we have an equation in terms of just X:

$f(X_i) = X_i \pm \sqrt{r^2 - X_i^2} * Tan(skewX)$

The plus/minus sign represent both sides of the circle after skewX:
image

The furthest X distance from the origin are the maximum/minimun of $f(X_i)$:
image

Computing $f'(X_i)=0$ for both cases, we find:

$X_i = \pm \frac{r}{\sqrt{1 + Tan^2(skewX)}}$

Substituting $X_i$ into the circle equation, we find $Y_i$:

$Y_i = \pm \sqrt{r^2 - X_i^2}$

That's our solution:

image

I'm considering the two points for projection, this can be a problem when the control points are too close. I'll adjust to consider only the point belonging to the yellow segment.

@ShaMan123
Copy link
Contributor

Got it.

The furthest X distance from the origin are the maximum/minimun of $f(X_i)$:

- The furthest X distance from the origin are the maximum/minimun of $f(X_i)$:
+ The furthest Y distance from the origin are the maximum/minimun of $f(X_i)$:

???

@luizzappa
Copy link
Contributor Author

@ShaMan123

I started working on projectStroke.ts.

I don't know much about typescript. In the projectStrokeOnPoints function you are indicating that the function returns an array of Point, right?

export const projectStrokeOnPoints = (
  points: Point[],
  {
    strokeWidth,
    strokeLineJoin,
    strokeMiterLimit, // https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/stroke-miterlimit
    strokeUniform,
    scaleX,
    scaleY,
  }: projectStrokeOnPointsOptions,
  openPath: boolean
): Point[] => {

I changed the return to a context:

coords.push({
                            'projectedPoint': applySkew(A.add(vector)),
                            'originPoint': A,
                            'bisector': bisector
                        });
(...)
return coords

How would I indicate this to typescript? Googling I think it would be something like this:

export const projectStrokeOnPoints = (
//(...)
):  Array<{projectedPoint: Point, originPoint: Point, bisector: Point}> => {

@ShaMan123
Copy link
Contributor

ShaMan123 commented Oct 1, 2022

TS can handle that under the hood. I don't bother adding a return value unless TS is confused about it (some devs like it because it makes the signature strict).
If you hover over it TS should inform you what it asserts it is.
So I would say focus on args. But someone might call me out on that. So for now whatever you choose.

What you wrote is fine, a syntax alias to Array<T> is T[] and we prefer that but not a big deal

@ShaMan123
Copy link
Contributor

I referred you to the new guides, right? #8189

Take a look at setting up prettier if you want it seamlessly integrated into the IDE (instead of calling npm run prettier:write)
https://github.com/fabricjs/fabric.js/blob/899178603653794f811d9c20ac3e387c07d8cc71/CONTRIBUTING.md#-guidelines

@luizzappa
Copy link
Contributor Author

Got it.

The furthest X distance from the origin are the maximum/minimun of f(Xi):

- The furthest X distance from the origin are the maximum/minimun of $f(X_i)$:
+ The furthest Y distance from the origin are the maximum/minimun of $f(X_i)$:

???

The text got a little confusing because of the axes of the graph. In terms of the graph it is the farthest Y, but Y = X after skew X.

The graph plots on the X-axis the Xi value (X before skewX), and on the Y-axis the X value after skewX. So the max/min of $f(X_i)$ is the X after the skewX furthest from the origin in the "X-axis direction".

Perhaps a better wording would be: "The furthest X after skewX distance from the origin are the maximum/minimun of f(Xi)".

@ShaMan123
Copy link
Contributor

I have developed an apatite for your explanations.
If you'd like to look at qrDecompose and make that available to the a mass I know by fact that @asturur would love that and so would I.
I googled it and dived into a maze of linear algebra, gram-schmidt, eigen values/vectors, orthonormal sets and came out a few hours later as entering.

I love khanacademy
but haven't found out enough yet

@luizzappa
Copy link
Contributor Author

luizzappa commented Oct 1, 2022

I'll take a look later. What I know of linear algebra is from college (engineering). Depending on the field, there are things that I never studied..

@ShaMan123
Copy link
Contributor

ShaMan123 commented Oct 2, 2022

It works and is correct
We face accuracy issues (floating point) but I don't think there is much to do

@luizzappa
Copy link
Contributor Author

@ShaMan123, I'm not finding hypot in util, is it implemented? If not, tell me where I put it

There was that precision problem:

/**
 * Returns the square root of the sum of squares of its arguments\
 * Chrome implements `Math#hypot` with a calculation that affects percision so we hard code it as a util
 * @see https://stackoverflow.com/questions/62931950/different-results-of-math-hypot-on-chrome-and-firefox
 * @static
 * @memberOf fabric.util
 * @param {...number}
 * @returns {number}
 */
function hypot(...values) {
    let sumOfSquares = 0;
    for (let i = 0; i < values.length; i++) {
        sumOfSquares += values[i] * values[i];
    }
    return Math.sqrt(sumOfSquares);
}

@ShaMan123
Copy link
Contributor

Yes
It is something we fixed here. I did add a lint rule to block using Math.hypot.
Under util/misc you can add a dedicated file or a math file. Whatever.
@asturur refactored utils as is so feel free to remove things.
All logic under projectStroke should be dumped and replaced by your work

@luizzappa
Copy link
Contributor Author

luizzappa commented Oct 3, 2022

@ShaMan123, As I had to start from scratch from master, I opened a new PR with the branch I worked on. If you think it's better to continue with this PR, I'll close this new one #8344

@ShaMan123
Copy link
Contributor

I am fine with what you decide.
I can use github api to copy the thread. Or it can be turned into a wiki. I like that idea. An md file with all this crucial knowledge.

@ShaMan123
Copy link
Contributor

But isn't a must

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants