fix: Correct cost transition #99
Merged
Conversation
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
ppinchuk
added
p-critical
Member
Author
castelao
force-pushed
the
fix/cost_step
branch
from
1807871 to
9c5e1d3
Compare
The cost value read is the average value for the grid point. Before we were simplifying and just taking the next grid point value as the transition. The correct estimate is half the path is along the origin gridpoint, plus half of the target grid point to reach the center of the target gridoint, therefore, the transition wheight should be the average between the current grid point cost and the target gridpoint cost. Note that the same principle is valid for diagonals, it is still the average of both values, but we have to scale for the longer distance along the diagonal, thus a sqrt(2) factor.
Correcting expected values to include the diagonal factor.
Updating expected solution to consider diagonal factor
Correcting expected answer to consider the diagonal effect. In this case, two successive diagonal moves for both cases, despite opposite dirrections.
Correcting expected cost to include 3 diagonal moves.
Before we didn't consider the diagonal factor, so it made sense to just add the full grid point value. Now, we have to add the average between the two points plus the diagonal factor when relevant.
Comparison with scikit-image should be with MCP_Geometric instead of MCP, since MCP does not penalize diagonal movements with longer distance while MCP_Geometric does.
The original pathfinder accepted only u64, so we truncated the f32 costs. Let's allow using truncated value of sqrt(2) for now.
castelao
force-pushed
the
fix/cost_step
branch
from
3bdeadb to
68078b6
Compare
Member
Author
|
@ppinchuk , this is ready for review. I'm postponing two tests that require corrections for later. But it is better to have everything else working now. |
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.
The cost value read is the average value for the grid point. Before we were simplifying and just taking the next grid point value as the transition.
The correct estimate is half the path is along the origin gridpoint, plus half of the target grid point to reach the center of the target gridoint, therefore, the transition wheight should be the average between the current grid point cost and the target gridpoint cost.
Note that the same principle is valid for diagonals, it is still the average of both values, but we have to scale for the longer distance along the diagonal, thus a sqrt(2) factor.