b-it-bots / mas_domestic_robotics

Robot-independent ROS packages for domestic applications
GNU General Public License v3.0
28 stars 45 forks source link

Refactor mdr_find_people #152

Open minhnh opened 5 years ago

minhnh commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

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

alex-mitrevski commented 5 years ago

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 commented 5 years ago

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

henrikschnor commented 5 years ago

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 commented 5 years ago

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

argenos commented 5 years ago

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 commented 5 years ago

sure I'll work on this later this week