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

Incorrect Bezier Curves Parsing #12

Closed
walkvanja opened this issue Dec 2, 2017 · 3 comments
Closed

Incorrect Bezier Curves Parsing #12

walkvanja opened this issue Dec 2, 2017 · 3 comments
Labels

Comments

@walkvanja
Copy link

Hi!

First of all thanks for this lib!
But playing around with it I found one issue: if straight line is drawn as Bezier Curves - it ignores transformation.

See details:
image
screenshot from https://codepen.io/walkvanja/pen/RjvpbP

Reason:
in Warp.js https://github.com/benjamminf/warpjs/blob/master/src/Warp.js#L55 used
points.slice(0, 2) for measuring distance

in our case we have points
https://monosnap.com/file/Fp1LmCt1yq2M4nD4o0FW8rMPlr9VSa
image
(we are talking about M0,0 C0,0, 200,0, 200,0)

and distance is 0 (between 0,0 and 0,0)

if remove slice(0, 2) - we'll use distance between 0,0 and 0,200 (first and last point)

Benjamin, if you have time - please share your thoughts if we really need .slice().
I looked into git history, but slice(0,2) was from the beginnig, it is not a hotfix

thanks,
Vanja

@benjamminf
Copy link
Owner

Hey Vanja, thanks for the detailed bug report!

The reason why the slice() call is there is mostly for performance reasons. Performing a distance calculation on all points in a bezier curve is slower than just performing a distance calculation on the first and last points in the curve. The accuracy is slightly worse but it's an appropriate trade-off in my opinion.

The bug has appeared because I forgot that the control points of a bezier curve would appear before the end point in the points array. I think at one point in development I had it in the order of start, end, control1, control2 but then changed it back to avoid confusion, forgetting I had relied on that order in this section.

I might have actually been confused at what level this array of points was. The structure of the points array is a multi-dimensional array, with each point being an array of values. A single point array contains firstly the x and y components, but as you can extend points with more values (see README.md) they should also be ignored when calculating the distance (including them might be useful though for some cases, but it seems needlessly overcomplicated as it makes it very difficult to judge what an appropriate threshold should be). Hence grabbing the first two x/y values with a slice() call, though being off by a dimension.

The fix is to replace points.slice(0, 2) with: [ points[0].slice(0, 2), points[points.length - 1].slice(0, 2) ]. Pretty ugly, but oh well, it works. I'm making a release for it now.

@walkvanja
Copy link
Author

Benjamin,

I appreciate your help. Your super-fast fix and detailed explanation was really helpful.
Now I am able to experiment with vector fonts wrapping (exported from Cufon library).

example:
https://codepen.io/walkvanja/pen/Rjvzre?editors=1010

P.S. Yes, I know about modern libs like Typr or opentype.js for vector fonts in SVG, but I used to Cufon =)
P.P.S. Do you have any plans to include sample transform functions into dist?

thanks,
Ivan

@benjamminf
Copy link
Owner

benjamminf commented Dec 5, 2017

No worries. Check out #11 as someone has been playing with warping text too.

Do you have any plans to include sample transform functions into dist?

Yeah eventually I want a collection of configurable filters included with the library, including what was developed in #11. Not sure when that'll happen – maybe soon.

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

No branches or pull requests

2 participants