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

Vector3 DistanceTo Polygon and Polyline are not consistent. #945

Open
wynged opened this issue Feb 21, 2023 · 4 comments
Open

Vector3 DistanceTo Polygon and Polyline are not consistent. #945

wynged opened this issue Feb 21, 2023 · 4 comments
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@wynged
Copy link
Member

wynged commented Feb 21, 2023

Describe the bug
The method for finding distance between a polygon and the polyline form of that polygon returns bad results. When a polygon is turned into a polygon we get the expected value in the test in this branch, but the polygon itself fails.

https://github.com/hypar-io/Elements/tree/fix-distance-to-polygon

To Reproduce
Steps to reproduce the behavior:

  1. Look at the test in the branch above.

Expected behavior
Wether a polygon has been turned into a polygon or not shouldn't affect the distance to that object.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • OS: [e.g. iOS]

Additional context
Add any other context about the problem here.

@ikeough ikeough added the Bug Something isn't working label Apr 14, 2023
@ikeough ikeough assigned ikeough and wynged and unassigned ikeough Apr 14, 2023
@ikeough ikeough modified the milestones: 2.0, 0.2.1, 2.1 Apr 14, 2023
@DmytroMuravskyi
Copy link
Contributor

@wynged the reason it fails it the fact that ToPolyline has division parameter set default to 10. Function attempts to interpolate Polygon by 10 uniformly placed segments (11 vertices) distorting it heavily in the process.
Since Polygon is already a Polyline by definition this behavior can be ignored as misuse of ToPolyline.
Otherwise, I see next possible solution:

  • Set default value of division parameter to 0 that would mean automatic. It still can be 10 for base implementation in BoundedCurve.
  • Create override of ToPolyline in IntexedPolycurve.
    a) If division is 0 - all vertices are included plus N segments for each Arc (let's say the same 10).
    b) If division is more than Vertices.Count - 1 - all vertices are still included and excess is uniformly distributed between Arcs. This may lead to Arcs being still heavily simplified.
    c) If division is less than `Vertices.Count - 1 the regular behavior is used - uniform distribution though domain distorting whole shape.

@DmytroMuravskyi
Copy link
Contributor

On the second thought default 0 can be confusing too because user doesn't know what to expect.

@wynged
Copy link
Member Author

wynged commented Oct 27, 2023

I think we should make this work, rather than simply call it a bad use of to Polyline although I see what you mean.
default 0 is a bit odd... but I like the idea that we can have a preserve vertices option.
Is there a way to just change this behavior for Polygons, and have just its default behavior become preserving all existing segments, but let other implementations proceed as they currently do? Maybe a comment in the Polygon.ToPolyline documentation that notes that providing a value is likely to result in distortions of the shape?

@DmytroMuravskyi
Copy link
Contributor

Create a PR. At the end of the day I decided to remove default 10 value from to ToPolyline and and added ToPolyline without parameters as it looks the cleanest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants