manuelbieh / geolib

Zero dependency library to provide some basic geo functions
MIT License
4.21k stars 341 forks source link

getDistanceFromLine is returning NaN #298

Open redcic75 opened 1 year ago

redcic75 commented 1 year ago

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 commented 9 months ago

@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 commented 9 months ago

What I additionally saw is, that a related issue was reopened: https://github.com/manuelbieh/geolib/issues/129

redcic75 commented 9 months ago

@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 commented 9 months ago

@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.