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

Containers and PCL point type for backwards compatibility #347

Open
wants to merge 8 commits into
base: melodic-devel
Choose a base branch
from

Conversation

spuetz
Copy link
Contributor

@spuetz spuetz commented Jun 8, 2020

This is a possible solution for #340 for melodic.

  • It restores the pcl dependency
  • The missing point_type.h is added again.
  • Corresponding containers without a time field are added.
  • The assignment of the time value has been fixed for the organized cloud container, see organized_cloudXYZIRT.cc.

Things to do before merging / releasing

@JWhitleyWork
Copy link
Contributor

Item 3 in your list is handled by #344. Since this functionality is necessary to get a new release, I'll merge this first and have the author of #344 rebase on your changes.

@JWhitleyWork JWhitleyWork changed the base branch from master to melodic-devel June 8, 2020 19:26
@spuetz
Copy link
Contributor Author

spuetz commented Jun 9, 2020

We should have this also in the master for noetic with the option of selecting the right container.
But, of course without the point_type.h and the pcl dependencies.
Further, we could handle the containers as plugins, using the pluginlib. Then people could use the driver and modify their container, meaning modifying what is in the cloud and how, e.g. how it is transformed, etc.
Additionally, for noetic I would remove the config flag for organize_cloud since this is covered by using one of the organized containers.

container_ptr_ = getContainer(default_container_.second);
}
ROS_WARN_STREAM("Using default container \"" << container_name << "\"!");
ROS_WARN_STREAM("Please use the \"container\" param with one of the following container names: " << container_list.str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please either do one of the following:

  1. Add the container parameter to all launch files and set it to the un-organized default of PointCloudXYZIR; or
  2. Set this to be an INFO statement instead of a warning.

Having the new default value cause a warning seems problematic and could unnecessarily alarm users. I would prefer the first option since it should also be accompanied by some documentation with the possible values in the launch file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also prefer the first option! Will do that.

}
EIGEN_ALIGN16;

}; // namespace velodyne_pointcloud
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also define the PointXYZIRT in terms of a PCL structure so people can use it in the same way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes for melodic that makes sense. For noetic, I'll move this to the velodyne_pcl package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

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

This item still needs to be done.

@JWhitleyWork
Copy link
Contributor

@spuetz Can you please address my concerns? After that, I'll see if I can find a package to test with.

@JWhitleyWork
Copy link
Contributor

@spuetz ping for changes.

@JWhitleyWork
Copy link
Contributor

@spuetz Any update here?

@spuetz
Copy link
Contributor Author

spuetz commented Sep 3, 2020

Sorry for the late reply. I'll have time in two weeks to finish this.

@JWhitleyWork
Copy link
Contributor

Thanks, @spuetz!

@JWhitleyWork
Copy link
Contributor

@spuetz ping

@JWhitleyWork
Copy link
Contributor

@spuetz Still two items remaining when you can make time.

@JWhitleyWork
Copy link
Contributor

@spuetz Can we get this wrapped up?

@hriihimaki
Copy link

Any update on this? @spuetz @JWhitleyWork

@JWhitleyWork
Copy link
Contributor

Rebased on current melodic-devel. Waiting for @spuetz to complete as I just don't have the background or time.

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