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

Efficiency improvements and customizable occupancy 2D map projection. #9

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

carlosmccosta
Copy link

  • Efficiency improvements:
    |- avoid transforming clouds already in map frame (added additional
    parameter to specify sensor frame_id for calculating sensor pose)
    |- create occupancy cloud directly as ros message instead of pcl cloud
    (avoids conversion)
  • Projection of nav_msgs::OccupancyGrid with customizable height and
    initialization as free or unknown.
  • Increased tf listener buffer to 30 sec.

+ Efficiency improvements:
| avoid transforming clouds already in map frame (added additional
parameter to specify sensor frame_id for calculating sensor pose)
| create occupancy cloud directly as ros message instead of pcl cloud
(avoids conversion)

+ Projection of nav_msgs::OccupancyGrid with customizable height and
initialization as free or unknown.

+ Increased tf listener buffer to 30 sec.
@ahornung
Copy link
Member

Thanks for your contribution! Is this meant for hydro or indigo? Best send the pull request against the right ...-devel branch then in the future (master tracks the latest stable releases).

More comments in the code.

size_t numberReservedPoints = m_octree->size();
float* pointcloudDataPosition = NULL;
if (publishPointCloud) {
cloud->header.seq = m_numberCloudsPublished++;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line (and the variable m_numberCloudsPublished) is not needed, the publisher should automatically increase .seq in the header.

of sensor frame id.

- Line 310 of OctomapServer.cpp was doing an unnecessary internal copy
(pc_nonground = pc), that can be avoided by using shared pointers.
- Improved assembly of pointcloud from octree by using vector reserve
instead of resize.
- Improved parametrization of sensor frame id. This way the input cloud
can be sent in any frame id (map, sensor, other), and the parameter
sensor_frame_id can be used to specify which is the sensor frame to
compute the origin for the raytracing algorithms (used to integrate the
pointcloud into the octree).
@carlosmccosta
Copy link
Author

Latest commit addressed the suggestions made during the pull request discussion.
Thanks for the fast feedback 👍
Was tested in ROS Hydro in Ubuntu 12.04 LTS.

P.S.
The reason for avoiding conversion between pcl::PointCloudpcl::PointXYZ and ROS sensor_msgs::PointCloud2, is because the PCL uses SSE aligned structures for speeding up computations.
http://pointclouds.org/documentation/tutorials/adding_custom_ptype.php
But since there will be no use of the pointcloud data (it's just a container in this case), using directly sensor_msgs::PointCloud2 can reduce memory consumption by as much as 25% (pcl::PointXYZ uses a float[4] to store the XYZ instead of a float[3]).

@jvgomez
Copy link
Contributor

jvgomez commented Jun 19, 2015

Can I ask why this PR hasn't been merged?

@ahornung
Copy link
Member

To be true, I haven't had a chance to test it thoroughly since the changes are rather fundamental with potential to change / break existing behavior.

There are also multiple features & improvements mixed in one PR, which makes it hard to track the individual influence (as opposed to quickly merging a more atomic PR).

@jvgomez
Copy link
Contributor

jvgomez commented Jun 22, 2015

I see and I completely understand :) I was just curious, as efficiency improvements are always wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants