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

Fix: Enable automatic skill selection if grounding fails #72

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthias-mayr
Copy link
Member

Previously the implementation relied on the "label" property. However, this property is set to the concrete instance, even if the compound skill did not specify an instance.

To explain this a bit more:
I have a use-case in which I have a skill that runs in a loop for different work pieces. In one part of the procedure, a skill needs to be automatically chosen depending on preconditions on the material properties. So it is used without specifying an implementation:

self.skill("MyFlexibleSkill", ""),

In the first iteration, it instantiates the correct implementation, but at a later point we might encounter a part that needs the other implementation & the grounding fails here:
https://github.com/RVMI/skiros2/blob/04353faa3f405ab970eb49915ce114354f9f5f94/skiros2_skill/src/skiros2_skill/core/skill_utils.py#L265-L272

This is fine, but when entering tryOther, a check is performed whether the label is set to be a specific skill:

https://github.com/RVMI/skiros2/blob/04353faa3f405ab970eb49915ce114354f9f5f94/skiros2_skill/src/skiros2_skill/core/skill_utils.py#L235-L240

Unfortunately this label is set to the currently set instance:

https://github.com/RVMI/skiros2/blob/04353faa3f405ab970eb49915ce114354f9f5f94/skiros2_skill/src/skiros2_skill/core/skill.py#L424-L430

Besides the entertaining comment, the _label property of the skill is still the original empty string and is not affected by the instance that was set before
So this PR proposes to use this instead.

Previously the implementation relied on the "label" property.
However, this property is set to the concrete instance, even
if the compound skill did not specify an instance.
@frvd
Copy link
Contributor

frvd commented Jul 3, 2023

I would rather fix it to check cut the link to an instance (has_instance) as a first thing in the tryOther function

@matthias-mayr
Copy link
Member Author

That was also my first attempt, but it lead to some error. I can check what at it and look a bit more at it.

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

Successfully merging this pull request may close these issues.

2 participants