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

Fix closest layer point edge case #9114

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MadhukarRagaji
Copy link

@MadhukarRagaji MadhukarRagaji commented Oct 5, 2023

This pull request addresses an edge case in the closestLayerPoint function where it did not properly consider the segment between the last and first points of a Polygon.

Details:

Identified that the closestLayerPoint function was missing the edge case where the point could be on the segment between the last and first points of a Polygon.
Modified the iteration logic to ensure this segment is also evaluated when finding the closest layer point.

Testing:

Set up a local test environment with a triangle polygon and markers. Confirmed that the function now correctly identifies points on the edge between the last and first points of the Polygon.
All existing tests pass, and no regressions were identified.

Related Issues:

Fixes: #9098

@MadhukarRagaji
Copy link
Author

#9098

@se-ti
Copy link

se-ti commented Oct 6, 2023

This PR seems to break Polyline.closestLayerPoint, adding new edge between last and first point, and even for a Polygon this shouldn't work when clipping occures.

I'd try to construct polygon with both start and last points outside the screen. This should give reduced _parts arrays, and closestLayerPoint would find a point on the [last point; first point] edge, which doesn't exist in original polygon.

Serge, author of #9098

@Falke-Design
Copy link
Member

I'd try to construct polygon with both start and last points outside the screen. This should give reduced _parts arrays, and closestLayerPoint would find a point on the [last point; first point] edge, which doesn't exist in original polygon.

@se-ti you are right, good catch 👍 but this problem already oocurs on polylines too and has not directly something to do with #9098

Demo Polyline: https://plnkr.co/edit/TzzbNQf5ljXTGRV2
Demo Polygon: https://plnkr.co/edit/7gPOyNiLgGOTDxUs

If I read the docs, I would think it will return always the closest point and has nothing todo with clipping. So my suggestion is to do a refactoring and use the actual latlngs (projected to points) and not the clipped parts.
grafik

@se-ti
Copy link

se-ti commented Oct 6, 2023

I agree about docs, @Falke-Design.
Probably the only aspect where current design is better -- it's much faster for large geometries like countries' borders.

@Falke-Design
Copy link
Member

If we replace _parts with _rings everything works fine. https://plnkr.co/edit/J78vmzTx82kPh1Ck
Perfomance could be still an issue, needs to be tested.

@mourner @IvanSanchez @jonkoops do you see any problems if we use _rings instead of _parts in closestLayerPoint? With _parts we have the problem that the calcuation is done with the clipped path.

@se-ti
Copy link

se-ti commented Oct 8, 2023

Thought for a while: if you decide to rewrite closestLayerPoint from _parts to _rings, could you please return not only point and distance, but also ring's index, and point's index in ring. (point -- starting point of the edge, closestLayerPoint belongs to).

I used closestLayerPoint's as a second filter (after bounding box) when looking for a nearest edge of large and complex geometries. But to find nearest edge i had to rewrite closestLayerPoint for a latlngs array and call it one more time, just to find those indices.

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

Successfully merging this pull request may close these issues.

closestLayerPoint doesn't find point on the edge between last and first point of Polygon
3 participants