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

PDDLSimplePlanParser is not able to parse Contingent-FF #255

Closed
dgerod opened this issue Jul 4, 2020 · 9 comments
Closed

PDDLSimplePlanParser is not able to parse Contingent-FF #255

dgerod opened this issue Jul 4, 2020 · 9 comments

Comments

@dgerod
Copy link
Contributor

dgerod commented Jul 4, 2020

The parsed plan from Contingent-FF is not being generated by PDDLSimplePlanParser. And it is normal because the parser is expected a syntax as POPF planner:

0.000: (goto_place b1) [0.001]

While Contingent-FF output to be parser is:

0||0 --- GOTO_PLACE C1 --- SON: 1||-1

And I think that this is not working also with FF, its syntax is:

0: PICKUP_OBJECT C1 V1

In the documentation is written that planner interface works with FF, Metric-FF and Contingent-FF among others. So, my understanding is that all nodes of ROSPlan work with Contingent-FF what is not happening.

Therefore, in my opinion this is a bug. Could you confirm that my understanding is correct? Thanks.

Finally, I would like to talk about the design of "planning_interface" and "parse_plan nodes". Parsing a planner output is hard coupled to the planner itself, so why these two functionalities are split in two different nodes? In my opinion they should be done by only one node that is the "planner_interface", and it should generate a ROSPlan defined format of actions.

In addition, I think that having the "planner_command" parameter for "planner_interface" is not a good idea because it is also hard coupled with implementation of the planner (e.g. POPFPlannerInterface). The only information that is not couple to the planner is the timeout and the directory path where the planner binary is stored, so these should be passed to the node as parameters.

@dgerod
Copy link
Contributor Author

dgerod commented Jul 5, 2020

After checking with more detail I understand that the FFPlannerInterface transforms the output of the FF planner to POPF plan format. So, the PDDLSimplePlanParser is able to parse the results of FF planner. This transformation to POPF plan format is what I named as ROSPlan defined format.

Therefore, the problem of parsing the Contingent-FF is not related to PDDLSimplePlanParser but to the FFPlannerInterface, and it is related to #254. This should be solved by transforming the Contingent-FF plan file to POPF plan format in FFPlannerInterface.

@dgerod
Copy link
Contributor Author

dgerod commented Jul 11, 2020

@dgerod
Copy link
Contributor Author

dgerod commented Aug 1, 2020

Pull request #256.

@bilal9876
Copy link

The parsed plan from Contingent-FF is not being generated by PDDLSimplePlanParser. And it is normal because the parser is expected a syntax as POPF planner:

0.000: (goto_place b1) [0.001]

While Contingent-FF output to be parser is:

0||0 --- GOTO_PLACE C1 --- SON: 1||-1

And I think that this is not working also with FF, its syntax is:

0: PICKUP_OBJECT C1 V1

In the documentation is written that planner interface works with FF, Metric-FF and Contingent-FF among others. So, my understanding is that all nodes of ROSPlan work with Contingent-FF what is not happening.

Therefore, in my opinion this is a bug. Could you confirm that my understanding is correct? Thanks.

Finally, I would like to talk about the design of "planning_interface" and "parse_plan nodes". Parsing a planner output is hard coupled to the planner itself, so why these two functionalities are split in two different nodes? In my opinion they should be done by only one node that is the "planner_interface", and it should generate a ROSPlan defined format of actions.

In addition, I think that having the "planner_command" parameter for "planner_interface" is not a good idea because it is also hard coupled with implementation of the planner (e.g. POPFPlannerInterface). The only information that is not couple to the planner is the timeout and the directory path where the planner binary is stored, so these should be passed to the node as parameters.

@bilal9876
Copy link

in particular, a contingent plan is a tree structure and after the ROSPlan converts it to a ROS message action by parsing interface, is there any objectives to create a contingent plan.

@dgerod
Copy link
Contributor Author

dgerod commented Aug 3, 2020

@bilal9876, sorry but I cannot get your point.

@bilal9876
Copy link

bilal9876 commented Aug 3, 2020

I mean that a contingency plan is a plan where the effect of any action may result in 'x' state (AND branch) or in 'y' state (OR branch). When the contingency plan is converted to temporal plan, (what is implemented in ROSPlan framework), the temporal plan considers only one branch (AND branch) as the effect of the action. So, after parsing the plan and then dispatching the plan, when action fail or the result of the action is not the same as the predicted one, the ROSPlan framework will replan again from the scratch and will not consider the another branch (OR branch) of the previously contingent plan.

is this true or not ?

@m312z
Copy link
Contributor

m312z commented Apr 1, 2021

Hello Bilal,

You are correct, the esterel/simple plan parsing assumes non-contingent plans. For parsing contingent plans you should use the PNP parser, which will generate a petr-net-plan for dispatch - you can find that here:
https://sites.google.com/site/rosplantopnp/home

About separating the planner interface and plan parsing nodes - this is how it should be. If they were one node, this couples the plan representation to the planner, which is not always desired. For example, I might want to generate plans with POPF and represent them as ESTEREL, PNP, or some other format. In addition, this allows to publish the plan as string to be shown to the user, or connected to some other component (not plan parsing and dispatch).

Michael

@m312z m312z closed this as completed Apr 1, 2021
@dgerod
Copy link
Contributor Author

dgerod commented Apr 3, 2021

@m312z , one question about this issue.

This issue is related to a missed functionality not with the question you are answering. I opened a PR to fixing it some time ago but it is still not merged, so is it correct to close this issue? Are you closing this one because this one already exists?

Thanks.

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

No branches or pull requests

3 participants