-
Notifications
You must be signed in to change notification settings - Fork 22
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
Update Graph class and add basic search algorithms #35
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nitpicky comments about docstrings and names.
Implementation looks solid so far, and I can't think of anything that would impact downstream planning approaches that would build such a graph.
Looking forward to these graph visualizations in the PRM implementation! Do take a look at how the RRT one puts stuff into Meshcat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted some things, and I think I got all of your very valid nits.
For #34.
A* and DFS both work,I tried to not completely obliterate the Graph class's interface so as to avoid having to make significant changes to RRT. The weirdest thing is having to have a dictionary of nodes to themselves to support finding actual node objects in the Graph. I guess this is why Rust has a borrow checker.
In summary,
Might need to think about how to make this cleaner...