tisoap / react-flow-smart-edge

Custom Edge for React Flow that never intersects with other nodes
https://tisoap.github.io/react-flow-smart-edge/
MIT License
242 stars 26 forks source link

Allow developers to change the path finding function #14

Closed lichenglu closed 2 years ago

lichenglu commented 2 years ago

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., https://github.com/qiao/PathFinding.js/pull/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 commented 2 years ago

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

lichenglu commented 2 years ago

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

tisoap commented 2 years ago

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 commented 2 years ago

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