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.

Sign up for GitHub

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

Jump to bottom

Add findPathSync to find & calculate a path then return the result directly #62

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add findPathSync to find & calculate a path then return the result directly #62

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3e821c0
some semicolon housekeeping and tests for enableSync mode
Apr 11, 2018
ad5259a
add findPathSync method with tests and ts definitions
Apr 11, 2018
db6a9fd
update docs
Apr 11, 2018
c7a3a76
remove redundant describe
Apr 11, 2018
28122c3
remive redundant comment
Apr 11, 2018
94dd38e
build
Apr 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -63,6 +63,9 @@ easystar.setAdditionalPointCost(x, y, cost);
easystar.setTileCost(tileType, multiplicativeCost);
```
```javascript
easystar.findPathSync(startX, startY, endX, endY);
```
```javascript
easystar.enableSync();
```
```javascript
Expand Down
46 changes: 29 additions & 17 deletions bin/easystar-0.4.3.js
View file
Open in desktop
Expand Up @@ -150,8 +150,8 @@ var EasyStar =
/**
* Sets the tile cost for a particular tile type.
*
* @param {Number} The tile type to set the cost for.
* @param {Number} The multiplicative cost associated with the given tile.
* @param {Number} tileType The tile type to set the cost for.
* @param {Number} cost The multiplicative cost associated with the given tile.
**/
this.setTileCost = function (tileType, cost) {
costMap[tileType] = cost;
Expand All @@ -163,7 +163,7 @@ var EasyStar =
*
* @param {Number} x The x value of the point to cost.
* @param {Number} y The y value of the point to cost.
* @param {Number} The multiplicative cost associated with the given point.
* @param {Number} cost The multiplicative cost associated with the given point.
**/
this.setAdditionalPointCost = function (x, y, cost) {
if (pointsToCost[y] === undefined) {
Expand Down Expand Up @@ -272,6 +272,29 @@ var EasyStar =
pointsToAvoid = {};
};

/**
* Find a path and calculate synchronously
*
* @param {Number} startX
* @param {Number} startY
* @param {Number} endX
* @param {Number} endY
* @returns {Array<{ x: Number, y: Number}|null>}
*/
this.findPathSync = function (startX, startY, endX, endY) {
var result = null;

syncEnabled = true;

this.findPath(startX, startY, endX, endY, function (path) {
result = path;
});

this.calculate();

return result;
};

/**
* Find a path.
*
Expand All @@ -285,17 +308,6 @@ var EasyStar =
*
**/
this.findPath = function (startX, startY, endX, endY, callback) {
// Wraps the callback for sync vs async logic
var callbackWrapper = function (result) {
if (syncEnabled) {
callback(result);
} else {
setTimeout(function () {
callback(result);
});
}
};

// No acceptable tiles were set
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely the most controversial change ☝️ , I'm happy to discuss this, besides from deferring the callback onto the next execution stack, I'm not sure what is gained from wrapping this in a setTimeout for the asynchronous mode.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is important as without it we would be introducing zalgo bugs. The fact that the callback is asynchronous should be consistent.

See here: http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a really interesting read, not something that I've ever come across. I'm not certain I 100% understand it in our context, but I'm more than happy to revert this.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our context it is relevant because

If you have an API which takes a callback,
and sometimes that callback is called immediately,
and other times that callback is called at some point in the future,
then you will render any code using this API impossible to reason about, and cause the release of Zalgo.

If sync is not enabled, we want to consistently call the callback asynchronously. Removing the setTimeout means the callback could be called immediately, making the callback behavior inconsistent.

if (acceptableTiles === undefined) {
throw new Error("You can't set a path without first calling setAcceptableTiles() on EasyStar.");
Expand All @@ -312,7 +324,7 @@ var EasyStar =

// Start and end are the same tile.
if (startX === endX && startY === endY) {
callbackWrapper([]);
callback([]);
return;
}

Expand All @@ -327,7 +339,7 @@ var EasyStar =
}

if (isAcceptable === false) {
callbackWrapper(null);
callback(null);
return;
}

Expand All @@ -342,7 +354,7 @@ var EasyStar =
instance.startY = startY;
instance.endX = endX;
instance.endY = endY;
instance.callback = callbackWrapper;
instance.callback = callback;

instance.openList.push(coordinateToNode(instance, instance.startX, instance.startY, null, STRAIGHT_COST));

Expand Down
13 changes: 13 additions & 0 deletions index.d.ts
View file
Open in desktop
Expand Up @@ -122,6 +122,19 @@ export class js {
*/
stopAvoidingAllAdditionalPoints(): void

/**
* Find a path synchronously.
*
* @param {Number} startX The X position of the starting point.
* @param {Number} startY The Y position of the starting point.
* @param {Number} endX The X position of the ending point.
* @param {Number} endY The Y position of the ending point.
* is found, or no path is found.
* @return {Array<{ x: Number, y: Number }>|null} The result of easystar.calculate(), a path array or null
*
*/
findPathSync(startX: number, startY: number, endX: number, endY: number): Array<{ x: number, y: number }>|null

/**
* Find a path.
*
Expand Down
96 changes: 54 additions & 42 deletions src/easystar.js
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* Implementation By Bryce Neal (@prettymuchbryce)
**/

var EasyStar = {}
var EasyStar = {};
var Instance = require('./instance');
var Node = require('./node');
var Heap = require('heap');
Expand Down Expand Up @@ -72,14 +72,14 @@ EasyStar.js = function() {
*/
this.enableDiagonals = function() {
diagonalsEnabled = true;
}
};

/**
* Disable diagonal pathfinding.
*/
this.disableDiagonals = function() {
diagonalsEnabled = false;
}
};

/**
* Sets the collision grid that EasyStar uses.
Expand All @@ -103,8 +103,8 @@ EasyStar.js = function() {
/**
* Sets the tile cost for a particular tile type.
*
* @param {Number} The tile type to set the cost for.
* @param {Number} The multiplicative cost associated with the given tile.
* @param {Number} tileType The tile type to set the cost for.
* @param {Number} cost The multiplicative cost associated with the given tile.
**/
this.setTileCost = function(tileType, cost) {
costMap[tileType] = cost;
Expand All @@ -116,7 +116,7 @@ EasyStar.js = function() {
*
* @param {Number} x The x value of the point to cost.
* @param {Number} y The y value of the point to cost.
* @param {Number} The multiplicative cost associated with the given point.
* @param {Number} cost The multiplicative cost associated with the given point.
**/
this.setAdditionalPointCost = function(x, y, cost) {
if (pointsToCost[y] === undefined) {
Expand All @@ -135,14 +135,14 @@ EasyStar.js = function() {
if (pointsToCost[y] !== undefined) {
delete pointsToCost[y][x];
}
}
};

/**
* Remove all additional point costs.
**/
this.removeAllAdditionalPointCosts = function() {
pointsToCost = {};
}
};

/**
* Sets a directional condition on a tile
Expand Down Expand Up @@ -225,6 +225,29 @@ EasyStar.js = function() {
pointsToAvoid = {};
};

/**
* Find a path and calculate synchronously
*
* @param {Number} startX
* @param {Number} startY
* @param {Number} endX
* @param {Number} endY
* @returns {Array<{ x: Number, y: Number}|null>}
*/
this.findPathSync = function(startX, startY, endX, endY) {
var result = null;

syncEnabled = true;

this.findPath(startX, startY, endX, endY, function (path) {
result = path;
});

this.calculate();

return result;
};

/**
* Find a path.
*
Expand All @@ -238,17 +261,6 @@ EasyStar.js = function() {
*
**/
this.findPath = function(startX, startY, endX, endY, callback) {
// Wraps the callback for sync vs async logic
var callbackWrapper = function(result) {
if (syncEnabled) {
callback(result);
} else {
setTimeout(function() {
callback(result);
});
}
}

// No acceptable tiles were set
if (acceptableTiles === undefined) {
throw new Error("You can't set a path without first calling setAcceptableTiles() on EasyStar.");
Expand All @@ -267,7 +279,7 @@ EasyStar.js = function() {

// Start and end are the same tile.
if (startX===endX && startY===endY) {
callbackWrapper([]);
callback([]);
return;
}

Expand All @@ -282,7 +294,7 @@ EasyStar.js = function() {
}

if (isAcceptable === false) {
callbackWrapper(null);
callback(null);
return;
}

Expand All @@ -297,7 +309,7 @@ EasyStar.js = function() {
instance.startY = startY;
instance.endX = endX;
instance.endY = endY;
instance.callback = callbackWrapper;
instance.callback = callback;

instance.openList.push(coordinateToNode(instance, instance.startX,
instance.startY, null, STRAIGHT_COST));
Expand Down Expand Up @@ -469,13 +481,13 @@ EasyStar.js = function() {
var isTileWalkable = function(collisionGrid, acceptableTiles, x, y, sourceNode) {
var directionalCondition = directionalConditions[y] && directionalConditions[y][x];
if (directionalCondition) {
var direction = calculateDirection(sourceNode.x - x, sourceNode.y - y)
var direction = calculateDirection(sourceNode.x - x, sourceNode.y - y);
var directionIncluded = function () {
for (var i = 0; i < directionalCondition.length; i++) {
if (directionalCondition[i] === direction) return true
}
return false
}
};
if (!directionIncluded()) return false
}
for (var i = 0; i < acceptableTiles.length; i++) {
Expand All @@ -493,14 +505,14 @@ EasyStar.js = function() {
* -1, 1 | 0, 1 | 1, 1
*/
var calculateDirection = function (diffX, diffY) {
if (diffX === 0 && diffY === -1) return EasyStar.TOP
else if (diffX === 1 && diffY === -1) return EasyStar.TOP_RIGHT
else if (diffX === 1 && diffY === 0) return EasyStar.RIGHT
else if (diffX === 1 && diffY === 1) return EasyStar.BOTTOM_RIGHT
else if (diffX === 0 && diffY === 1) return EasyStar.BOTTOM
else if (diffX === -1 && diffY === 1) return EasyStar.BOTTOM_LEFT
else if (diffX === -1 && diffY === 0) return EasyStar.LEFT
else if (diffX === -1 && diffY === -1) return EasyStar.TOP_LEFT
if (diffX === 0 && diffY === -1) return EasyStar.TOP;
else if (diffX === 1 && diffY === -1) return EasyStar.TOP_RIGHT;
else if (diffX === 1 && diffY === 0) return EasyStar.RIGHT;
else if (diffX === 1 && diffY === 1) return EasyStar.BOTTOM_RIGHT;
else if (diffX === 0 && diffY === 1) return EasyStar.BOTTOM;
else if (diffX === -1 && diffY === 1) return EasyStar.BOTTOM_LEFT;
else if (diffX === -1 && diffY === 0) return EasyStar.LEFT;
else if (diffX === -1 && diffY === -1) return EasyStar.TOP_LEFT;
throw new Error('These differences are not valid: ' + diffX + ', ' + diffY)
};

Expand Down Expand Up @@ -544,13 +556,13 @@ EasyStar.js = function() {
return (dx + dy);
}
};
}

EasyStar.TOP = 'TOP'
EasyStar.TOP_RIGHT = 'TOP_RIGHT'
EasyStar.RIGHT = 'RIGHT'
EasyStar.BOTTOM_RIGHT = 'BOTTOM_RIGHT'
EasyStar.BOTTOM = 'BOTTOM'
EasyStar.BOTTOM_LEFT = 'BOTTOM_LEFT'
EasyStar.LEFT = 'LEFT'
EasyStar.TOP_LEFT = 'TOP_LEFT'
};

EasyStar.TOP = 'TOP';
EasyStar.TOP_RIGHT = 'TOP_RIGHT';
EasyStar.RIGHT = 'RIGHT';
EasyStar.BOTTOM_RIGHT = 'BOTTOM_RIGHT';
EasyStar.BOTTOM = 'BOTTOM';
EasyStar.BOTTOM_LEFT = 'BOTTOM_LEFT';
EasyStar.LEFT = 'LEFT';
EasyStar.TOP_LEFT = 'TOP_LEFT';