Improved manual registration - #5530
Conversation
…/pcl into fix-manual-registration
5d3e0ef to
437218a
Compare
437218a to
c89cc79
Compare
manureg.mp4 |
|
Would someone kindly take a peek at this? I'd like to get it into master. |
|
We won't merge anything unless its broken, since we are mid-release of 1.13. So there will be a while before it will be merged. |
|
Looks good - but why did you change from PointXYZRGBA to PointXYZ? |
|
The reasoning was that in my field of mobile scanning, having rgb data is actually rare and most sensors deliver xyzi. Clouds without rgb show up black in the viewer, which renders them hard to see or invisible, depending on background. So it seemed more reasonable to me to use the common denominator of most pointcloud channels which is xyz.
Is this an undesirable change?
|
|
Well I was just curious, but I think it makes good sense. Also since the color is not used for the registration, nor in ICP, I'm fine with it being XYZ. |
Remove unused variables.
|
I have opened a PR towards your PR - mainly removing the two timers, to avoid the heavy re-rendering every 5 ms in main app and every 10 ms in the modal dialog. |
9a8cd50 to
d8614c4
Compare
Some more suggestion for perf improvement.
|
Tested and it still works. |
…ish/pcl into better-manual-registration
|
Might someone hit the button on this? |
larshg
added
the
changelog: enhancement
| <!-- <string>Undo</string> --> | ||
| <!-- </property> --> | ||
| <!-- </widget> --> | ||
| <!-- </item> --> |
There was a problem hiding this comment.
Is this still useful for some purpose, or why did you keep it around?
| pcl::registration::TransformationEstimationSVD<pcl::PointXYZ, pcl::PointXYZ> tfe; | ||
| tfe.estimateRigidTransformation(src_pc_, dst_pc_, transform_); | ||
| std::cout << "Transform : " << std::endl << transform_ << std::endl; | ||
| PCL_INFO("Calculated tranform:\n%1.5f %1.5f %1.5f %1.5f\n%1.5f %1.5f %1.5f " |
There was a problem hiding this comment.
How about PCL_INFO_STREAM("Calculated transform" << std::endl << transform_ << std::endl);
It is more compact and Eigen can take care of formatting, alignment of decimal points etc. (one more in refinePressed)
There was a problem hiding this comment.
That would be better, i just copied what the original code already used.
There was a problem hiding this comment.
addressed in eea0068.
however, I have noticed that swichting to this method now subjects the output to some terminal color shenanigans, which didnt happen before. E.g. in my solarized iterm the output now looks like this
Presumably because that macro has this in it pcl::console::change_text_color(CSTR, pcl::console::ATTR, pcl::console::FG);, while the PCL_INFO() and friends print without this.
| refreshView(); | ||
| PCL_INFO("Refining transform ...\n"); | ||
| VoxelGrid<PointT> grid_filter; | ||
| grid_filter.setLeafSize(0.05, 0.05, 0.05); |
There was a problem hiding this comment.
I don't like that these parameters (and the ones of icp below) seem to be optimised for your used case, with no possibility to change them, e.g. from the command line. But I am okay with this for now, we can deal with it if someone actually has a problem with the parameters.
There was a problem hiding this comment.
Since its an GUI app I would prefer a field to set them in 😄
There was a problem hiding this comment.
Same, it just increases the complexity of the change. Considering the feature was previously not even implemented … I'd still say it's better than before.
There was a problem hiding this comment.
I'd still say it's better than before.
@themightyoarfish I agree, that is why I am fine if you don't make the parameters configurable at runtime. But please address my other two review comments.
3 participants
I find the manual registration tool very useful. This modification has the following features