Bezier spline #73
Bezier spline #73
Conversation
Sources/Turf/Turf.swift
Outdated
| /** | ||
| Returns a new LineString based on bezier transformation of the input line | ||
| */ | ||
| public static func bezier(_ line: LineString, resolution: Int = 10000, sharpness: Double = 0.85) -> LineString? { |
There was a problem hiding this comment.
This static function would probably be a better fit as a method LineString.bezier(resolution:sharpness) in LineString.swift or as an extension on LineString in Spline.swift.
There was a problem hiding this comment.
Done as LineString extension in Spline.swift
Sources/Turf/Spline.swift
Outdated
| #endif | ||
|
|
||
|
|
||
| /** |
There was a problem hiding this comment.
nit: Add documentation or remove the block if SplinePoint is only for internal use.
There was a problem hiding this comment.
of course, done.
1ec5
added
improvement
|
@frederoni I updated the code according to your reviews. Thanks. |
There was a problem hiding this comment.
Thanks for this contribution! Bézier splines will be pretty useful in combination with the map SDK.
I didn’t spot any actual porting problems, though it does look like Turf.js might have an off-by-one error. Most of the comments below are suggestions on how to make the code more functional. I think the functional alternatives make the code easier to understand. (There may be a memory tradeoff in the case of zip(_:_:), but the other suggestions should be memory-neutral at worst.)
| t = 0 | ||
| } | ||
| if t > duration { | ||
| t = duration - 1 |
There was a problem hiding this comment.
Is it just me, or is there an off-by-one error in the original JavaScript version? If t is equal to duration, then leave it alone, but if it’s greater than duration, cap it at duration−1. 🤔
Without further investigation, I’m not sure we should deviate from Turf.js, given our experience with another seemingly accidental off-by-one error that turned out to be significant: #27.
There was a problem hiding this comment.
Filed upstream for further investigation: Turfjs/turf#1567.
|
I merged master into this pull request to keep all things sync as I started with version 2.1 and I saw merge conflicts with v3 |
Add bezier spline support by porting original code