-
Notifications
You must be signed in to change notification settings - Fork 63
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 GoToPlace
to allow finding nearest spot
#308
Conversation
This commit adds support for selecting the nearest spot on the same floor. This behaviour is convenient when looking at things from a cancellation perspective. This commit depends on open-rmf/rmf_task#101 Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
I have not implemented flexible constraints here. If we really need it can do it in a follow up pr. Signed-off-by: Arjo Chakravarty <[email protected]>
This pr depends on: * open-rmf/rmf_task#101 * open-rmf/rmf_ros2#308 You can ask a robot to go to its nearest spot using the newly added script. Signed-off-by: Arjo Chakravarty <[email protected]>
Thanks for working on this! Tried it out with the demos script and it's working for me 👍 I see that having the final selected destination to be on the same map is a requirement in the code itself because of the lack of API for constraints. Right now it seems like if all the locations provided are on a different map from the robot, the first place provided will be the destination. Should we not make this behaviour a default and allow the robot to take lifts? Or is this only temporary until the constraints API are added in? |
Signed-off-by: Michael X. Grey <[email protected]>
Signed-off-by: Michael X. Grey <[email protected]>
@@ -67,8 +69,20 @@ void add_patrol( | |||
std::make_move_iterator(place.errors.begin()), | |||
std::make_move_iterator(place.errors.end())); | |||
} | |||
auto desc = GoToPlace::Description::make_with_multiple(goals); | |||
auto desc = GoToPlace::Description::make_for_one_of(goals); | |||
return {desc, errors}; |
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.
return {desc, errors}; |
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.
Move this line below, otherwise the description would return before accounting for the constraints
desc->prefer_same_map(true); | ||
} | ||
} | ||
} |
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.
} | |
} | |
return {desc, errors}; |
{ | ||
for (const auto& constraint : constraints.value()) | ||
{ | ||
if (constraint["type"].get<std::string>() == "prefer_same_map") |
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.
Constraint property should be category
instead of type
according to the schema
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.
I can't believe I messed up my own schema.
Signed-off-by: Michael X. Grey <[email protected]>
Thanks for catching those constraint parsing errors @xiyuoh . I've made the fixes and run simulation tests to confirm that the |
I've opened a related PR for |
On another note for code readability's sake, should we move the GoToPlace description out of |
To keep the diff down and get this merged quickly let's keep it as-is, but I'm open to shuffling that around if we ever touch this code again. |
Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Michael X. Grey <[email protected]> Co-authored-by: Michael X. Grey <[email protected]>
This commit adds a stupidly simple parking spot system. It ensures the next location a robot goes to is not occupied. It also publishes currently free parking spots. When combined with #308 it allows a robot to move to the nearest free spot instead of the nearest spot only. note: This PR still needs more testing Depends on: - open-rmf/rmf_internal_msgs#63 Known issues: - When the robots start in simulation not all robots seem to be running the idle task, hence some robot dont end up claiming parking spots. Signed-off-by: Arjo Chakravarty <[email protected]>
This commit adds a stupidly simple parking spot system. It ensures the next location a robot goes to is not occupied. It also publishes currently free parking spots. When combined with #308 it allows a robot to move to the nearest free spot instead of the nearest spot only. note: This PR still needs more testing Depends on: - open-rmf/rmf_internal_msgs#63 Known issues: - When the robots start in simulation not all robots seem to be running the idle task, hence some robot dont end up claiming parking spots. Signed-off-by: Arjo Chakravarty <[email protected]>
New feature implementation
Implemented feature
This commit adds support for selecting the nearest spot on the same
floor. This behaviour is convenient when looking at things from a cancellation perspective, one may want to go to the nearest parking spot for instance.
This commit depends on open-rmf/rmf_task#101
Note: While the schema supports contraints, we don't yet have an API to express such constraints, hence by default it will always use the same floor constraint.