Add support for more flexible goal specification#330
Conversation
sea-bass
left a comment
There was a problem hiding this comment.
This is a really cool addition!
Two comments:
- (Un)fortunately, PDDLStream lets you create derived predicates so that you do not need to directly specify stuff like this. See for example:
However, I think it's nice to have the option to do this as well. I assume that based on how PDDLStream works, this functionality would also hold for things like Imply as well?
- You will need to support this in the ROS interface as well. See
src/pyrobosim/pyrobosim_ros/examples/demo_pddl_goal_publisher.py.
The tricky thing about this is that the ROS messages GoalSpecification and GoalPredicate don't currently have a mechanism to specify such complex expressions ... hence the derived predicates.
So I might say, unless the ROS interface is also figured out, it might be better to encourage users to create derived predicates instead?
| PARAM1 = "?o" | ||
| PARAM2 = "?o" |
There was a problem hiding this comment.
I would probably prefer you just used the literal "?o" directly in the expression below
... and actually, it should be ?l or ?loc since the Has derived predicate accepts Location type and not Object.
| """ | ||
|
|
||
| def process_element(elem): | ||
| """Helper function to process individual elements recursively""" |
There was a problem hiding this comment.
Need to add description of what the input and output arguments are. See other docstrings.
| ): # Skip variables (starting with ?) | ||
| # Check if string corresponds to a named entity | ||
| entity = world.get_entity_by_name(elem) | ||
| return entity if entity else elem |
There was a problem hiding this comment.
| return entity if entity else elem | |
| return entity or elem |
| if j > 0 and isinstance(elem, str): | ||
| entity = world.get_entity_by_name(elem) | ||
| if entity: | ||
| replace_goal_literal_tuple(goal_literals, i, j, entity) |
There was a problem hiding this comment.
Does this mean that the replace_goal_literal_tuple function can be removed with this change?
Seems like your new function replaces it completely, as this was the only place in the code base it was being used, so you can remove it.
I'd also suggest renaming it from process_element() to something more descriptive, like populate_entities() - - this might better indicate what it is the function does.
| goal_literals = [ | ||
| Exists( | ||
| [PARAM1], | ||
| And( | ||
| ("Has", PARAM1, "banana"), | ||
| ("Has", PARAM1, "apple1"), | ||
| ("Has", PARAM1, "water1"), | ||
| ), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Might be nice to add a comment here.
The current process_goal_specification function can only process simple goal specifications.
https://github.com/sea-bass/pyrobosim/blob/ada1d009830f5ba07494a3a25c2775034b7e383a/pyrobosim/pyrobosim/planning/pddlstream/utils.py#L128C1-L143C1
This pull request updates the process_goal_specification function to process more flexible goal specifications, including constructs such as Exists and Imply.
c9e8fd8#diff-03e61f646fe833861aaa6e076241f719d3854c411ea0ba8f531552096bab6098R135-R148