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

Allow developers to change the path finding function #14

Closed
lichenglu opened this issue Mar 10, 2022 · 4 comments · Fixed by #19
Closed

Allow developers to change the path finding function #14

lichenglu opened this issue Mar 10, 2022 · 4 comments · Fixed by #19
Labels
enhancement New feature or request

Comments

@lichenglu
Copy link

First of all, thanks for the great package! It saves me tons of time.

Originally, the package did not seem to be "smart" on my end (still using the BezierEdge from react-flow). I then found out that the length of smoothedPath for me is always smaller or equal to 2, which fallbacks to the edges of react-flow (see codes here). Then I was thinking it cannot be that smoothedPath is always that case. It turns out there is a bug in the dependency pathfinding, where no coordinate will be recorded if coming across a block (start and end coordinate will not be updated either).

function smoothenPath(grid, path) {
        ....
        blocked = false;
        ....
        if (blocked) {
            lastValidCoord = path[i - 1]; // THIS IS NOT DEFINED AND CAUSE AN INTERNAL ERROR
            newPath.push(lastValidCoord);
            sx = lastValidCoord[0];
            sy = lastValidCoord[1];
        }
    }
    
    newPath.push([x1, y1]);

    return newPath;
}

The fix is easy but it seems that no one is maintaining pathfinding anymore (e.g., qiao/PathFinding.js#192).

Suggestions

  1. Maybe react-flow-smart-edge should use a fork of pathfinding to have known bugs fixed?
  2. Even better, maybe we can find a more up-to-date package related to graph theory as this might be maintainable in the long run?
@tisoap
Copy link
Owner

tisoap commented Mar 11, 2022

Hey @lichenglu thanks for pointing it out! When I choose pathfinding as a dependency I didn't care that it wasn't being maintained anymore, since "path finding" is a solved problem, but turns out there's still bugs in it.

I don't feel confident in forking/maintaining it, which leaves the option to find another up to date dependency

@tisoap tisoap added bug Something isn't working help wanted Extra attention is needed labels Mar 11, 2022
@lichenglu
Copy link
Author

lichenglu commented Mar 11, 2022

Hey @lichenglu thanks for pointing it out! When I choose pathfinding as a dependency I didn't care that it wasn't being maintained anymore, since "path finding" is a solved problem, but turns out there's still bugs in it.
I don't feel confident in forking/maintaining it, which leaves the option to find another up to date dependency

That makes sense as I would expect it to be a mature enough problem with robust solutions. Do you have an alternative in mind? I have briefly looked around and it seems https://github.com/prettymuchbryce/easystarjs can be used to easily replace pathfinding. However, easystarjs seems only support the A* algorithm and I noticed in other issues that JumpPointFinder might be needed in the future.

P.S. I would love to help revamp the use of path finding if you don't mind determining an alternative package

@lichenglu lichenglu reopened this Mar 12, 2022
@tisoap
Copy link
Owner

tisoap commented Apr 3, 2022

Thinking on how to circumvent this without putting the burden on me to update/change the path finding dependency, I'm planning to create a factory function. Developers can "build" their own versions of a smart edge passing down configurations to this factory function, and I'll expose the option to change the default path finding function.

@tisoap tisoap changed the title Smart edge might not work when path smoothening is blocked Allow developers to change the path finding function Apr 3, 2022
@tisoap tisoap added enhancement New feature or request and removed bug Something isn't working help wanted Extra attention is needed labels Apr 3, 2022
@tisoap tisoap mentioned this issue Apr 10, 2022
@tisoap
Copy link
Owner

tisoap commented Apr 10, 2022

Hey @lichenglu ! I've released a beta version that makes it possible to configure the path finding function using a smartEdgeFactory, could you test if it can be used to solve your use case?

Available to test using the beta tag:

npm install @tisoap/react-flow-smart-edge@beta

New documentation can be found here

EDIT: Version 1.0 released with this feature

Repository owner locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants