-
Notifications
You must be signed in to change notification settings - Fork 30
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
Clean up robot/util. #588
Clean up robot/util. #588
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.
Found some time for a quick review! Broad strokes seem solid, although I'd also like to see a corresponding libada PR before merging.
|
||
// Create planning problem. | ||
auto castedGoalState | ||
= static_cast<const statespace::dart::MetaSkeletonStateSpace::State*>( |
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.
For some reason, I thought that we take the goal state as an Eigen::VectorXd
instead. Is there a reason we take a State
? What happens if this isn't actually a MetaSkeletonStateSpace::State
?
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.
Yeah, this is something I noticed as well. I'm not sure why we decided to take StateSpace::State*
instead of MetaSkeletonStateSpace::State*
. Since we're working with ConcreteRobot
we must be working with MetaSkeletonStateSpace::State*
(unless the user is doing something very stupid).
I agree that this design decision is a little funky (especially considering that we take a metaSkeleton
as another argument), but changing it would break a lot of things just because of type issues. Maybe we can file an issue to clean this up later?
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.
@aditya-vk does this seem ok for now?
@brianhou I tried to handle #447 by enforcing that any DART planner (and only a DART planner) has to satisfy these two invariants when
The only DART planner class that didn't need to be updated was the VFP offset planner (since the core VFP code does both of these anyway). LMK what you think of this solution: if we're happy with it, I can document this new invariant for DART planners somewhere. This also means we don't have to worry about locking or saving state anywhere in Also, for what it's worth I tested |
…nto sniyaz_clean_util
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.
Seems generally fine to me, but would like @aditya-vk's eyes on this too.
Looks good to me. Has it been tested through |
@aditya-vk Yup! The only other thing we're waiting on are the corresponding ADA changes. I'll chat with @egordon about those. |
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.
LGTM
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.
Thanks for the checks with ADA and HERB!
This PR cleans up
robot/util
by deleting all of theplanTo*
methods. This requires us to rewrite the corresponding methods inConcreteRobot
andConcreteManipulator
to reroute through the new Planner API.This PR also removes all CRRT functionality in both
ConcreteRobot
andConcreteManipulator
. This will be added back later once we actually decide how to integrate CRRT and AIKIDO. None of our demo code should rely on CRRT, so this is fine.NOTE: This PR depends on #587 by @brianhou, due to the new planner adapter.
Before creating a pull request
make format
Before merging a pull request
CHANGELOG.md