Detect coincident points when drawing arcs with rounding.#39
Open
jasondavies wants to merge 4 commits intod3:mainfrom
Open
Detect coincident points when drawing arcs with rounding.#39jasondavies wants to merge 4 commits intod3:mainfrom
jasondavies wants to merge 4 commits intod3:mainfrom
Conversation
When rounding is used, it's possible for `arc()` to generate empty arcs in the case where the start and end points are almost coincident, and become coincident after rounding is applied. This adds a check for coincident points after rounding is applied, and splits the arc into two if coincident points are detected. Fixes d3#38.
7f4ed0d to
97f6a50
Compare
Fil
reviewed
Oct 15, 2024
Skip unnecessary division for performance. Use `d` rather than `digits` to compute the power of 10. Co-authored-by: Philippe Rivière <fil@rezo.net>
b339f8a to
dbde484
Compare
Fil
reviewed
Oct 16, 2024
Fil
approved these changes
Oct 16, 2024
mbostock
requested changes
Oct 16, 2024
Member
mbostock
left a comment
There was a problem hiding this comment.
Thanks for the fix! And hi Jason! 👋 Two small nits.
- Use pessimistic `!(d <= 15)` check in case of NaN. - Use `digits == null` check to match previous line.
Author
|
It might also be worth generating a set of visual tests to double-check ccw={true, false} and values of a0, a1 where abs(a1 - a0) is some multiple of 2π, in case there's a faulty assumption with da /= 2. |
Author
|
I think my assumptions are correct. After these lines: // Does the angle go the wrong way? Flip the direction.
if (da < 0) da = da % tau + tau;
// Is this a complete circle? Draw two arcs to complete the circle.
if (da > tauEpsilon) {
this._append`A${r},${r},0,1,${cw},${x - dx},${y - dy}A${r},${r},0,1,${cw},${this._x1 = x0},${this._y1 = y0}`;
}We can be sure that 0 ≤ da ≤ tauEpsilon, hence it's safe to use da /= 2 to determine the midpoint of the arc. |
Author
|
Ping :) |
|
Great to hear from you @jasondavies !!! I must say I'm a huge fan of your work. This contribution looks amazing. |
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When rounding is used, it's possible for
arc()to generate empty arcs in the case where the start and end points are almost coincident, and become coincident after rounding is applied.This adds a check for coincident points after rounding is applied, and splits the arc into two if coincident points are detected.
Fixes #38.