strands-project / strands_perception_people

long-term detection, tracking and recognition of people
96 stars 70 forks source link

Adding the velocity of detect people to PeopleTracker message #169

Closed cdondrup closed 8 years ago

cdondrup commented 9 years ago

Similar to the people_msgs/Person that the tracker is already publishing, I added the velocity of each tracked human to the PeopleTracker message. In contrast to the Person message this is done via a geomotry_msgs/Vector3 instead of a geometry_msgs/Point because the Vector3 is easily transformable into other coordinate systems via tf whereas the Point obviously turns into rubbish when transforming.

Why do I do this? My current work needs the velocity of a human in addition to all the other values published in the PeopleTracker message and instead of subscribing to the PeopleTracker and People topic to get both, I find it more convenient to have all in one message. Consider it an oversight on my part that this has not been included from the start; mainly due to the MDL tracker not providing reliable velocities in the first place. With the bayes tracker on the other hand this is quite robust and reliable.

The velocities have been tested on the robot for the Person message but not for the new implementation of the PeopleTracker message yet. I mainly put this here for discussion and to see if people would have a problem with this because this would void all the recorded data as it would become incompatible with the new message type. There is however the possibility of converting rosbags: http://wiki.ros.org/rosbag/migration but I am not sure how this could be achieved for the mongodb. Maybe @hawesie has an idea or @Jailander can write a magic migrate.py for me?! ;)

If possible I'd like input from @lucasb-eyer @ferdianjovan @PDuckworth and who ever is using the tracker currently or has used it to record data in the past.

P.S.: The stuff for the emulator has been tested.

lucasb-eyer commented 9 years ago

You didn't change the MDL tracker, so that one still outputs the old format, right? So that makes them no more interchangeable. I'm wondering if anybody is/was still using the MDL one or we should completely drop it, since after this change, it's not useable as-is anymore, right?

As for old rosbag files, I didn't think of that, so thanks for the warning! Since I only ever recorded the low-level data, that's fine with me.

To recap, the change is OK with me, but I wonder if it still makes sense to keep MDL tracker in there at all after this change?

cdondrup commented 9 years ago

ATM the mdl tracker is just there because I sunk a lot of work into it and it can still be used as a vision only tracking approach. The messages they both publish are different any way and always have been so there shouldn't be a big change there.

Arguments for keeping the MDL:

Arguments against:

I could change the messages to be the same, but don't see much point to it if no one I know of is using it.

Long story short, the trackers have never been interchangeable and no one ever complained ;) This is far from ideal but imho currently a waste of effort and time to change it.

lucasb-eyer commented 9 years ago

The messages they both publish are different any way and always have been

Oh, then my point is moot anyways, so yeah, let's just keep it.

PS: I want to start using tracks sometime "soon", but we still have the "can't go from tracks to detections" issue I wonder if we'll ever manage to solve.

cdondrup commented 9 years ago

This: https://github.com/LCAS/bayestracking/pull/13 needs some more love I think but we're getting there at least for NN data association. Maybe @pet1330 knows more on the status of that PR.

ferdianjovan commented 9 years ago

I think the changes is good, and will not void anything because what we have been recording, AFAIK, is only human trajectories.

PDuckworth commented 9 years ago

I echo Ferdi's message. Thanks for asking though.

cdondrup commented 9 years ago

Great. Thanks guys! I'll test it asap on the robot and merge if it works. I'll also provide a script to migrate rosbags and try to fix the database entries; So it might take a few days ;)

marc-hanheide commented 9 years ago

"few days" means "after ICRA deadline", I suppose?

cdondrup commented 9 years ago

Isn't that what "few days" always means?

cdondrup commented 9 years ago

Tested on Linda and works fine. But I still have to write the migration scripts. Once that is done, I'll merge this.

pet1330 commented 9 years ago

Sorry! Completely missed this. Yes, they're done for the moment, it allows you to pass a message though, and if you use NN_LABEL or NNJPDA_LABEL for your algorithm then it will also Bayesian track them. If however, you don't want the labels to be considered, but you want them to be passed through the tracker to be used on the other side then use NN or NNJPDA algorithm. If no label is passed in, then each track is assigned an empty string.

cdondrup commented 9 years ago

This PR should now be mergable.

To fix old bag files after the merge, just run rosbag check BAGFILE which should tell you thet there is a rule to migrate it and then run rosbag fix INBAG OUTBAG to transform the old bag into a new one containing the correct message type. For database entries, run rosrun bayes_people_tracker_logging migrate.py which alters the entries in place so please copy the people_perception collection in the message store before running this to make sure no data is lost in case of failure. All this is mentioned in the READMEs of bayes_people_tracker and bayes_poeple_tracker_logging.

The new velocities filed will contain empty geometry_msgs/Vector3s but the number of empty vectors corresponds to the number of uuids so that your loop does not break. I tested all the velocity message related things in simulation and on Linda, but if anyone else wants to give it a go, please do.

marc-hanheide commented 9 years ago

:+1: