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

getDistanceFromLine is returning NaN #298

Open
redcic75 opened this issue Apr 18, 2023 · 4 comments
Open

getDistanceFromLine is returning NaN #298

redcic75 opened this issue Apr 18, 2023 · 4 comments

Comments

@redcic75
Copy link

Hello @manuelbieh ,

I've just noticed that the fix you've commited after issue #227 and pushed does not deal with all the problematic cases. When we call getDistanceFromLine we still get NaN when point === startLine and when startLine === endLine.

This fix in getDistanceFromLine.ts should correct this issue:

    if (d1 === 0 || d2 === 0) {
        // point located at the exact same place as lineStart or lineEnd
        return 0;
    }
    if (d3 === 0) {
        return d1; // lineStart and lineEnd are the same - return point-to-point distance
    }

I've written the following test. They fail with the current version of the code but they pass with the above fix:

   it('https://github.com/manuelbieh/geolib/issues/227 - Point === startLine', () => {
        expect(
            getDistanceFromLine(
                {
                    latitude: 53,
                    longitude: 5,
                },
                {
                    latitude: 53,
                    longitude: 5,
                },
                {
                    latitude: 53,
                    longitude: 6,
                }
            )
        ).toEqual(0);
    });

    // The test below does not fail but it's by accident.
    it('https://github.com/manuelbieh/geolib/issues/227 - Point === endLine', () => {
        expect(
            getDistanceFromLine(
                {
                    latitude: 53,
                    longitude: 5,
                },
                {
                    latitude: 53,
                    longitude: 6,
                },
                {
                    latitude: 53,
                    longitude: 5,
                }
            )
        ).toEqual(0);
    });

    it('https://github.com/manuelbieh/geolib/issues/227 - startLine === endLine', () => {
        expect(
            getDistanceFromLine(
                {
                    latitude: 53,
                    longitude: 6,
                },
                {
                    latitude: 53,
                    longitude: 5,
                },
                {
                    latitude: 53,
                    longitude: 5,
                }
            )
        ).not.toBeNaN();
    });

I don't have the access right to push this modification on a new branch on your repo.

@timvahlbrock
Copy link
Contributor

@redcic75 Ran into the same issue as you. Great that you found a fix. Usually you can fork the project and create a fix on you own repository. You can then create a PR from your fork to this repo and link that PR with this issue. But if you want I can do that for you.

@timvahlbrock
Copy link
Contributor

What I additionally saw is, that a related issue was reopened: #129

@redcic75
Copy link
Author

@timvahlbrock : Thanks for your message. I'm working on other subjects at the time but feel free to use my proposed code to create the PR if you need it.

@redcic75 Ran into the same issue as you. Great that you found a fix. Usually you can fork the project and create a fix on you own repository. You can then create a PR from your fork to this repo and link that PR with this issue. But if you want I can do that for you.

@timvahlbrock
Copy link
Contributor

@timvahlbrock : Thanks for your message. I'm working on other subjects at the time but feel free to use my proposed code to create the PR if you need it.

@redcic75 Ran into the same issue as you. Great that you found a fix. Usually you can fork the project and create a fix on you own repository. You can then create a PR from your fork to this repo and link that PR with this issue. But if you want I can do that for you.

Already happened. Opened it for the other Issue, as there was more traffic. Did some cleanup and mentioned you as co-author.

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

No branches or pull requests

2 participants