pcl
pcl copied to clipboard
PointCloudLibrary
Point and area picking improvement for get name of selected cloud
Changes are made for point and area picking event Added new methods that user can select cloud for which wants indices, or get all selected clouds (for area picking). Get name for point picking.
The behavior of picking events has been subtly wrong for a long time, so this PR is very welcome. However, I believe we should be careful about not changing the current default behavior. That is, the new (correct) behavior should be an opt-in.
Most parts of this PR are in private areas; the only user-facing classes are AreaPickingEvent/PointPickingEvent. So I think it is a great opportunity to redesign them slightly. Let's take the former as an example. Right now, the user iteracts with it through the method:
bool AreaPickingEvent::getPointsIndices (std::vector<int>& indices) const
We can keep this method signature intact together with the old behavior. The new behavior with per-pointcloud indices does not have to replicate this (dated) interface. Vectors can be returned by value. There can be a method to query if a certain cloud has its points selected. We can have an operator[] to access indices based on pointcloud name. @kunaltyagi you are very much into modern C++, what would you suggest?
Both classes could are functionally equivalent to std::optional<std::map<std::string, std::vector<pcl::index_t>>>.
We can either derive the classes privately, or create the required interfaces. A nice interface would be:
- some elements of
optionalinterface because currently that information is not available. This will allow things likeif (!area_pick_event) { /* no point selected*/ } - better ctor,
[]operators - iteration capability (since we are providing indexed operations anyways)
- similar interface as map (insert, edit), so
performAreaPickis able to return anAreaPickingEvent. This might be controversial but it allows a downstream callback to apply filters on the indices and keep propagating it further instead of copying because the internals are currently const past initialization - return by value instead of out param (better optimizations possible in modern c++)
This is a conservative list. If I was refactoring visualization module, I'd create 3 classes, one to denote pre-event (eg: information incomplete), one for the actual event (official event) and one post-event (eg: filtered), and this is possible using tagged template method.
class A_impl;
template <typename T>
class A;
template <>
class A<struct incomplete>: public class A_impl {};
template <>
class A<struct raw_event>: protected class A_impl
{
// implicit copy/move from A<incomplete>
// expose only the const iterface
}
template <>
class A<struct filtered>: public class A_impl
{
// implicit copy/move from A<raw_event>
// deleted copy/move from A<incomplete>
}
This last bit allows confidence in the interface without much verbosity, but it's for situations where there's a pipeline with the same data being operated upon and you want to express the pipeline at compile-time while also allowing easy callback based extensions.
@taketwo Behavior of classes areapicking and point picking are the same and user have now more options to select points based on cloud name. @kunaltyagi Proposed new classes for interface are out of my knowledge of c++ (only basics) so thats task for you.
Behavior of classes areapicking and point picking are the same and user have now more options to select points based on cloud name.
Right, sorry that I did not articulate it well enough. I agree that the PR does preserve the old behavior. My main point was that we should think about a nicer new interface (but keeping the old one).
some elements of optional interface because currently that information is not available. This will allow things like if
(!area_pick_event) { /* no point selected*/ }
If there are no points selected, then no callback is fired? Don't see a use-case for an empty event.
better ctor, [] operators
:+1:
iteration capability
Similar to map, iterate over pairs of (cloud name, indices)?
similar interface as map (insert, edit),
This is going a bit too far, perhaps. But can be considered in future.
return by value instead of out param
:+1:
If there are no points selected, then no callback is fired?
Then we can simply remove the code dealing with checking if some points have been selected. Would simplify the class
@janekT do you happen to have a test app that you can share so I can play around with the new feature?
I am using it in my app called 3D Forest You can find it in my repositories. I use such functions when I want want to adjust cloud when more clouds are displayed. So I can find corresponding indices for edited cloud.
@janekT please make use of git apply and apply all my proposed changes in a single commit. Here are some instructions on how to accomplish that
https://stackoverflow.com/a/12320940/9458162
You'll need to drop the commits you added (see interactive rebasing) after my review in order for the patch to apply without issues.
Edit: If you want to apply changes after, you can do it and now they'll be separated from the changes I proposed and easier to see what changed.
@SergioRAgostinho sorry for that mess, I am stucked into browser changes now, so that why I. made changes file by file, and then correct typos.
No worries. If you prefer I can directly clean up your branch. Is this ok by you?
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
Setting milestone to 1.12 since it deprecates, and let's not add deprecation in a minor release
PointPickingCallback::performAreaPick changes the API. Do we consider that an an internal API?
If yes, then this creates a ABI change (due to std::vector -> std::map in the AreaPick class). If no, then this is an API change (and @SergioRAgostinho has mentioned that API only label is to be used in case of API + ABI change)
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.
Seems like these changes are contained completely in https://github.com/PointCloudLibrary/pcl/pull/5476 , so I am closing this one.
