Skip already-visited tiles during radius calculation (approx. 5% fewer tiles) #25
Conversation
This file contains 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
…r tiles) We have found the shortest path to a given tile when it is dequeued from the priority queue for the first time. There is no shorter path because all other tiles in the priority queue have the same or equal cost from the starting tile (Djikstra). To keep the implementation simple, this commit uses the existing `closed` array to mark tiles that have been visited. We can also remove the check for `currentVector.equals(start)` because the tile for `start` will already have been put into `closed`. The tests pass. The number of visited tiles is slightly lower in one of the Radius test cases (added logging to measure this). The random MapGenerator test perhaps shows a more representative impact. I ran it one time before and after this commit, and looked at the average number of tiles visited in `calculateRadius` during each run: before = 395, after = 372. Of course, there is randomness and it shows we can expect about 5% savings in tiles visited.
ide
commented
May 19, 2024
| continue; | ||
| } | ||
| closed[index] = true; |
There was a problem hiding this comment.
This is the only line that matters. The rest of this PR just moves around variables.
|
Amazing, thanks for the PR and the improvement! Radius calculation is finicky, especially with Trenches that take movement points on traversal in/out but the tests should catch all of the edge cases these days. I believe in a past version there was an issue where a shorter path was discovered later on, but that was before I made use of the priority queue. |
cpojer
pushed a commit
that referenced
this pull request
May 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have found the shortest path to a given tile when it is dequeued from the priority queue for the first time. There is no shorter path because all other tiles in the priority queue have the same or greater cost from the starting tile (Djikstra).
To keep the implementation simple, this commit uses the existing
closedarray to mark tiles that have been visited. We can also remove the check forcurrentVector.equals(start)because the tile forstartwill already have been put intoclosed.The tests pass. The number of visited tiles is slightly lower in one of the Radius test cases (added logging to measure this).
The random MapGenerator test perhaps shows a more representative impact. I ran it one time before and after this commit, and looked at the average number of tiles visited in
calculateRadiusduring each run: before = 395, after = 372. Of course, there is randomness and it shows we can expect about 5% savings in tiles visited.