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

Refactor mdr_find_people #152

Open
minhnh opened this issue May 22, 2019 · 8 comments
Open

Refactor mdr_find_people #152

minhnh opened this issue May 22, 2019 · 8 comments
Assignees
Labels
Feature: Perception Type: Refactoring Changes in the way the code works internally.

Comments

@minhnh
Copy link
Member

minhnh commented May 22, 2019

As discussed in #145, we need further discussion in how to handle mdr_find_people.

Yeah but mdr_detect_person can be extended to have 3D processing, instead of creating a new package. OpenPose and other methods can also be added later there.

mdr_find_people, I think, will potentially use mdr_detect_person for detection but most likely also involve navigation and moving the head, which may need to be made into a more complex skill. It may also be redundant with find_object skill, depending on how we implement that.

With that in mind, I think it's a good idea to merge the detection and 3D calculation into mdr_find_people. There's a portion which handles inserting the person's info into the knowledge base, however, that may remain here in mdr_find_people.

Originally posted by @minhnh in https://github.com/_render_node/MDIzOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTc0NjAzMzE5OnYy/pull_request_review_threads/more_comments

@argenos argenos changed the title refactor mdr_find_people Refactor mdr_find_people May 23, 2019
@argenos argenos changed the title Refactor mdr_find_people Refactor mdr_find_people May 23, 2019
@minhnh minhnh mentioned this issue Jun 4, 2019
@argenos
Copy link
Contributor

argenos commented Jun 4, 2019

From #168: The function to find out whether people are inside the arena should be refactored to get this from the topological map. This part can probably be assigned to one of the new members.

Sent with GitHawk

@minhnh
Copy link
Member Author

minhnh commented Jun 4, 2019

After a quick discussion with @henrikschnor today, I think we can consolidate the different person recognition functionalities into a people recognition action server.

  • would combine mdr_detect_person, mdr_recognize_emotion_action, mdr_gender_recognition
    under mdr_planning/mdr_actions/mdr_perception_actions/ into one action
  • detection from RGB images:
  • extract 3D info of people: this may require the action server to use point clouds, and extract RGB info from these point clouds for detection and recognition tasks. Alternatively, message_filters can also be used for synchronizing topics (Python API example exists now woohoo!).
  • ensemble of recognition models: age, emotion, gender can all be done on the detected RGB face/body images
  • direct subscription to the image and point cloud topics to avoid sending additional data on the network
  • proposed YAML configuration for initializing the action server
# assuming all the recognition models are Keras
recognition_models:
  gender: /path/to/gender/model.h5
  emotion: /path/to/emotion/model.h5
  ...
identity_model: /path/to/identity/model.h5
age_model: /path/to/age/model.h5   # doesn't exist yet
detection_models:
  full_body_detection:
    module: mas_image_detection.ssd_tensorflow
    class: SSDTfModelsImageDetector
    config_package: mas_image_detection
    kwargs_file: configs/ssd_tensorflow_coco_kwargs.yml
    class_file: configs/tf_models_coco_classes.yml
    person_class: 1
  key_point_detection:
    # to be implemented still
    model_path: /path/to/some/model
  • proposed action specs
---
mas_perception_msgs/PeopleScene people_scene
---
  • proposed mas_perception_msgs/PeopleScene.msg (new)
sensor_msgs/Image scene_image  # store the full image of the scene
mas_perception_msgs/Person[] people
sensor_msgs/RegionOfInterest[] face_rois
sensor_msgs/RegionOfInterest[] body_rois
  • proposed mas_perception_msgs/Person.msg
string name
float32 age  # does not exist yet
sensor_msgs/Image face_image
sensor_msgs/Image body_image
sensor_msgs/PointCloud2 body_points
# a field for key points, possibly a float32[], but also does not exist yet
string[] attribute_names  # gender, emotion,...
string[] attributes  # e.g. male, angry,...
float32[] attribute_confidences  # e.g. 0.71, 0.80,...

@argenos @alex-mitrevski @PatrickNa feel free to share thoughts on the topic

@alex-mitrevski
Copy link
Member

alex-mitrevski commented Jun 4, 2019

At a quick glance, this seems like a solid improvement, particularly because the gender and emotion recognition actions were not really actions in the first place.

I'd thus say go for it.

P.S. You might want to think about actual person recognition as well, namely matching the detected people with known names. I see the name field is already there in the proposed Person message, but then a recognition model will also be needed.

@minhnh
Copy link
Member Author

minhnh commented Jun 4, 2019

Good point on the name model. Edited! @robertocaiwu has also expressed interest in working on this issue.

@henrikschnor
Copy link
Contributor

A complete rewrite using things from mdr_find_people and the other packages is probably reasonable. However I won't be available until the end of next week, so @robertocaiwu feel free to start already and let me know it goes.

@argenos
Copy link
Contributor

argenos commented Jun 30, 2019

This sounds good! Can we find a way to split this issue so both @robertocaiwu and @henrikschnor can work in parallel?

@argenos
Copy link
Contributor

argenos commented Jul 1, 2019

After going through the rules again, there are three functionalities related to this: 1) the perception related part of detecting the person, 2) perception related part of recognition of different stuff and 3) the speech part of providing the description.
I'd say that during the implementation we should be a bit careful of making this modular, in some cases we may not want to do all the above actions

@minhnh with this being mostly refactoring, can you take the lead? I think we need an initial version of a branch with the right structure, maybe then we can split the rest of the task between @robertocaiwu and @henrikschnor (depending on what comes out of the scenario tests)

@minhnh
Copy link
Member Author

minhnh commented Jul 1, 2019

sure I'll work on this later this week

@minhnh minhnh added the Type: Refactoring Changes in the way the code works internally. label Jul 1, 2019
minhnh added a commit to minhnh/mas_perception_msgs that referenced this issue Jul 5, 2019
* remove name & id fields in Face.msg in favor of identity field in Person.msg
* remove safe_pose field in Person.msg, so safety logic will be isolated from data model
* add comments about legacy field in Person.msg, but not removing for compatibility
* remove unused message FaceList.msg
* is motivated by discussion in b-it-bots/mas_domestic_robotics#152
alex-mitrevski pushed a commit to b-it-bots/mas_perception_msgs that referenced this issue Jul 19, 2019
* remove name & id fields in Face.msg in favor of identity field in Person.msg
* remove safe_pose field in Person.msg, so safety logic will be isolated from data model
* add comments about legacy field in Person.msg, but not removing for compatibility
* remove unused message FaceList.msg
* is motivated by discussion in b-it-bots/mas_domestic_robotics#152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Perception Type: Refactoring Changes in the way the code works internally.
Projects
None yet
Development

No branches or pull requests

5 participants