Add: set transformation for people tracker #606
Conversation
|
Hi Moritz, that is a huge contribution, thanks! Unfortunately, I am not really in touch with this part of PCL, so won't comment on how useful/sensible your modifications are. Hopefully we'll get feedback from others, in particular @mmunaro as the author of the original contribution. In the meanwhile we could take care of some minor stuff related to PCL style conventions and other coding issues. I will go through the commits and make a few in-line comment that you may consider. |
|
Thanks Moritz for your contribution! |
| * Software License Agreement (BSD License) | ||
| * | ||
| * Point Cloud Library (PCL) - www.pointclouds.org | ||
| * Copyright (c) 2013-, Open Perception, Inc. | ||
| * Copyright (C) 2014, BMW Car IT GmbH |
There was a problem hiding this comment.
Since your changes to the files are minor, can you please remove this extra Copyright line? If you want, you can add your name to \author Doxygen tag.
|
I tested the new contributions and a couple of parameters must be changed in order to have detection performances similar to the previous versions of the code (see inline comments). |
|
@taketwo I have a quick question with respect to the space before parentheses: should I really add those spaces, even though this results in a non-consistent style of the respective source files (note that in the current code, there are no spaces before parentheses when a function is called)? Actually, initially, I had added the spaces, but then I saw it would clash with the style of the rest of the people tracker source and decided to omit them... |
|
Actually I don't have a strong opinion about this. @jspricke could you please comment? |
|
In the original version of the code, the space before parentheses was not there for forgetfullness. |
For the person classifier a flag has already been used, but for the intrinsics matrix and the ground coefficients, data structure specifics are used in order to determine whether or not the variable has been set. Since not all data structures have such a specific characteristic, (e.g. in one of the next patches we introduce a transformation matrix for which it can't be easily checked whether it has been set or not), and for consistency, it is better to use flags throughout the whole code.
The update of min_points_ and max_points_ happens every time the compute method is called. If voxel_size_ is different from the standard value of 0.06, for each call to the compute method, min_points_ and max_points_ are getting smaller (for a voxel size > 0.06). After a couple of iterations, they are equal to zero. It is better to set min_points_ and max_points_ outside of the compute method, since there is no need to recalculate them every time. In addition, min_points_ and max_points_ depends on internal knowledge of the people tracker (the voxel_size_ and the fact, that there is only one point per voxel after downsampling). For a user of the people tracker, the only thing that should matter is the height and the width of a person cluster. The cluster limits can be deduced from this information.
In the next patch, the filtering method will be extended. In the future, it would be desirable to have all computing steps in the compute method in separate methods, but for the moment we limit ourselves to the filtering.
In order to be able to cut off outliers, the z-dimensions of the field of view can be set from outside the people tracker. If there is only a single outlier, it might not be able to filter the point cloud due to a lack of memory.
The choice of the coordinate frame matters for the people tracker (e.g. for the height calculations), and it is important to allow for different choices of coordinate systems. Doing the transformation inside the people tracker leads to a better performance than doing it outside (since it can be applied to the downsampled point cloud), and also simplifies the use of the people tracker.
For outdoor environments (curbs, stones, uneven sidewalks, ...), it is necessary to have a higher tolerance. The people tracker still seems to work fine with this higher value in indoor environments.
|
Thanks for all comments! I just pushed a new revision:
Looking forward to further comments! |
|
LGTM, if Matteo doesn't have any further comments then I can merge. |
|
Sorry for the late reply. Regarding the extra spaces, I think it depends if we the application as part of the lib or as actually a separate project (which just happen to live inside the same repo). For the lib, the style guide is clear and we should fix style errors if we touch the line in question anyhow (or have a clean up commit if there would be really to many, maybe). For the application it would be nice to adopt the same style guide to make it easier to read coming from PCL, but that's not so important as for the lib, where you are required to read into the source to use it. |
|
It seems that the test run took too long and therefore one of the builds on Travis did not run through, anybody has an idea how to fix this? I did not change anything significant in the tests... |
|
Travis has a 50 minute limit on a task run. And it often takes more than 50 minutes for gcc to compile the library, leave alone running unit tests. So this is okay since the task with clang compiler succeeded. |
|
Ok for me, thanks Moritz! |
|
@moritzblume Would be nice to finally merge this. There are just a few lines with missing spaces, could you please fix it? |
|
I am a bit confused now. I thought we would keep the current style of the people tracker (no spaces before parentheses when calling a function), and then later make a space correction patch. Can you clarify this again? |
|
I was referring to the comment of @jspricke:
However my comment was a bit misleading, sorry. Actually in the majority of cases there is no space and there are just a few lines with spaces. Which might make sense to remove for consistency. On the other hand the spaces are anyways against the style rules, so this is probably not too important. @jspricke should we merge? |
Add: set transformation for people tracker
I suppose this means where the camera frame is not the map frame. How do you set the ground plane coefficients? I tried similar approach as in this patch but it got segfaults. Currently I'm transforming the returned cluster centers from camera frame to map frame which should be even simpler and use less computation than transforming the downsampled no ground cloud. |
This patch series contributes to the PCL people tracker: