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

Removed sensor_msgs::msg::PointCloud #254

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 4, 2024

PointCloud is deprecated since Foxy for PointCloud2. I would say it's safe to remove this here.

This will require some changes in RViz2

Signed-off-by: Alejandro Hernández Cordero <[email protected]>
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

If there are know core tools such as rviz still using it it's too early to remove. In general this has been deprecated for close to a decade. There is a very large cost for potential users and almost no cost for us to keep the message definition available. It's just a directory "cleanup". We can keep it around until there's an additional cost to maintain it versus clearing it.

If we do want to clean it out, we should do an audit of known code bases and see how much it is used and do a communication push with potentially helper scripts to ease the transition before we remove it.

@clalancette
Copy link
Contributor

I have to agree with @tfoote on this one; just looking through the Rolling ecosystem, I see at least 12 downstream packages that are using sensor_msgs::msg::PointCloud. I'm not sure that it is worth the effort at this point to remove it and fix all of them.

@christianrauch
Copy link
Contributor

I don't think the costs come from maintaining the message definition per se, or from compiling and storing the message library.

I find the confusion of which message type to use to publish point cloud data, and thus the costs on the subscriber side to support both message types, much higher. Using the deprecated message type does not cause a deprecation warning or similar. Thus, there may be novel users who do not know that the message type is deprecated and use it to publish point cloud data. If the old and new message types continue to be used to publish point clouds, "good-behaving" consumers of point clouds also have to support both types with code for conversions etc.

I would probably start by adding deprecation warnings to all sensor_msgs/msg/PointCloud callbacks in the officially supported packages for one release. For the next release after, you could either remove all usages of the type in official packages without removing the message definition or remove the message definition completely.

Generally, having the ability to deprecate message definitions, such that they show a warning when they are de-/serialised, would be quite useful to maintain the lifecycle of a message definition.

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.

4 participants