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

greatCircle returns array of NaN if start and end is the same #2342

Open
laurenceks opened this issue Oct 4, 2022 · 5 comments · May be fixed by #2343
Open

greatCircle returns array of NaN if start and end is the same #2342

laurenceks opened this issue Oct 4, 2022 · 5 comments · May be fixed by #2343

Comments

@laurenceks
Copy link

laurenceks commented Oct 4, 2022

@turf/great-circle 6.5.0

If the coordinates passed to greatCircle are the same it returns an array of NaNs.

This has happened during a line animation along a greatCricle. The animation draws a line up to the progress marker so that the line is divided in colour to show progress. Effectively the initial complete greatCircle line is being painted over by the same line (this is in mapbox-gl).

As the progress percentage in the first frame is 0% the coordinates passed to greatCricle are the same for start and end points. greatCricle returns a linestring with coordinates of [[NaN, NaN], [NaN, NaN]...], which cannot be used and cause an error.

Expected output would be an array of all coordinates the same (the start point).

Example

This does not work when coordinates.start.arrayLongLat and progressMarkerCoordinates are equal:

const progressRouteCoordinates = greatCircle(coordinates.start.arrayLongLat, progressMarkerCoordinates).geometry.coordinates;

Comparing the start and end and substituing a simple linestring if they are the same prevents this issue, but I feel shouldn't be necessary

const progressRouteCoordinates = progressMarkerCoordinates[0] === coordinates.start.long && progressMarkerCoordinates[1] === coordinates.start.lat ? [coordinates.start.arrayLongLat, progressMarkerCoordinates] :
            greatCircle(coordinates.start.arrayLongLat, progressMarkerCoordinates).geometry.coordinates;

Code context

const animateMap = (durationInMs = mapAnimationDurationInMs) => {
    //make sure the event listener only fires once
    scrolledIntoFrame.unobserve(document.querySelector("#map"));
    const easing = BezierEasing(0.16, 1, 0.3, 1); //easeOutExpo to match countUp
    let startTime = null;
    const progressAnimation = (timestamp) => {
        if (!startTime) {
            startTime = timestamp;
        }
        const runtime = timestamp - startTime;
        const progressPercent = easing(runtime / durationInMs);

        //calculate new coordinates based on current progress
        const progressMarkerCoordinates = along(linestring(route.data.geometry.coordinates),
            sheetsData["Total distance"] * progressPercent, {
                units: "kilometers"
            }).geometry.coordinates;

        //**ISSUE IS HERE** - without comparison, greatCircle returns array of NaN because the line hasn't progressed yet and so the start/end coordinates are the same
        const progressRouteCoordinates = greatCircle(coordinates.start.arrayLongLat, progressMarkerCoordinates).geometry.coordinates;


        const remainingRouteCoordinates = greatCircle([...progressRouteCoordinates].pop(),
            coordinates.end.arrayLongLat).geometry.coordinates;
        //update marker and lines
        progressMarkerMapBoxGl.setLngLat(progressMarkerCoordinates);
        map.getSource("progressRoute")
            .setData({
                ...progressRoute.data,
                geometry: {
                    ...progressRoute.data.geometry,
                    coordinates: progressRouteCoordinates,
                },
            });
        map.getSource("remainingRoute")
            .setData({
                ...remainingRoute.data,
                geometry: {
                    ...remainingRoute.data.geometry,
                    coordinates: remainingRouteCoordinates,
                },
            });

        //if not yet complete, request a new frame
        if (progressPercent < 1) {
            requestAnimationFrame(progressAnimation);
        }
    };
    requestAnimationFrame(progressAnimation);
};

Is this intended behaviour or a genuine issue?

(Similar old issues #1361 #1946 with lineOffset, ?related)

@rowanwins
Copy link
Member

Thanks for the bug report @laurenceks - it sounds like a more valid output would be a linestring with duplicated first and last coords.

@rowanwins
Copy link
Member

rowanwins commented Oct 6, 2022

One question @laurenceks - do you think the resulting line should have two coords, or should it have npoints repeated as per the option?
eg

const line = greatCircle(start, end)
=> line.geometry.coordinates.length = 2
or 
=> line.geometry.coordinates.length = 100

At this stage I'm leaning towards the first, and adding the following to the documentation

If the start and end positions are the same then a LineString with 2 points will be returned and npoints option will be ignored.

@laurenceks
Copy link
Author

My instinct is actually to match npoints as personally I would expect greatCircle to always give a consistent array length, even if it's an array of 100 duplicates. Whilst the shorter line string would work for my use case, there might be scenarios where someone's code is expecting an array of the same length (npoints) every time.

The issue at hand isn't the array length but the contents of the array. If greatCircle returned an array of 100 duplicates I think that would still plot fine on a map; it's the output of NaN causing the problem. I think anyone wanting to remove duplicates could use cleanCoords?

That being said I'm just an amateur and don't really have a strong opinion! My broad understanding is consistency is usually better with function returns but so long as it outputs a valid line string my issue would be fixed.

Ps thanks for picking this up quickly!

@rowanwins
Copy link
Member

Awesome - having slept on it I think I agree with you @laurenceks so I've updated the PR.

@RobbertWolfs
Copy link

Any update on this?

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

Successfully merging a pull request may close this issue.

4 participants