Pcl: Magic numbers in organized_segmentation_demo.cpp

[aPonza]

• Operating System and version: Ubuntu 16.04
• Compiler: gcc8
• PCL Version: 1.7.2

Trying to understand and use this in an application I found many magic numbers in this source file. I'm mainly wondering if I understood correctly, but this can also help improve the source so I'll post here:

• this line sets the same value as the default: comment out?

• this line and the next one set the same values as the default ones: comment out?

• could this line be rewritten as if (label_indices[i].indices.size() > mps.getMinInliers())?

I have also substituted all of the new calls with boost::make_shared but you're currently working to switch to std:: stuff, so I guess you don't care here.

[SergioRAgostinho]

The app was written by @atrevor a long time ago, so he's the best person to comment about it, although I suspect after 7 years, he will be also trying to guess what was the purpose behind these numbers.

Nevertheless, your suggestions are all sound with the minor addendum we usually don't comment out code, we simply delete it.

I have also substituted all of the new calls with boost::make_shared but you're currently working to switch to std:: stuff, so I guess you don't care here.

We are indeed proceeding with those replacements. For, everything related to the boost -> std conversion, feel free to submit a separate pull request. Those are priority items in the work queue, so they get quick feedback and reviews.

[atrevor]

Hello and thanks for the cleanup here! aPonza, I took a quick look at your suggestions and agree that we can remove the lines in the demo that set values to their defaults, and also with your substitution for the inlier count. If there is a specific question about the meaning of any of these parameters, I do still have some recollection of how they were chosen -- feel free to ask.

In general, the defaults were set to perform well for tabletop objects as imaged by a primesense sensor, but can be adapted for other use cases. Improvements to the parameter documentation in the MPS header are of course welcome also!

[aPonza]

Don't really want to open an issue for what is essentially a question (mainly for @atrevor) related to this app again.

pcl::EdgeAwarePlaneComparator<PointT, pcl::Normal>::Ptr eapc = boost::dynamic_pointer_cast<pcl::EdgeAwarePlaneComparator<PointT, pcl::Normal> >(edge_aware_comparator_);
eapc->setDistanceMap (distance_map);
eapc->setDistanceThreshold (0.01f, false);

I don't understand the purpose of these three lines (link) since you're never referencing eapc again.

1) Not being able to use this UI I'm moving the procedure to ROS: I choose the comparator at runtime via a config file, so for any given run I have a set PlaneCoefficientComparator type. You are instead modifying the edge_aware_comparator_ member, so the cast is always successful, but maybe useless? Isn't it casting to its own type anyways? In my case instead it would be useful, since I'd be casting a pointer to base.

2) The setDistanceThreshold call is independent of the type of PlaneCoefficientComparator but the setDistanceMap one depends on you having an EdgeAwarePlaneComparator so that'd be my reason to use the dynamic cast (and on this, is the only alternative having reflection in C++?) but then you don't check if the cast is successful: you don't really have to, if my reasoning in point 1 is sound, but otherwise this'd be undefined behaviour if I understood correctly how a dynamic cast works.

[SergioRAgostinho]

@aPonza This was likely unreviewed code. I assumed it suffered a couple of iterations and by the time it was committed to the repo, that dynamic_pointer_cast was no longer useful. Taking a reference is sufficient.

const pcl::EdgeAwarePlaneComparator<PointT, pcl::Normal>::Ptr& eapc = edge_aware_comparator_;

In your case, if you're casting to the base class use a static_pointer_cast. You don't need the dynamic check up because you know the cast will be successful in advance.

[aPonza]

So you think these used to work for the the paper but now don't accomplish anything at all? I mean, he's never setting the distance map or the distance threshold like this, unless I'm missing/misunderstanding something.

Off topic, but I'm casting base class to derived, I only know at runtime (reading the parameter set by the config file) which kind of comparator I'm using. It's probably not optimal to check at every iteration, but I'm still trying to make it work, the results are not good at the moment, not even sure why.

if (auto eapc = boost::dynamic_pointer_cast<pcl::EdgeAwarePlaneComparator<pcl::PointXYZRGBA, pcl::Normal> >(plane_comparator_))
  eapc->setDistanceMap(distance_map.getData());

[SergioRAgostinho]

So you think these used to work for the the paper but now don't accomplish anything at all? I mean, he's never setting the distance map or the distance threshold like this, unless I'm missing/misunderstanding something.

My comment was only on the way that particular step is implemented in C++. Notice the cast is only making a copy of the pointer to the object. It is still referencing the same edge_aware_comparator_ member and therefore

eapc->setDistanceMap (distance_map);
eapc->setDistanceThreshold (0.01f, false);

are actually changing the properties of edge_aware_comparator_. So from a functionality perspective, it is still doing something. The implementation is just slightly misleading.

[aPonza]

copy of the pointer to the object

I was indeed missing something, thanks Sérgio!