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

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
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
2 changes: 1 addition & 1 deletion bin/easystar-0.4.3.min.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.