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.

Sign up for GitHub

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

Jump to bottom

closestLayerPoint doesn't find point on the edge between last and first point of Polygon #9098

Open
4 tasks done
se-ti opened this issue Sep 20, 2023 · 8 comments · May be fixed by #9114
Open
4 tasks done

closestLayerPoint doesn't find point on the edge between last and first point of Polygon #9098

se-ti opened this issue Sep 20, 2023 · 8 comments · May be fixed by #9114
Labels
bug needs triage Triage pending

Comments

@se-ti
Copy link

se-ti commented Sep 20, 2023 •
edited

Checklist

  • I've looked at the documentation to make sure the behavior isn't documented and expected.
  • I'm sure this is an issue with Leaflet, not with my app or other dependencies (Angular, Cordova, React, etc.).
  • I've searched through the current issues to make sure this hasn't been reported yet.
  • I agree to follow the Code of Conduct that this project adheres to.

Steps to reproduce

  1. Create a triangle polygon let it be [[0,0], [1,1], [0,1]]
  2. Choose a point on it's edge between last and first point -- let it be [0, 0.5]
  3. Try to find closest layer point: instead of the same point, algorithm would return points on other edges.

Reason: Even though Polygon extends Polyline, it doesn't fix polyline's closestLayerPoint method,
which doesn't take into account the edge [length-1, 0].

Leaflet/src/layer/vector/Polyline.js

Line 98 in 641a382

for (let i = 1, len = points.length; i < len; i++) {

That's correct for polyline, but not for polygon.

Expected behavior

since point [0, 0.5] lies on the edge [[0, 0], [0, 1]], i'd expect closestLayerPoint to return point, that would be reprojected back to [0, 0.5]

Current behavior

closestLayerPoint returns point, that is reprojected back to [0.25, 0.25].

Minimal example reproducing the issue

https://plnkr.co/edit/BmMqSBJYM3QoOiO6

Environment

  • Leaflet version: Leaflet 1.9.4+v1.d15112c,
  • Browser (with version): Chrome 116.0.5845.190 (Official Build) (64-bit) (cohort: Control)
  • OS/Platform (with version): Win 10
@se-ti se-ti added bug needs triage Triage pending labels Sep 20, 2023
@se-ti
Copy link
Author

se-ti commented Sep 24, 2023

Fix, that worked for me.
Inspired by L.Polyline.closestLayerPoint

L.Polygon.prototype.closestLayerPoint = function(p) {
    let minPoint = L.Polyline.prototype.closestLayerPoint.call(this, p);
    let minDistance = minPoint ? minPoint.distance * minPoint.distance : Infinity;
    const closest = L.LineUtil._sqClosestPointOnSegment;

    for (let j = 0, jLen = this._parts.length; j < jLen; j++) {
        const len = this._parts[j].length;
        if (len == 0)
            continue;

        const sqDist = closest(p, this._parts[j][0], this._parts[j][len-1], true);
        if (sqDist < minDistance) {
            minDistance = sqDist;
            minPoint = closest(p, this._parts[j][0], this._parts[j][len-1]);
        }
    }

    if (minPoint) {
        minPoint.distance = Math.sqrt(minDistance);
    }
    return minPoint;
}

@codebreaker0001
Copy link

Hey @se-ti can you tell me , how i can work on this issue

@se-ti
Copy link
Author

se-ti commented Oct 1, 2023

Fix it?
Sorry, i don't understand your question :( (or does "to work on" have other meanings except "to influence"?)

@MadhukarRagaji
Copy link

Have raised a PR for this one
[#9114]

@MadhukarRagaji MadhukarRagaji linked a pull request Oct 5, 2023 that will close this issue
Open
@se-ti
Copy link
Author

se-ti commented Oct 6, 2023

Sorry, @MadhukarRagaji, your fix is a no go.
It ruins correct behaviour of a polyline's closestLayerPoint in favour of highly likely incorrect behaviour of a polygon's closestLayerPoint.

There's a difference between _parts and latlngs arrays when clipping occures. So i'd expect wierd bugs with your (and my fix too) when polygon doesn't fit in screen, like country borders often do.

@MadhukarRagaji
Copy link

ah, what if we handle this as a specific edge case just for a polygon?
modified preview (do a in-app refresh)
@se-ti

@MadhukarRagaji
Copy link

ah okay will update if I find something

@se-ti
Copy link
Author

se-ti commented Oct 6, 2023

See my comments on reproducing clipping errors below your pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug needs triage Triage pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix closest layer point edge case MadhukarRagaji/Leaflet
3 participants
@se-ti @MadhukarRagaji @codebreaker0001