Fix incorrect calculation in get distance for diagonals#78
Open
mikeparker wants to merge 2 commits intoprettymuchbryce:masterfrom
Open
Fix incorrect calculation in get distance for diagonals#78mikeparker wants to merge 2 commits intoprettymuchbryce:masterfrom
mikeparker wants to merge 2 commits intoprettymuchbryce:masterfrom
Conversation
mikeparker
changed the title
aJamDonut
reviewed
Jan 23, 2024
src/easystar.js
Outdated
| return DIAGONAL_COST * dx + (dy-dx); | ||
| } else { | ||
| return DIAGONAL_COST * dy + dx; | ||
| return DIAGONAL_COST * dy + (dx=dy); |
There was a problem hiding this comment.
@mikeparker sorry i know its old, i implemented this change myself also but wanted to ask... why was it dx=dy
Author
There was a problem hiding this comment.
Yeah it's pretty old, that looks like a typo to me, it's probably supposed to be (dx-dy)
If I was better with javascript this is exactly the sort of thing a unit test should check for.
Author
There was a problem hiding this comment.
Yes that makes sense thinking about it. If dx < dy then you travel all the X distance via diagonals then the remainer dy via vertical moves.
If its the other way around then you travel all the Y distance via diagonals then the remainder dx via horizontal moves.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I am implementing the ability to return incomplete paths and noticed this calculation was off.
For example if a node is 1x,1y away from the target this function incorrectly returns a distance of 2.4 instead of 1.4.
Another example, if a node is 4x,2y away, this should move diagonally for 2y which will also result in 2x movement, then the remaining 2x, so it should be
2*DIAGONAL_COST + 2*STRAIGHT_COSTrather than2*DIAGONAL_COST + 4*STRAIGHT_COST