Skip to content

Add nav_msgs/PoseParticle and nav_msgs/PoseParticleCloud messages #118

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: rolling
Choose a base branch
from
Open

Add nav_msgs/PoseParticle and nav_msgs/PoseParticleCloud messages #118

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03d3588
Add nav_msgs/Particle and nav_msgs/ParticleCloud messages
naiveHobo May 19, 2020
a8588d9
Rename to PoseParticle
naiveHobo May 19, 2020
f11e15c
Fix Particle msg reference in PoseParticleCloud
naiveHobo May 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions nav_msgs/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ set(msg_files
"msg/MapMetaData.msg"
"msg/OccupancyGrid.msg"
"msg/Odometry.msg"
"msg/Particle.msg"
"msg/ParticleCloud.msg"
"msg/Path.msg"
)
set(srv_files
Expand Down
2 changes: 2 additions & 0 deletions nav_msgs/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ For more information about ROS 2 interfaces, see [index.ros2.org](https://index.
* [MapMetaData](msg/MapMetaData.msg): Basic information about the characteristics of the OccupancyGrid.
* [OccupancyGrid](msg/OccupancyGrid.msg): Represents a 2-D grid map, in which each cell represents the probability of occupancy.
* [Odometry](msg/Odometry.msg): This represents an estimate of a position and velocity in free space.
* [Particle](msg/Particle.msg): This represents an individual particle with weight produced by a particle filter.
* [ParticleCloud](msg/ParticleCloud.msg): This represents a particle cloud containing particle poses and weights.
* [Path](msg/Path.msg): An array of poses that represents a Path for a robot to follow.

## Services (.srv)
Expand Down
5 changes: 5 additions & 0 deletions nav_msgs/msg/Particle.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# This represents an individual particle with weight produced by a particle filter
Copy link
Member

Choose a reason for hiding this comment

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

My main concern here is that the name does not match the contents. In my experience, which is admittedly limited here, there's nothing inherent about a particle in a particle filter that indicates pose with a single weight, even in robotics, and even with navigation (though with robot navigation I assumes common place). If you want this to be specifically for particle filters that use pose and a weight, then a different name is in order, imo. If you want it to be a general purpose particle that can be used with any particle filter then the name is fine, but the contents should be more flexible. I think the way Pointcloud2 is structured (with channels describing binary data) is an appropriate example. Same if you want it to be a particle that can be used with any robot navigation particle filter, except maybe you don't need configurable channels.

For a different name, I'm not 100% sure what it should be.

If the idea is that most all particle filters that are for robot localization would use this message structure (pose + 1 weight), then something like nav_msgs/WeightedPoseParticle and nav_msgs/WeightedPoseParticleCloud could be good, even if verbose. It would be nice to save the names Particle and ParticleCloud for a more generic particle message if we ever need that.

However, this leads me to think that it isn't that generic. I'd be happy to be shown wrong, but to do that I'd like to hear from others in the community that implement nav particle filters on whether or not they would converge on this type. Or maybe if there are other messages out there on the internet and we could collect a few and show how similar they are (and therefore worthwhile to try and standardize them).

If the idea is that this message is just what works for one or more filters in the nav2 project and we want it here to be convenient or to just see if other people use it, then I don't think it belongs here yet and also that the rviz plugin mentioned in the original description also does not belong in the default plugins. I'm not trying to be harsh here, but what I want to do is encourage convergence on common types, but for that to happen the types need to be reusable and need to apply to a decent set of existing or planned works. This is hard work and if no one is interested in doing that then I think it's better to keep the message and plugins in their own repositories until they can be shown to be general. Others may disagree, so please chime in. @ros2/team

Copy link
Contributor

@SteveMacenski SteveMacenski May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Same if you want it to be a particle that can be used with any robot navigation particle filter

I'm not bringing this up to try to argue your point, see below I think we have some philosophical overlap on naming, but I'd like to temper expectations on what this package is. nav_msgs, to the best of my understanding, has never been a collection of messages suitable for every form of robot navigation. If that were the case, about 2/3 of these messages shouldn't be here. These messages are here to support the work happening in the navigation stack, which often can lend themselves useful to other navigation projects. Effort should be placed in making sure they're reusable when reasonable, but at the end of the day, this is the navigation project's messages and I think its a little different from the rest of the packages in this repo that it acts to serve a specific ecosystem of users rather than generally used messages. I would argue that this package shouldn't be in this repo at all and belongs in ros-perception like moveit_msgs, which is a direct analog. I don't really care where it lives, and I think this is a really great discussion we're having, but I think we're coming at this from different mindsets that we should explicitly acknowledge.

Anyhow.

I think your name suggestions are really good - that brings up another side of your argument that I can agree with. This should be more specific as to the type of particle it is. Personally I think the weighted bit is redundant. In the general formulation of a particle filter, each particle has a score so I think that's totally implied. PoseParticle and PoseParticleCloud I could totally support.

Other examples in the community I found (still MCLs admittedly) which could readily use this definition of the messages proposed, under whatever name we decide on:

  • NDT-MCL uses a PoseParticle
  • Quick-MCL uses just a visualization_msgs/MarkerArray - no weights
  • mcl pi gazebo uses the PoseArray that we're trying to move from to show particle weights
  • MRPT localization uses the PoseArray that we're trying to move from to show particle weights
  • AMCL3D fails to publish anything at all

You're not being harsh, we're having a conversation - a good conversation :-)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, with the presumption that Particle means "state plus a single weight", then PoseParticle* sounds good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@naiveHobo are you OK with that?

Copy link
Member

Choose a reason for hiding this comment

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

nav_msgs, to the best of my understanding, has never been a collection of messages suitable for every form of robot navigation. If that were the case, about 2/3 of these messages shouldn't be here.

I agree with you, but moving forward, if I'm pressed to say "what should be in common interfaces or not" and "what should be in rviz default plugins or not", then I think it should either be "the message is generally useful for running a ROS system" or "the message is general enough to be used by multiple parties for the same purpose". Many of the messages here are here due to historical reasons.

We probably need a "generic nav messages package" and a "ROS nav(2) project specific messages package" to separate the use cases. Like many things in ROS the nav stack's messages were intended to be the standard, but it fell short.

Copy link
Author

Choose a reason for hiding this comment

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

PoseParticle works. I'll go ahead with the changes then?

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need a "generic nav messages package" and a "ROS nav(2) project specific messages package" to separate the use cases. Like many things in ROS the nav stack's messages were intended to be the standard, but it fell short.

We sort of have that, we have nav2_msgs that are messages in a staging ground before eventually being homologated with nav_msgs. Whether that's nav2_msgs go into nav_msgs or nav_msgs gets relocated to ros-perception. The other things in nav2_msgs are still changing and don't belong here (yet). I suspect the actions will never belong here unless we move to ros-perception out of common_interfaces to signal the more specific project nature of it. I'm not pushing for that right now or in the near term future.

In any case, hopefully over the next few years of navigation work, we'll start being able to generalize things better. I'm slowly breaking up alot of the early assumptions and making things more flexible, but it takes time.

@naiveHobo, yes please make those updates.

Copy link
Member

Choose a reason for hiding this comment

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

These messages are here to support the work happening in the navigation stack

I've never thought about the nav_msgs package this way before. Though indeed it is written on the wiki and in the README of this package that they are for the navigation stacks. Personally, I've used them in several other projects that have not used any of the other elements of the nav stack (ROS 1). It might be worth considering removing the emphasis on any particular navigation stack.

+1 for renaming to PoseParticle and PoseParticleCloud.

I think it would go a long way to get buy-in from maintainers of the other packages that produce similar data.
We can also elicit general feedback from the community with a Discourse post.


geometry_msgs/Pose pose

float64 weight
6 changes: 6 additions & 0 deletions nav_msgs/msg/ParticleCloud.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# This represents a particle cloud containing particle poses and weights

std_msgs/Header header

# Array of particles in the cloud
Particle[] particles