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

Remove Pose2D deprecation warning and retain it #179

Open
SteveMacenski opened this issue Mar 15, 2022 · 4 comments
Open

Remove Pose2D deprecation warning and retain it #179

SteveMacenski opened this issue Mar 15, 2022 · 4 comments
Labels
more-information-needed Further information is required

Comments

@SteveMacenski
Copy link
Contributor

Hi,

I know this discussion has come up before, but it has once more come to my attention that Pose2D is an important message type which is used widely and has value in readability and conciseness for the mobile robot use-case. While I do not generally support using it internally within Nav2 because I'd like to be as agnostic as possible, there are many situations where it is used for specialized purposes and has significant value in existence.

More than representing SE2 poses in the navigation context, it is also useful for image processing like in vision_msgs where having 3 axis rotations are not meaningful for image plane analysis of results.

Finally, while not a strong argument on its own, it seems odd to depreciate the message in ROS 1 given it won't be removed from any existing OS, it was still included in Noetic, and there will not be another one. The commit message for this deprecation warning says that it is for forward porting it, but its forward porting something that will never actually be removed in the initial location.

@ijnek
Copy link
Contributor

ijnek commented Mar 16, 2022

I agree with this point. When working with images, having to deal with quaternions makes no sense while dealing with simple theta angles is far easier.

Quoting Pose2D.msg,

If we have parallel copies of 2D datatypes every UI and other pipeline will end up needing to have dual interfaces to plot everything

If they are information in different dimensions, wouldn't it make sense to have parallel copies if needed?
Does this plotting refer to rqt_plot? I don't see who it affects rqt_plot if it is, as rqt_plot shouldn't need to know the msg type beforehand.
What are some other cases that support this statement?


EDIT

I think the suggestion in ros-perception/vision_msgs#65 is reasonable.

@tfoote
Copy link
Contributor

tfoote commented Mar 16, 2022

The use in vision_msgs is a perfect example of a complete misuse of this message. I've ticked that in ros-perception/vision_msgs#65 and it looks like the update to ros-perception/vision_msgs#64 resolved it appropriately.

For background, this deprecation was one of the outcomes of the design review in preparation for the Foxy release: #86 The intent of deprecating this message in Foxy and Noetic was to let people know that this message is deprecated in all forks and that they should stop using it. (And they wouldn't be on noetic 5 years later and have no viable migration target once it's removed in a future version of ROS 2. As we're well past the minimum duration of deprecation and this has been raised, I would actually propose that this is a good reminder that we should clear out this long deprecated message for the next release instead of adding it back.

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented Mar 16, 2022

Discussions on this topic have been divisive at best with some real push back from the mobile robotics side of the house beyond just me when its come up on Discourse/ROS Answers and similar. I think depreciating this message adds hardship without offering a great deal of benefit. This is a useful debug message type for mobile robot users.

It’s certainly not the case that usages are unfixable if it were removed, but I’d argue that it has practical utility. I see no reason to remove something of usefulness that has long standing in geometry_msgs.

I’ve agreed with your suggestion with respect to vision_msgs, but that’s a cherry picked example that isn’t illustrative of the general use of the type to minimally and human-readably represent an SE2 pose, which is a pervasively used euclidean space for localization, mapping, planning, 2D kinematic simulation, and visualization.

I’m not going to die on this hill, but I would suggest reconsidering this point or bringing it up explicitly in a new discussion in discourse to allow folks to express their thoughts.

@tfoote
Copy link
Contributor

tfoote commented Mar 22, 2022

There are cases where presenting an SE2 pose for user interactions is valuable. This deprecation and removal of the message is not about preventing that from being done. What the focus of this review and the standardization on using the full 3D message by default as the interface is to avoid fracturing the community. If we were to have two first class messages for Pose a 2D and a 3D representation, every piece of software that wanted to interact with poses would need to be able to subscribe to either or both to be compatible in a generic pipeline. And then you'll get into issues where you have to parameterize each element in the pipeline to accept 2D or 3D messages. And then select your preferred output format, so that you don't get double publishing. This now leads to requiring 4x the number of tests for an interface. As well as significantly more code duplication just doing the logic of switching input types and output types as well as adding projection logic at every stage that might be required to downconvert the data. And depending on your application you may want to downcovert into a specific plane or coordinate frame which adds another parameterization requirement for any/all the elements in the pipeline.

On the other side you can incur the relatively small marginal overhead of sending a few extra bytes per pose to represent the full 3D pose and use the full 3D representations and readily available and highly mature 3D linear math libraries to do the full computation for each element in a SIMD optimized manner. There are no conditional logic for which representation is being sent from either side. There's no requirement to project it down onto a plane in mid computation.

And as I said this does not require or prevent any UI/dialog or any other user interaction from presenting to the User a SE2 representation with any units you want such as degrees. The messages should be optimized to be able to be debuggable effectively but we should not pick our transport primatives specifically to be human readable instead of being efficient and standard. If we wanted to make things more "human readable" we wouldn't be using radians, nor would we be using seconds since the epoch. These can be easily presented to the human in alternative forms in the same way as the Pose can be rendered into SE2 and upconverted by the UI elements. Or even have configurations for what format the user wants to interact with.

In any of the cases you listed such as localization, mapping, planning, or visualization interfaces, they are already typically projecting onto a 2D plane for interactions. Even for obstacles that have potentially height such as octrees, they get projected down onto the plane. The pose messages can do the same. And if you're taking inputs from that world. It's trivial for you to pad out the representation with zeros for the other dimensions. However, even in the "2D" worlds sometimes heights are actually very important. For example if you want to navigate through a door, the height of the handle is an important 3D pose. You could set that as a goal and flatten it into the plane. But then if you wanted to compute the appropriate standoff distance for the robot using IK solutions you would not be able to do so based on the 2D pose because the arm planning would no longer have access to the height or potentially full orientation. Whereas if the pipelines elements can apply a proper semantic interpretation of the data, either preserving unused dimensions or specifically projecting into the coordinate frame of relevance to them, then the extra information can be retained and leveraged by downstream consumers.

This was brought up for the full community to review on ROS Discourse 2 years ago and I don't believe that anything has changed. This has been a long standing issue that was raised many years before but we didn't take action to clean it up. And this generally follows our policy of using a slightly more generic format unless there's a strong reason to use the more specific format. Such as we use float64 values not float32 values in most places by default because the marginal cost is relatively low and avoids the issue of having fractured ecosystems. But in places where there's very high datavolume we make exceptions such as PointClouds use 32bit representations and images support dense encoding formats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

4 participants