-
Notifications
You must be signed in to change notification settings - Fork 942
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 turf-line-arc to generate the correct number of steps in the generated arc #2525
Conversation
…ct number of steps. Also a bit related to Turfjs#2446, which should have been fixed a while ago by a previous fix to the previous version of this code (see Turfjs#2142). Including a unit test to make sure we don't go backwards with this approach.
…result of fix to turf-line-arc. All changes checked visually using geojson.io, noting start and end coords are identical and it's only the intermediate ones changing due to the curve being smoother by default.
// 2 to 2 + 1, ... | ||
const expectedPoints = geojson.properties?.steps | ||
? geojson.properties?.steps + 1 | ||
: 64 + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WIthout any context seeing that the arc would get N+1 points feels weird. The jsdoc on the function doesn't indicate that it would be +1 either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My interpretation of a "step" is similar to a "segment". So a line with 3 segments has 4 points: +---+---+---+
Have I misunderstood the steps param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can 'understand' steps as either the number of points or the number of segments. It isn't documented so I suppose we're free to do whichever we want (and maybe document it?). We could maybe consider the previous behavior as the expected behavior. I'm not really tied to that as being important, but maybe we should document whatever the outcome is in the jsdoc at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll push a commit updating the docs, and then merge. Thanks for reviewing 👍
const arcStep = (arcEndDegree - arcStartDegree) / steps; | ||
// Add coords to the list, increasing the angle from our start bearing | ||
// (alpha) by arcStep degrees until we reach the end bearing. | ||
while (alpha <= arcEndDegree) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this gets weird with floating point logic with N steps and very occasionally skips the final step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had crossed my mind. I did some simple testing and if there are precision errors in the reverse direction they tend to go low:
let steps = 1234567890; // Tried various values for steps
const x = 1 / steps;
// x 8.10000007371e-10
// Re-multiplication never seems to *exceed* the original value
const t = 0 + x * steps;
// t 1
// t 0.9999999985906001
// t 0.9999999978858999
If the bearing tends a little short then the <= should let that loop get as close as possible to the endBearing without going a whole step over.
We could go back to a "calculate all the step angles except the last one and use the endAngle for the last" approach. From what I can see above though JS floating point shoudl be ok, and the code is simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With a slightly different construction I think we can go over, but I appreciate you digging in. I feel good about this and we can always substitute the arcEndDegree for the final point later if there's something we didn't forsee.
let steps = 1e5;
const x = 1/steps;
let v = 0;
for (let i=0; i<steps; i++) {
v+=x;
}
v
0.9999999999980838
let steps = 1e10; // warning this is pretty slow
const x = 1/steps;
let v = 0;
for (let i=0; i<steps; i++) {
v+=x;
}
v
1.0000000694756235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
… rebuilt the readme as generate-readmes needs some work. Will make that a seperate PR though.
Resolves #2524 by fixing a bug in the way lineArc steps its way through the arc and generates coordinates for each segment. Lots of test fixture changes, however these are mostly arcs that are now being generated more smoothly with the default 64 segments, whereas before they would be generated with fewer than 64.
Please fill in this template.
npm test
at the sub modules where changes have occurred.npm run lint
to ensure code style at the turf module level.Submitting a new TurfJS Module.
n/a