qiao
add `allowCrossDiagnal` option
This option allows movements like this:
Note this is the desired behavior when it's hostile creatures instead of walls. When this option is used, usually wall corners are always connected.
This PR only changed the AStar finder - if you think this is a good idea, I can modify other finders and their respective comments/documentations as well.
@yyx990803 Hmm... I had the same feature in my own pathfinding library, but I was considering ditching it, because it didn't sound much useful to me. Can you mention some games/demos where agents can make such moves through walls ?
Pixel Dungeon is one the best roguelikes on Android and it allows this behavior. The image is probably a bit misleading, since this is more about being able to "squeeze" through enemies rather than through walls.
This PR introduces another flag which adds more complexity. I'm not sure how useful is this feature of allowing squeezing through obstacles? More votes are needed before this can be accepted.
My original image was probably misleading, I've updated it to use hostile creatures as the example - in most roguelikes, the player expects to be able to move like that.
I stumbled upon this PR while trying to solve a similar issue in my own application. While it seems useful at first glance, I tend to agree with @imor and @Yonaba that it adds unnecessary complexity and is not necessary functionality for the core pathfinding library. It's also somewhat unintuitive to think of it being used with dontCrossCorners: true.
It seems like a very context specific issue that should be handled by the application on a level above the pathfinding library.
For example, in my game, I've only had to deal with this issue when the player attempts to move short distances when surrounded by different configurations of enemies. (For longer distance movements, the issue becomes less apparent, and it is perfectly natural for the player to pathfind around the enemies). I handle it with specialized player action code, and avoid invoking the pathfinder completely.
@doggan while I don't really care if this PR gets merged or not, I'd argue otherwise:
- It is "unnecessary" when you can implement a desired behavior without forking the library. Imagine when the player is surrounded by enemies and walls. I expected to be able to escape by moving diagonally through the enemies (even with long distance clicks), but instead I'm stuck with the current implementation. In my case, adding this option is the only way to get the desired behavior. And note this is not only for me; this is how the player moves in most if not all roguelike games.
- I'd only label something as adding "complexity" when it affects the existing use cases. But this change does not. You can keep on using the lib perfectly fine without even knowing this change ever happened, and the change to the internal logic is also minimal and easy to grasp.
In short, I don't think it's fair to label the PR as adding "unnecessary complexity".
IMO, the best way to handle it is to introduce a new flag called cornerCuttingStrategy that can be used to represent both the dontCrossCorners flag and the allowCrossDiagonal flag.
We can have 3 enumerations to represent the strategies,
-
allow diagonal movement whenever the diagonal node is walkable.
In the following diagram, Node A can reach node B even if the adjacent two nodes are blocked.
+-----+-----+ | | | | A |Block| | | | +-----------+ | | | |Block| B | | | | +-----+-----+ -
allow diagonal movement when one or more adjacent nodes are walkable.
In this case, in the following diagram Node A can NOT reach node B, since the adjacent two nodes are all blocked.
+-----+-----+ | | | | A |Block| | | | +-----------+ | | | |Block| B | | | | +-----+-----+While In the following diagram, Node A can reach node B.
+-----+-----+ | | | | A |Block| | | | +-----------+ | | | | | B | | | | +-----+-----+ -
allow diagonal movement only when both adjacent two nodes are walkable.
In this case, Node A can reach node B only when the following layout is satisfied.
+-----+-----+ | | | | A | | | | | +-----------+ | | | | | B | | | | +-----+-----+
@yyx990803 Yes it will be. And I think the current API is messy and needs to be redesigned. Such as
- Using a single function with a object option for users to find the path instead of letting them instantiating the finder class and grid class manually. And actually, there's no point to define finder classes (These classes only has one method exposed to the user.) and they should be functions instead. e.g.
var path = PF.findPath({
strategy: PF.Strategies.AStar,
start: [0, 0],
end: [4, 5],
matrix: [
[0, 0, 0, 0, 1],
[1, 0, 0, 0, 1],
....
],
heuristic: PF.Heuristic.Manhattan,
allowDiagonal: true,
cornerCuttingStrategy: ...
})
- The return value for a non-existing path should be null instead of an empty array.
+1 for the new API. Also would be nice to have the default strategy/heuristic (probably AStar + Manhattan)
Enum solution and new API look good.
@yyx990803 Sorry if my original post came off as too harsh - I did not mean to label your PR as being unnecessary :pensive:. I only intended to say that 1). the proposed flag felt unintuitive when used in combination with other flags (which is addressed by the above enum solution), and 2). depending on the game, this problem may be addressable in the game-specific action code.
Just to refine this I propose following changes. Depending upon the type of obstacles there are three cases when moving diagonally -
A) No obstacles in diagonal boxes
+-----+-----+
| | |
|FROM | |
| | |
+-----+-----+
| | |
| | TO |
| | |
+-----+-----+
B) One obstacle in one of the diagonal boxes
+-----+-----+ +-----+-----+
| | | | | |
|FROM |BLOCK| |FROM | |
| | | | | |
+-----+-----+ OR +-----+-----+
| | | | | |
| | TO | |BLOCK| TO |
| | | | | |
+-----+-----+ +-----+-----+
C) Two obstacles in the diagonal boxes
+-----+-----+
| | |
|FROM |BLOCK|
| | |
+-----+-----+
| | |
|BLOCK| TO |
| | |
+-----+-----+
- The name of the enum should be DiagonalMovementStrategy and name of the option should be diagonalMovementStrategy
- The values of this enum should be -
- AlwaysMoveDiagonally - Movement will be allowed in cases A, B and C
- NeverMoveDiagonally - Movement will not be allowed in any of the cases A, B or C
- MoveDiagonallyIfOneObstacle - Movement will be allowed in cases A and B but not in C
- MoveDiagonallyIfNoObstacles - Movement will be allowed in case A but not in case B or C
Of all the possible combinations of allowing or disallowing A, B or C the above listed cases were the only sensible choices. Forbidden cases are listed below -
- Movement allowed in case A and C but not in B
- Movement allowed in case B and C but not in A
- Movement allowed only in case B
- Movement allowed only in case C
@qiao Instead of making it a breaking change right now can't we just accept a new option which, if present, overrides the behaviour of allowDiagonal and dontCrossCorners options and mark these older options as deprecated at the same time? Like this e.g. -
var finder = new PF.AStarFinder({
allowDiagonal: true,
dontCrossCorners: true,
diagonalMovementStrategy: DiagonalMovementStrategy.AlwaysMoveDiagonally
});
This will allow for a gentler upgrade for users.
With e5b16787042ffedf6b270078a05a88bbc9b8dd4b, 40d157905e28d48b4f894238cd3e6655d1a5216c and efb5c33ed6aafe453f531be5a4f8560e4c09869c it is now possible to specify the diagonalMovement option to control the type of diagonal movement allowed. The possible values are Always, Never, IfAtMostOneObstacle and OnlyWhenNoObstacles. e.g.
var finder = new PF.JumpPointFinder({
diagonalMovement: PF.DiagonalMovement.Always
});
These commits also add support for the diagonalMovement to the JumpPointFinder. The OrthogonalJumpPointFinder is no longer exposed directly but is used behind the scenes if the diagonalMovement is set to Never.
The old options allowDiagonal and dontCrossCorners will work if specified but will be marked deprecated in the docs. diagonalMovement is the new unified option to specify diagonal movement. If both old and new options are specified the old ones will take preference.
Docs and the demo are yet to be updated. A lot of new tests also need to be added.