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

Fix/pathfinding #95

Merged
merged 3 commits into from
Sep 24, 2024
Merged

Fix/pathfinding #95

merged 3 commits into from
Sep 24, 2024

Conversation

bal7hazar
Copy link
Collaborator

@bal7hazar bal7hazar commented Sep 16, 2024

Introduced changes

  • Fix pathfinding issue appearing in very rare cases (some children were missed while sorting down the heap)
  • Add a pseudo randomness to prioritise decisions while searching a path
  • Add a printer for the heap to help debugging

@bal7hazar bal7hazar requested a review from glihm September 16, 2024 18:47
Copy link
Contributor

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Nice fix! Some questions:

  1. If we could enhance the printer to use the "_" and "|" that could be better to ensure we have one unified way to represent the maps including in the comment.

Nice to have moved comment doc into interfaces. 👍

/// * The heap is printed
/// # Note
/// * This function is only for debugging purposes
fn print(ref self: Heap<T>);
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could be relocated into the printer module to only be added for testing. Or do you need this to be used in katana?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you that it is not consistent with MapPrinter, but I feel like the Type itself should be responsible of how it should render, so it makes sense to me to have it at the type definition location.
If you want to use it for a pure cairo purpose it seems ok to have it directly in the type, if it is for a Starknet purpose it will fail at the compilation time, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, that's a good vision on that point, thanks for sharing that.
Yes, now sozo displays a warning, before it wasn't. So it makes it easy anyway to see that you've a print in production code.

crates/map/src/helpers/astar.cairo Show resolved Hide resolved
Copy link
Contributor

@glihm glihm left a comment

Choose a reason for hiding this comment

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

Can tackled modifications in other PR as you mentioned you'll rework the crate.

@bal7hazar bal7hazar merged commit b409d01 into main Sep 24, 2024
10 checks passed
@bal7hazar bal7hazar deleted the fix/pathfinding branch September 24, 2024 21:05
bal7hazar added a commit that referenced this pull request Sep 24, 2024
Co-authored-by: notV4l <122404722+notV4l@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants