Unify tf frame parameters between transform and cloud nodes #344
Conversation
|
This is really useful inclusion. Can it be merged soon? |
|
Unfortunately Github's file size limit is limited to 10MB so I published a very short explanatory video at YouTube: https://youtu.be/Ji9btFjtnJM |
Thank you for sharing. Two questions:
|
You can see in the video, that as soon the field for
Currently our odom frame is published at 100Hz. Our velodyne sensor rotates with 10 rotations per second (600rpm). So there are 10 TF "updates" for a full rotation or a "update" every 36deg of rotation. This is sufficient for our use case as we are not driving that fast. It would be no problem to publish the odom frame at a higher rate for highly dynamic scenarios (especially with high turn rates of the ego vehicle). Currently the transformations are only done per packet (contains 384 points == 6 shots of the 64 diodes for our Velodyne 64E_S2). Therefore maximum meaningful rate to publish odom frame would be approx. 3470Hz for our 64E_S2 because it publishes ~347 packets per rotation and our sensor rotates at 10Hz. |
|
Thank you for the very informative response and thank you for your work. I hope it gets merged soon. |
|
We also think that this pull request is very useful as it reduces duplicate code and, more importantly, fixes some bugs with the frame_id names. |
|
@JWhitleyWork Do you have any plans to review this pull request? |
I apologize for the long delay. My job has been overwhelming lately. I will review it today. To what extent has this been tested and on which sensors? |
|
@JWhitleyWork I have tested this pull request on VLP-16. @AndreasR30 has tested this on Velodyne 64E_S2 as shown in their shared video above. |
|
We tested it on a HDL-64E_S2. We can also test it on HDL32E and VLP16 next week. But in general the changes are not sensor specific but are on a higher (point cloud) level so there should be no (newly indroduced) issues just for a specific sensor. |
|
To avoid cunfusion: @AndreasR30 works at our institute and will communicate just through this account from now on. |
|
@AndreasR30 Since |
There was a problem hiding this comment.
Thank you for doing the merge of the two classes. I intended to have this merge at some point as mentioned in here #214 (comment) more than a year ago when I developed the containers for the organized mode and removed pcl.
I would be careful about always having a TFListener, I intended the node also having the option to not transform points. Further, we should go for tf2 here and also use the fixed frame in the lookupTransform. Using tf2 there is no need to waitForTransform.
I also would prevent to check if computeTransformToFixed returns true all the time.
Additionally, we should watch out for the transform with respect to the time stamp related to each single point, and not only packets wise.
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
I agree.
I think it is necessary to check if the transform is available. Otherwise the program will not work properly.
I'm really not sure whether it is a good idea to call lookupTransform and to calculate a transform matrix millions of times per second. This will introduce a very, very large CPU overhead for minimal win. Furthermore one would have to publish the odometry frame (or something similar) millions of times per seconds, which introduces additional overhead. We think this error introduced by a per-packet-calculation is negligible to other errors. |
|
@UniBwTAS Please rebase on the current |
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
At this point there is no need any more for cloud node because transform node includes all features of cloud node.
UniBwTAS
force-pushed
the
unify_tf_frames
branch
from
7698c1d
to
254f46d
Compare
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
velodyne_pointcloud/include/velodyne_pointcloud/datacontainerbase.h
Outdated
Show resolved
Hide resolved
|
@spuetz and @JWhitleyWork is everything ok for you now or did I miss some remarks? I would be happy when you integrate it into the master branch. |
|
@UniBwTAS Thank you for working on this! Can you please create another PR to apply the same changes to the |
* Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
…ers#344) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de>
) * Unify tf frame parameters between transform and cloud nodes At this point there is no need any more for cloud node because transform node includes all features of cloud node. * Reformat code slightly to avoid roslint errors * Use boost bind instead of lambdas to add support for kinetic * Fix some search and replace errors in comments * Add fixed_frame and target_frame params back * Use more common ROS terminology instead of 'htm' * Undo renaming of publisher and subscriber * Just use tf listener when necessary * Migrate package to tf2 * Explicitly store sensor frame in a dedicated variable to make code easier to read * Reformat code slightly to avoid roslint errors * Avoid unnecessary transforms when sensor_frame == target_frame * Fix comments Co-authored-by: anre <andreas.reich@unibw.de> Co-authored-by: AndreasR30 <andreas-reich@live.de> Co-authored-by: anre <andreas.reich@unibw.de>
At this point there is no need any more for cloud node because transform node includes all features of cloud node.
Fixes #342