Add PCLVisualizer::containsCloud.

[sotte]

Hey,

I "needed" the following function and it's just two lines of code. It basically allows you to check the PCLVisualizer contains the cloud with the given id independent of the type of the cloud.

Let me know what you think.

---

containsCloud allows to check if the viewer already contains a cloud with the
given id.

updatePointCloud also checks this AND updates the point cloud, however,
in many scenarios only the check is important.
containsCloud is much clearer and only requires the id of the cloud.

Why containsCloud:
In many tutorials the user keeps track of the added pointclouds by hand, i.e.,
flags like graph_added are used.
This is necessary because the updatePointCloud cannot be used.
containsCloud solves this issue.

[VictorLamoine]

cloud not clound 😉

[sotte]

And this is why reviews are a good idea, thanks! :)

[taketwo]

👍

There is one subtlety that's worth documenting. For some reason when you add a polygon mesh (with addPolygonMesh()), the created actor ends up in cloud_actor_map_. Therefore your check will be give false positive if a polygon mesh with the given id already exists.

I would add a note/warning to the docstring to avoid confusion.

[sotte]

I searched a bit more and pretty much everything you add to the viewer ends up in the cloud_actor_map_, i.e., all functions that start with add extend the cloud_actor_map_.
Here is an imcomplete list:

• addPointCloud
• addPolyModelFromPolyData
• addTextureMesh

I guess the underlying problem is that the concept of the cloud_actor_map_ is not really explained anywhere and the name implies that it only deals with clouds, which is not the case.

Anyway, should we mention previously named functions as well? Or should we rename the function? containsId, containsInput, containsXXX?

[taketwo]

Maybe this function should also check shape_actor_map_?

[sotte]

Oh man, I didn't even know that shape_actor_map_ exists. There is also coordinate_actor_map_.

I don't know what is the best way to proceed. How about this:

• containsCloud(const std::string& id) only checks the cloud_actor_map_
• containsShape(const std::string& id) only checks the shape_actor_map_
• containsCoordinate(const std::string& id) only checks the coordinate_actor_map_ (Is this function really necessary? I never worked with coordinates.)
• contains(const std::string& id) checks all of the above, basically checking if something with the given id was added.

[taketwo]

Or we can enforce uniqueness of the name independently of the particular actor map where it ends up. Then only a single contains function will be needed. What do you think?

[sotte]

This would mean that every time we want to add "stuff" we have to check if the id aleady exists in one of the three actor maps, right?

This would require quite a bit of refactoring (nothing hard but quite a few lines of code). My two-liner is turning into something bigger but I guess I could implement it.

Would this somehow influence the backward compatibility?

[taketwo]

The refactoring will amount to changing lines like:

ShapeActorMap::iterator am_it = shape_actor_map_->find (id);
if (am_it != shape_actor_map_->end ())
{
   // ...

with which every add function starts already to just a call to your new contains function.

Would this somehow influence the backward compatibility?

Well may give problems to those who used objects of different types with the same name. (But I have hard times imagining why someone would add a point cloud called "cloud" and then a sphere also called "cloud"). Anyway, a warning message will be printed, so it will be relatively easy to solve the problem for those who are affected.

[sotte]

OK, I'll implement it...

[sotte]

I just found https://github.com/PointCloudLibrary/pcl/blob/master/visualization/src/pcl_visualizer.cpp#L780.
There seems to be a certain semantic for ids with the same name in ShapeActorMap and CloudActorMap. Now I'm wondering if we don't break anything by requiering uniqueness.

[taketwo]

I don't think so. Check this commit message. It seems that there was/is quite a jumble with what goes where (to which actor map). For example, polygon meshes end up in cloud map, whereas polygons go to the shape map...

[sotte]

Some addPointCloud function do multiple things. Here is an example:

pcl::visualization::PCLVisualizer::addPointCloud (
   const typename pcl::PointCloud<PointT>::ConstPtr &cloud,
   const GeometryHandlerConstPtr &geometry_handler,
   const std::string &id, int viewport)
{
  CloudActorMap::iterator am_it = cloud_actor_map_->find (id);
  if (am_it != cloud_actor_map_->end ())
  {
    am_it->second.geometry_handlers.push_back (geometry_handler);
    return (true);
  }
  PointCloudColorHandlerCustom<PointT> color_handler (cloud, 255, 255, 255);
  return (fromHandlersToScreen (geometry_handler, color_handler, id, viewport, cloud->sensor_origin_, cloud->sensor_orientation_));
}

If id was not added yet, the data is added.
If id was found, the handler is updated.

Should the later case not be handled in the update function?

[taketwo]

Hm... maybe. But that's a different issue, isn't it?

Could you please squash the first two commits? Also please correct a few whitespace/line break issues in the last commit (lines 85, 124, 614).

👍 for the elaborate commit message!

[sotte]

I just wanted to mention the issue though.

I squashed the two commits. But I'm not sure about the whitespaces. Looks ok to me. What exactly do you mean?

[taketwo]

Extra space before and missing space after contains

[taketwo]

Thanks! I left inline comments.

[sotte]

Thanks! Updated.