Add nav2_msgs/ParticleCloud display plugin #541
Conversation
|
I had to add nav2_msgs as a dependency for rviz_default_plugins which might be a problem since nav2_msgs is not part of the default ros2 packages right now. |
|
We can also move those messages into We would really like to get this in for Foxy as this is now our state of master branch that will be the nav2 foxy release. We will keep backwards compatibility with the pose array for a few months but it will be dropped relatively quickly because this interface is superior. |
..._plugins/include/rviz_default_plugins/displays/particle_cloud/flat_weighted_arrows_array.hpp
Show resolved
Hide resolved
|
@clalancette can we get some movement on this? It is blocking some efforts on navigation. |
There was a problem hiding this comment.
Overall, I'd like to hear reasons we should pull this into the core and not just have it live in navigation2. It seems pretty nav-specific, and adding new stuff this way will increase the core build times. Meanwhile, navigation2 seems to have all of the infrastructure and dependencies it needs to host this.
| @@ -63,6 +63,7 @@ find_package(interactive_markers REQUIRED) | |||
| find_package(laser_geometry REQUIRED) | |||
| find_package(map_msgs REQUIRED) | |||
| find_package(nav_msgs REQUIRED) | |||
| find_package(nav2_msgs REQUIRED) | |||
There was a problem hiding this comment.
As you discussed, this could be a problem. I really don't want to add more dependencies to the core, as it is way too big already.
Stepping back, why can this plugin not live in nav2_rviz_plugins? You already have the dependency available there and you have all of the infrastructure.
There was a problem hiding this comment.
I'd be more than happy to move this message to nav_msgs, so if you're OK with that then, I'll make that PR. We can discuss a home below.
There was a problem hiding this comment.
I think we should see it moved to nav_msgs or have nav2_msgs accepted into common_interfaces, which is ever is more suitable (sounds like the former). This step ensures the message type being visualized is broad and generic enough to justify shipping with rviz, and is not specific to the nav2 project specifically.
|
So the home, here are my thoughts.
So if Navigation no longer supports the 2D navigation topic ( Even a step more, since this is a general message for a particle cloud, it could have uses for other forms of particle cloud visualization from the Going down the "slippery slope trail" for a moment: Keeping the list of plugins here static seems like it would be a mistake and result in overall stagnation of the project. Even many of these I think should probably be merged here as well for those same reasons to keep RVIZ relevant. Its one of the biggest drawers of users to ROS from industry and more support and more types I think is only better. |
It's not that I (necessarily) want to keep it static. But building But I'm sure my opinion isn't universally shared. I guess I'll ask @wjwwood and @ros2/team for some other feedback here. |
|
Maybe we need some sort of marketplace inside RViz to make it easy to discover and install plugins and then have a discussion about which plugins are absolutely essential and move the others to other packages? And instead add more "helpers" for plugin development (maybe in a separate package)? |
|
I wouldn't push moving the Navigation2 plugins like the lifecycle manager interface / waypoint follower interface here - but for visualizing basic robotics display types, I really think they should live and be shiped with rviz for out of the box use. |
|
The default plugins is a compromise between the concerns raised here. I really don't want to be adding new message packages and paradigms for the reasons @clalancette pointed out (about this being the whole point of plugins), but if it's a new message that is accepted as broadly useful enough to make it into "common interfaces" then I think it's ok to add it to the default plugins. As @SteveMacenski pointed out, really custom stuff like the lifecycle of nav2 specifically or the "waypoint follower", would not be good candidates for inclusion here, imo. As for the performance issues of CI, that's a reasonable thing to consider, but I wouldn't use it as the deciding factor in a case like this. |
I don't understand this, why can this plugin not live in As I see it the necessary steps are (and I know this is only becoming clear now, so I'm not trying to point any fingers):
I think we're way too close to foxy to be considering it at this point, especially since it requires additions to common interfaces. If it ends up being completely new stuff (seems to be) then we can add it in a patch release of foxy most likely, but that also means it can wait until things settle down in the release pipeline. Basically, please don't make this precondition of progress in nav2, we're just going to slow you down. |
If the data structure is not specific to nav then maybe it should be in I know I said it already, but I think sorting all of this out is a necessary precondition for having a display plugin for it in rviz's default plugins. |
|
It could live in nav2 for now, but I'm hesitant to merge it if its final home is here. I don't want to merge it there under one name and then transfer it over here under another name (or as the same name and cause potential collisions in the middle of a distribution release cycle). We're currently supporting the PoseArray alongside this ParticleCloud message for backwards compatibility for Foxy. After we branch off Foxy, it will be removed for all future distributions. I don't like the idea of using a temporary fix in place of a permanent one. I'll wait for the permanent one. There's not a big reason to merge it in nav2 for the short term if we are keeping compatibility for Foxy with PoseArray. Msgs:
If you look over them, I'm not sure there's a whole lot there to argue with, but I have no problem with it not shipping out in the first debian form of Foxy. This isn't massively blocking. However, I imagine once people sit down to review it, it shouldn't be a big deal. On your point about geometry_msgs or other msgs, agreed. I'm not terribly concerned about the specific package its put into. I think @naiveHobo please submit a PR to https://github.com/ros2/common_interfaces with these 2 messages and we can follow up there with William and Co.
|
|
+1 for waiting on the message's inclusion into |
I'll just say, having a new plugin start out as in a separate package is way better process in my opinion. It let's people try it now more easily, and it lets you should the value of the plugin when trying to motivate a pull request like this one.
Wait, why not just use a point cloud? |
|
A custom templated pointcloud in rviz would still require a custom RVIZ plugin to represent the particles' orientations and scores/weights. |
|
Oh, I see, sorry, I misread the particle message. It would be interesting to have an option to visualize a set of channels in a pointcloud2 as a pose. That way you could use pointcloud2 and get all the tools that come with that automatically. |
|
I think a pointcloud is not really the right data structure. If it were just a point and a weight, yeah, I could make an argument for that. But because orientation is important, we're now talking about a PointXYZIRPY, which seems excessive. |
I'm not sure that I agree.
I don't think that it is. It's no more excessive than something like Also, now that I'm thinking about it more, "particle" doesn't really imply anything about orientation to me, I'm guessing it is terminology specific to the type of navigation filters you're using. If we want to generalize it would need to be something like pose cloud or pose with weight cloud or a version of pose array/cloud that has arbitrary channels attached similar to pointcloud2. Which leads me back to the point that the flexibility and structure of pointcloud2 was honed over a long period and is worth reusing if you can. |
Having a colored point from a RGB-D sensor is a reasonable thing to have because its derivative of a sensor for which a Point has some properties to put into a cloud. Having a More thematically: your point also then invalidates the concept of the PoseArray message, which was used prior and clearly has a different semantic meaning than a PointCloud, or I don't think it would exist. A particle in a particle filter to me is a set of quantizations of state of some noisy information. The fact that its called a "particle" shouldn't imply 2D to you or imply 3D pose to me. That's just nomenclature of a higher level concept that an MCL uses. This seems a little off topic, but we should have it somewhere, whether that's here or in the common_interfaces PR. Edit: I'm not sure I really care one way or another, I just want to make sure there's a principled reason behind it. Dragging PCL into projects especially should be well reasoned because that's a big dependency to casually add after years of effort to remove it in ROS1. |
|
At least there is a version of point for normals (which I don't feel is that different) in pcl: https://pointclouds.org/documentation/structpcl_1_1_point_x_y_z_i_normal.html
What does coming from a sensor have to do with it? The points in the point cloud library, while often used with RGBD cameras, are also general data structures that can be used in a variety of ways. It happens that Pointcloud2 is in
PoseArray adds some semantic meaning, but it's also older than both point cloud and point cloud 2 if I understand correctly. So, I think it's likely that if PointCloud2 had existed it might have not been created.
Which I think lends to my point that having a 3D pose in it makes it more than just a particle, it's a specific kind of particle. For the 3D/2D case we usually say, when doing 2D just use the 3D type and leave z as 0, but if your particle was storing something else, then this name really makes no sense anymore right?
I agree, I think a lot of what I'm asking now will be seen when considering it for inclusion in a general package. I do feel, however, that is would be nice to reuse and extend the point cloud display plugin in order to visualize this case, just because it is so flexible and polished. I could see the argument that having a semantically meaningful message is valuable for communicating between nodes and recording and architecture, but I also think it's ok to have something like
There's no need for pcl here I think. It's just a good reference for what "points" are being used for more broadly. We've managed to avoid it in a lot of places, e.g.: https://github.com/ros2/common_interfaces/blob/master/sensor_msgs/include/sensor_msgs/point_cloud2_iterator.hpp |
|
Lets pause and let @naiveHobo jump in as this is his work that I'm just advising / managing. Maybe he totally agrees with you and I'm outvoted and we just do it that way. Either way, before going much deeper on this, he should have his opinion voiced. The point on sensor data I was trying to make wasn't that PCs are only suited for sensors, but rather an example of where that type of information may derive from because the raw "thing" that it represents is a Point, which then has some attributes about it. The argument I was trying to make is that our core "thing" isn't a point, but a Pose and semantically representing a pose as a point in a pointcloud seems to me like a misrepresentation. All the example PointT's that I am aware of represent points with properties, which I think is enough different from pose with properties it doesn't quite belong. I can certainly live with it. I think it would be alot more difficult in every way however. I'm not sure there's an option for visualizing points as arrows in the pointcloud2 display, is that then an addition you would merge? Because then the only user of it is this plugin and invalid for all other uses of the pointcloud visualization type. |
|
I have to say I agree with @SteveMacenski here. There are several messages which can technically be used to carry particle cloud information (visualization_msgs/MarkerArray, or PointCloud2 as @wjwwood pointed out). But the point here is that a message specifically aimed to carry particle cloud information does justice to the concept of particle clouds which is a core part of robotics. We are trying to conform particle clouds to the existing messages to support re-usability but I think a Using PointCloud2 definitely makes it easier to integrate from the visualization perspective but having a core message type to represent particle clouds is semantically the way to go in my opinion. |
|
Here's the PR on |
|
Ok, fair enough. I do think that a "mature" version of I still have mixed feelings on the name, given particles (even in robotics) need not be poses. But I can comment over there. |
We can always update later based on developing needs. If there's a need that appears that would like to use this, we're open to changes. I prefer to be fluid and then optimize when required - I don't foresee this being something alot of people will need to or want to extend. If you foresee a different future for this and you believe that people will, practically, make substantive changes to a message like this for their different needs, then I agree, maybe we should go the pointcloud route. wrt the rviz plugin, I still feel that the pointcloud display type with arrows would be more of a user net-negative, since functionally no other PC2 type would use the arrows markers as they lack orientation data. I would though yield that adding a PC2 arrow type could be useful for representing potential fields. That would be nice. |
This sort of indicates to me that these messages are not ready for standardization, and therefore are not ready for inclusion in
I think it would be useful for Normals, which are just points with orientations associated with them. |
This is representative of my style, not of the selection of this message. I'd say the same thing about We can discuss in the other thread now. Link for posterity: ros2/common_interfaces#118 (review) |
Closes #538
Original navigation2 PR ros-navigation/navigation2#1688
Added a display plugin for nav2_msgs/ParticleCloud which closely resembles geometry_msgs/PoseArray display, but scales arrows based on the allotted weights.