Behavior Tree #95
Behavior Tree #95
Conversation
micolspitale93
requested review from
chrismbirmingham and
RMichaelSwan
and removed request for
chrismbirmingham
|
Great work so far! Everything you have done so far will be useful for future work. We appreciate the work that you have done getting a working example for integrating HARMONI with py_trees. However, for where the project is right now we have some additional requirements and recommendations for how we would like to see the integration happen:
In the future we will be looking at the possibility of a standalone python based HARMONI, but for now we want the py_trees to utilize the communication system we have already set up. The current revision of work should be saved as a separate branch so that we can use it later. |
There was a problem hiding this comment.
I see a lot of improvements have been made. I still need to test on my PC and review some of the newer non-leaf classes, but overall looks pretty good. One comment on commit history: use descriptive commits and/or squash commits together! I am not too picky on git history, but when I see multiple commits with the exact same comment, it does make it notably harder to understand what actually occurred.
| @@ -255,6 +255,7 @@ def _get_viseme_data(self, data): | |||
| else: | |||
| behavior_data = data | |||
| viseme_set = [] | |||
| #FIXME decommenta le righe | |||
There was a problem hiding this comment.
| #FIXME decommenta le righe |
| @@ -57,7 +57,7 @@ def do(self, data): | |||
| if type(data) == str: | |||
| if ".wav" in data: | |||
| data = self.file_path_to_audio_data(data) | |||
| duration = data["duration"] | |||
| duration = data["duration"]-0.5 | |||
There was a problem hiding this comment.
Leave out magic numbers if not obvious. One option: make a constant, e.g. AUDIO_DELAY or something (the reason for or meaning of "0.5" should be clear).
| #FIXME | ||
| #vorrei scrivere | ||
| #self.result_msg = tts_response["audio_data"] | ||
| #ma scrivo |
There was a problem hiding this comment.
Is this still needed? Also, English please :) this is not the only instance.
| INTERACTION = "Background_noInterazione" | ||
| VISUAL = "Background_visivo" | ||
| CARTA = "Carta" | ||
| RACCOLTA = "ConfirmRaccolta" |
There was a problem hiding this comment.
Forgive my lack of Italian, but could we get translations in comments at least? Ideally everything would be in English, but I can understand not reworking lower importance things. Much appreciated whatever you decide.
| @@ -86,7 +86,9 @@ def _preempt_callback(self): | |||
| """Used to signal a cancel/pause to the currently running service so | |||
| that a new goal can be received. | |||
| """ | |||
| self.service_manager.pause() | |||
| self.service_manager.stop() | |||
There was a problem hiding this comment.
I believe this was working with pause, so not clear on why the change was made here. Stop might mean a service will require a more in-depth startup procedure to begin again
| @@ -0,0 +1,175 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Repeated code is a warning sign/design smell. All the leaves in this pull request tend to have a similar structure which means a small architecture change (e.g. something with HarmoniServiceSever) would require identical modifications to all leaves. To reduce total amount of code, I would strongly recommend creating an abstract base class for all leaves.
Rostests already implements for the following pytree leafs:
rostest harmoni_face face_pytree.testrostest harmoni_bot lex_pytree.testrostest harmoni_tts polly_pytree.testrostest harmoni_speaker speaker_pytree.testrostest harmoni_web web_pytree.testThe remaining leaf's tests (harmoni_microphone, harmoni_camera, and harmoni_stt) are still work in progress.
If you want to test a already created Tree, you can run the following:
rostest harmoni_pattern pytree.testIt will execute a try composed by a sequence of:
Still not handling the different HARMONI API in the testing (WORK IN PROGRESS)