Add `pcl::weak_ptr` to have a single-switch move from `boost::` to `std::` weak pointers #3753

aPonza · 2020-03-15T11:06:34Z

Stems from #3750

aPonza · 2020-03-15T13:37:01Z

Hah! It is in the public API, it's being passed around after the lock in the .cpp file. I don't think there's a way, is there?

kunaltyagi · 2020-03-15T15:08:52Z

I don't think there's a way, is there?

Creating a pcl::weak_ptr and then replacing with std in the alias would be one.

aPonza · 2020-03-15T15:35:07Z

I have it locally, but it seems way too much for a single instance honestly, don't you think?

aPonza · 2020-03-15T15:37:37Z

Here you can see what I had

kunaltyagi · 2020-03-15T15:39:57Z

Another option is to create a local type-alias, like used for all shared_ptr ClassName::Ptr complemented by ClassName::WeakPtr

aPonza · 2020-03-16T00:23:14Z

Going down the rabbit hole of fixing the weak pointers leads to the app Point Cloud Editor. There, it becomes an exercise in futility as there's a circular dependency between the classes Cloud and Selection impeding progress if one wanted to have nice Cloud::Ptrs and Selection::Ptrs together with the Selection::WeakPtr. There only one SO suggestion atm but it's even more work and it doesn't really fit with the rest of the codebase.

I stand by the idea of leaving the toggle where it is currently for std::weak_ptrs. I'll push the non-compiling code so you can cringe with me. To continue here you really need to love the idea of this pcl::weak_ptr.

kunaltyagi · 2020-03-16T05:52:13Z

Some of the patch could be integrated as is, while other needs change (for the same reasons as circular dependency). Only public facing API needs to be type-aliased, and I don't think apps module fits that (but io does).

There shouldn't be a circular dependency. The reason is that the class which uses the type-alias is the one declaring it, not the class being used. If this were not the case, every class will need to come with it's own Ptr, WeakPtr declarations and impose them on downstream users. We don't care how the downstream users use our class, only that the internals of the class remain consistent.

#include <memory>
#include <boost/shared_ptr.hpp>
//---
class A;
class B;

class A{
using DataPtr = std::shared_ptr<B>;
DataPtr data;
};
class B {
using ControllerPtr = boost::shared_ptr<A>;
using ControllerWeakPtr = boost::shared_ptr<A>;
ControllerWeakPtr controller;
ControllerPtr fixController();
};

aPonza · 2020-03-16T09:04:10Z

Only public facing API needs to be type-aliased, and I don't think apps module fits that (but io does).

You think I should keep the typedef confined to io?

the class which uses the type-alias is the one declaring it, not the class being used

Are you suggesting to remove these (in PointCloudEditor only) in favour of private typedefs with the full name CloudPtr, SelectionWeakPtr? I thought it would be inconsistent with the rest of the PRs tackling this. If we were sticking to using Ptr, ConstPtr and therefore WeakPtr, what would you include/forward declare in:

../selection.h:

pcl/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/selection.h

Lines 55 to 65 in fb8ca1b

    
           class Selection : public Statistics 
        
           { 
        
             public: 
        
               /// The type for shared pointer pointing to a selection buffer 
        
               using Ptr = pcl::shared_ptr<Selection>; 
        
               /// The type for shared pointer pointing to a constant selection buffer 
        
               using ConstPtr = pcl::shared_ptr<const Selection>; 
        
               /// The type for weak pointer pointing to a selection buffer 
        
               using WeakPtr = pcl::weak_ptr<Selection>;

pcl/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/selection.h

Lines 67 to 72 in fb8ca1b

    
           /// @brief Constructor. 
        
           /// @param cloud_ptr A pointer to the const cloud object for which this 
        
           /// object is to maintain selections. 
        
           Selection (Cloud::ConstPtr cloud_ptr, bool register_stats=false) 
        
             : cloud_ptr_(std::move(cloud_ptr)) 
        
           {

../cloud.h:

pcl/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h

Lines 80 to 87 in fb8ca1b

    
           class Cloud : public Statistics 
        
           { 
        
             public: 
        
               /// The type for shared pointer pointing to a cloud object 
        
               using Ptr = pcl::shared_ptr<Cloud>; 
        
               /// The type for shared pointer pointing to a constant cloud object 
        
               using ConstPtr = pcl::shared_ptr<const Cloud>;

pcl/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h

Lines 212 to 216 in fb8ca1b

    
           /// a faster rendering mode; this also occurs if the selection object is 
        
           /// empty. 
        
           void 
        
           setSelection (const Selection::Ptr& selection_ptr);

kunaltyagi · 2020-03-16T10:58:40Z

in favour of private typedefs with the full name CloudPtr, SelectionWeakPtr?

Yeah, sort of, except replace with public type alias if the type is used in public API, else no type alias needed.

I thought it would be inconsistent with the rest of the PRs tackling this

PCL hasn't been perfect over the years, and as such there have been exceptions in the code. The best example would be use of Indices in pcl_base.h based on PointIndices.h.

IndicesPtr is declared outside the class and Ptr inside PointIndices, though have the same underlying type. Another type alias is created (PointIndicesPtr) in pcl_base but not preferred. However, if we were to look beyond the usage, at the usage, we see that:

only PointIndicesPtr is strictly needed, and available in most classes (I didn't number crunch this, but such aliases were present in all 50 classes that I saw)
IndicesPtr is a type-alias to prevent creating this new alias everywhere
PointIndices::Ptr is not beneficial in any sense, and is seldom used (X::Ptr usage is 3.5k compared to 10k usage of XPtr, aka like 1 and 2)

If we were sticking to using Ptr, ConstPtr and therefore WeakPtr, what would you include/forward declare in

That's the issue. This approach is ok in most cases, but fails when it causes circular references. On the other hand, using approach 1 and 2 (declared when needed and externally declared) don't cause such issues.

aPonza · 2020-03-21T14:58:19Z

Notes on 4041300: is this how you intended it to be? If it is, I can finish up by removing all the using declarations on localTypes.h so that the whole app compiles with aliases in each class.

kunaltyagi · 2020-03-22T07:17:56Z

is this how you intended it to be?

Yes. Please note that you can exclude aliases for something not in public domain to reduce the work needed to be done.

aPonza · 2020-03-25T22:47:17Z

I was doing it but realized there's no real reason to remove aliases in localTypes.h in this PR. This is currently the minimum to get this std::weak_ptr "nice" switch working. What are the required changes to get this merged after a rebase?

common/include/pcl/memory.h

kunaltyagi · 2020-03-26T06:32:15Z

apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h

@@ -412,7 +421,7 @@ class Cloud : public Statistics
    /// @brief A weak pointer pointing to the selection object.
    /// @details This implementation uses the weak pointer to allow for a lazy
    /// update of the cloud if the selection object is destroyed.
-    std::weak_ptr<Selection> selection_wk_ptr_;
+    SelectionWeakPtr selection_wk_ptr_;


Apart from here, this isn't used anywhere else. Is this a public variable (named like a protected/private variable)?

pcl/apps/point_cloud_editor/src/cloud.cpp

Lines 184 to 199 in d6ac1e8

void

Cloud::setSelection (const SelectionPtr& selection_ptr)

{

selection_wk_ptr_ = selection_ptr;

if (!selection_ptr || selection_ptr->empty())

return;

IncIndex inc;

partitioned_indices_.resize(cloud_.size());

std::generate(partitioned_indices_.begin(), partitioned_indices_.end(), inc);

unsigned int pos = 0;

// assumes selection is sorted small to large

for (auto it = selection_ptr->begin(); it != selection_ptr->end(); ++it, ++pos)

{

std::swap(partitioned_indices_[pos], partitioned_indices_[*it]);

}

}

That's not an interface boundary

If this pointer becomes a boost::weak_ptr due to being a pcl:weak_ptr then I can't convert it to a std::shared_ptr within this function. If this instead will not be changed to a pcl:weak_ptr then I was right in closing this PR 18 days ago: I thought it was clear I was continuing the work here and within this PCL app only because you indirectly said it was worth it:

I stand by the idea of leaving the toggle where it is currently for std::weak_ptrs. [...] To continue here you really need to love the idea of this pcl::weak_ptr.

from #3753 (comment)

I'd have otherwise left the micro change inside #3750 since it's about 4 lines worth like Sergio pointed out.

io/include/pcl/io/openni_camera/openni_driver.h

aPonza · 2020-04-02T01:56:33Z

Hey @kunaltyagi any feedback here?

SergioRAgostinho · 2020-04-02T09:04:36Z

Guys, this PR spiraled out of control. The last boost::weak_ptr replacement will need to happen alongside the shared pointer alias switch.

/pcl/io/src/openni_camera/openni_driver.cpp: In member function ‘openni_wrapper::OpenNIDevice:
:Ptr openni_wrapper::OpenNIDriver::getDeviceByIndex(unsigned int) const’:                                                      
/pcl/io/src/openni_camera/openni_driver.cpp:291:17: error: could not convert ‘device’ from ‘st
d::shared_ptr<openni_wrapper::OpenNIDevice>’ to ‘openni_wrapper::OpenNIDevice::Ptr {aka boost::shared_ptr<openni_wrapper::OpenN
IDevice>}’                                                                                                                     
   return (device);

This would have been enough

$ git diff
diff --git a/common/include/pcl/memory.h b/common/include/pcl/memory.h
index 4037c5ace..009f7ee91 100644
--- a/common/include/pcl/memory.h
+++ b/common/include/pcl/memory.h
@@ -47,6 +47,7 @@
 
 #include <boost/make_shared.hpp>  // for boost::allocate_shared, boost::make_shared
 #include <boost/smart_ptr/shared_ptr.hpp>  // for boost::shared_ptr
+#include <boost/smart_ptr/weak_ptr.hpp>  // for boost::weak_ptr
 
 #include <Eigen/Core>  // for EIGEN_MAKE_ALIGNED_OPERATOR_NEW
 
@@ -80,6 +81,9 @@ namespace pcl
 template <typename T>
 using shared_ptr = boost::shared_ptr<T>;
 
+
+using boost::weak_ptr;
+
 #ifdef DOXYGEN_ONLY
 
 /**
diff --git a/io/include/pcl/io/openni_camera/openni_driver.h b/io/include/pcl/io/openni_camera/openni_driver.h
index 6ece49406..9317a7853 100644
--- a/io/include/pcl/io/openni_camera/openni_driver.h
+++ b/io/include/pcl/io/openni_camera/openni_driver.h
@@ -43,6 +43,7 @@
 #include "openni_exception.h"
 #include "openni_device.h"
 #include <pcl/io/boost.h>
+#include <pcl/memory.h>
 #include <pcl/pcl_macros.h>
 
 #include <map>
@@ -217,7 +218,7 @@ namespace openni_wrapper
       std::shared_ptr<xn::NodeInfo> image_node;
       std::shared_ptr<xn::NodeInfo> depth_node;
       std::shared_ptr<xn::NodeInfo> ir_node;
-      boost::weak_ptr<OpenNIDevice> device;
+      pcl::weak_ptr<OpenNIDevice> device;
     } ;
 
     OpenNIDriver ();

Why is this PR touching ~1800 lines to address this problem?

aPonza · 2020-04-02T11:22:17Z

Most of those touched lines are formatting/spaces, so out of scope, I guess. The rest is to have all weak_ptr have the same singular "switch" inside memory.h instead of needing separate toggles. This is useless if the only plan is to deprecate the switch itself (pcl::weak_ptr) in favour of using std::weak_ptr directly. It is instead useful if at any time in the future a need arises to change the pointer's implementation.

aPonza · 2020-04-20T17:57:35Z

I'm pretty sure the error shows how all the edits in point_cloud_editor are mandatory:

/__w/1/s/apps/point_cloud_editor/src/deleteCommand.cpp:69:44: error: no matching function for call to 'Cloud::setSelection(SelectionPtr&)'
   cloud_ptr_ -> setSelection(selection_ptr_);
                                            ^
In file included from /__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/copyBuffer.h:44:0,
                 from /__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/deleteCommand.h:45,
                 from /__w/1/s/apps/point_cloud_editor/src/deleteCommand.cpp:41:
/__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h:213:5: note: candidate: void Cloud::setSelection(const SelectionPtr&)
     setSelection (const SelectionPtr& selection_ptr);
     ^
/__w/1/s/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h:213:5: note:   no known conversion for argument 1 from 'SelectionPtr {aka std::shared_ptr<Selection>}' to 'const SelectionPtr& {aka const boost::shared_ptr<Selection>&}'
apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/build.make:217: recipe for target 'apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/src/deleteCommand.cpp.o' failed
make[2]: *** [apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/src/deleteCommand.cpp.o] Error 1
make[1]: *** [apps/point_cloud_editor/CMakeFiles/pcl_point_cloud_editor.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

and therefore #3949 would become only a change in the realsense file. Should that be the case?

kunaltyagi · 2020-04-20T23:22:55Z

This error is because global SelectionPtr (in localTypes.h) is being masked by the class's internal SelectionPtr. We can resolve it by

preventing name clash (optional, but preferred) + switch the global SelectionPtr to pcl::shared_ptr
removing the global type and using class local types only (your suggestion, except you're keeping the global type)
modify the global type and remove class local types

I've no idea which is better for apps module.

SergioRAgostinho

Current state is ok for me. Let's get that CI green first though.

SergioRAgostinho · 2020-04-28T08:02:46Z

diff --git a/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h b/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
index fc66674ec..3d1fd80c9 100644
--- a/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
+++ b/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/cloud.h
@@ -78,11 +78,8 @@
 class Cloud : public Statistics
 {
   public:
-    /// The type for shared pointer pointing to a selection buffer
-    using SelectionPtr = pcl::shared_ptr<Selection>;
-
     /// The type for weak pointer pointing to a selection buffer
-    using SelectionWeakPtr = pcl::weak_ptr<Selection>;
+    using SelectionWeakPtr = std::weak_ptr<Selection>;
 
     /// @brief Default Constructor
     Cloud ();

This fixed it for me.

aPonza · 2020-04-28T17:50:26Z

The point was having a single switch. Changing the scope of the PR means going directly back to #3753 (comment) to me, there would be no point in being more verbose than those few line changes.

aPonza · 2020-04-28T17:51:57Z

And, reiterating, I believe all the changes in the now separate PR were due to this CI error we see now. I'll merge that bit in this PR and leave out only the API changes in that PR which are likely only the ones in RealSense scope.

kunaltyagi · 2020-04-30T11:17:04Z

The file localTypes.h has confusing entries now. The classes are overriding the SelectionPtr so it might make sense to remove the one unneeded local type to avoid confusion.

https://github.com/PointCloudLibrary/pcl/blob/master/apps/point_cloud_editor/include/pcl/apps/point_cloud_editor/localTypes.h#L77

aPonza · 2020-04-30T11:21:38Z

I absolutely know it, that's why the commit regarding point_cloud_editor is named as it is. ~~Also please refer to #3753 (comment) for why~~ I'd much rather create an issue and maybe work on it another time. It's a VERY long ordeal to remove that file's typedefs.

EDIT: the reason is I tried and it's a long ordeal, there are lots of files using that header.

kunaltyagi

Based on the discussion, this PR LGTM pending an issue

SergioRAgostinho

Also LGTM, feel free to merge if CI is green(ish)

Edit: CI is green. Shall we squash the first two and last two commits?

aPonza · 2020-05-02T13:31:12Z

Issue opened

kunaltyagi added this to the pcl-1.11.0 milestone Mar 15, 2020

kunaltyagi added changelog: ABI break Meta-information for changelog generation module: io needs: code review Specify why not closed/merged yet labels Mar 15, 2020

aPonza closed this Mar 15, 2020

aPonza deleted the weak_ptrs branch March 15, 2020 14:16

aPonza restored the weak_ptrs branch March 15, 2020 15:36

aPonza reopened this Mar 15, 2020

aPonza force-pushed the weak_ptrs branch from 1ff78da to fb8ca1b Compare March 16, 2020 00:28

aPonza force-pushed the weak_ptrs branch from fb8ca1b to 4041300 Compare March 21, 2020 14:55

aPonza mentioned this pull request Mar 25, 2020

Change PCL smart pointers from boost to std #3750

Merged

kunaltyagi reviewed Mar 26, 2020

View reviewed changes

aPonza force-pushed the weak_ptrs branch from 4041300 to 9d076db Compare March 26, 2020 20:30

SergioRAgostinho self-requested a review April 2, 2020 08:00

SergioRAgostinho approved these changes Apr 28, 2020

View reviewed changes

kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Apr 28, 2020

aPonza force-pushed the weak_ptrs branch 2 times, most recently from 7dadce9 to a9a0432 Compare April 28, 2020 19:58

aPonza marked this pull request as ready for review April 28, 2020 23:51

aPonza requested review from kunaltyagi and SergioRAgostinho April 30, 2020 10:58

SergioRAgostinho added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Apr 30, 2020

kunaltyagi approved these changes Apr 30, 2020

View reviewed changes

SergioRAgostinho approved these changes Apr 30, 2020

View reviewed changes

SergioRAgostinho removed the needs: code review Specify why not closed/merged yet label Apr 30, 2020

aPonza mentioned this pull request May 2, 2020

[apps] Deal with aliases in PointCloud Editor app #4013

Open

aPonza and others added 2 commits May 2, 2020 19:45

add pcl::weak_ptr and deal with openni_camera's boost::weak_ptr

ae3c8a6

deal with real_sense's and point_cloud_editor's std::weak_ptr

2196d68

aPonza force-pushed the weak_ptrs branch from a9a0432 to 2196d68 Compare May 2, 2020 17:46

kunaltyagi merged commit e135ec8 into PointCloudLibrary:master May 2, 2020

aPonza deleted the weak_ptrs branch May 3, 2020 12:30

aPonza mentioned this pull request May 5, 2020

[compile error] test_outofcore doesn't compile #4033

Closed

taketwo changed the title ~~Add pcl::weak_ptr to have a single-switch move from boost:: to std:: weak pointers~~ Add pcl::weak_ptr to have a single-switch move from boost:: to std:: weak pointers May 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `pcl::weak_ptr` to have a single-switch move from `boost::` to `std::` weak pointers #3753

Add `pcl::weak_ptr` to have a single-switch move from `boost::` to `std::` weak pointers #3753

aPonza commented Mar 15, 2020

aPonza commented Mar 15, 2020

kunaltyagi commented Mar 15, 2020 •

edited

Loading

aPonza commented Mar 15, 2020

aPonza commented Mar 15, 2020

kunaltyagi commented Mar 15, 2020

aPonza commented Mar 16, 2020 •

edited

Loading

kunaltyagi commented Mar 16, 2020

aPonza commented Mar 16, 2020

kunaltyagi commented Mar 16, 2020

aPonza commented Mar 21, 2020

kunaltyagi commented Mar 22, 2020

aPonza commented Mar 25, 2020

kunaltyagi Mar 26, 2020

aPonza Mar 26, 2020

kunaltyagi Apr 2, 2020

aPonza Apr 2, 2020 •

edited

Loading

aPonza commented Apr 2, 2020

SergioRAgostinho commented Apr 2, 2020 •

edited

Loading

aPonza commented Apr 2, 2020

aPonza commented Apr 20, 2020

kunaltyagi commented Apr 20, 2020

SergioRAgostinho left a comment

SergioRAgostinho commented Apr 28, 2020

aPonza commented Apr 28, 2020

aPonza commented Apr 28, 2020

kunaltyagi commented Apr 30, 2020 •

edited

Loading

aPonza commented Apr 30, 2020 •

edited

Loading

kunaltyagi left a comment

SergioRAgostinho left a comment •

edited

Loading

aPonza commented May 2, 2020

	void
	Cloud::setSelection (const SelectionPtr& selection_ptr)
	{
	selection_wk_ptr_ = selection_ptr;
	if (!selection_ptr \|\| selection_ptr->empty())
	return;
	IncIndex inc;
	partitioned_indices_.resize(cloud_.size());
	std::generate(partitioned_indices_.begin(), partitioned_indices_.end(), inc);
	unsigned int pos = 0;
	// assumes selection is sorted small to large
	for (auto it = selection_ptr->begin(); it != selection_ptr->end(); ++it, ++pos)
	{
	std::swap(partitioned_indices_[pos], partitioned_indices_[*it]);
	}
	}

Add pcl::weak_ptr to have a single-switch move from boost:: to std:: weak pointers #3753

Add pcl::weak_ptr to have a single-switch move from boost:: to std:: weak pointers #3753

Conversation

aPonza commented Mar 15, 2020

aPonza commented Mar 15, 2020

kunaltyagi commented Mar 15, 2020 • edited Loading

aPonza commented Mar 15, 2020

aPonza commented Mar 15, 2020

kunaltyagi commented Mar 15, 2020

aPonza commented Mar 16, 2020 • edited Loading

kunaltyagi commented Mar 16, 2020

aPonza commented Mar 16, 2020

kunaltyagi commented Mar 16, 2020

aPonza commented Mar 21, 2020

kunaltyagi commented Mar 22, 2020

aPonza commented Mar 25, 2020

kunaltyagi Mar 26, 2020

Choose a reason for hiding this comment

aPonza Mar 26, 2020

Choose a reason for hiding this comment

kunaltyagi Apr 2, 2020

Choose a reason for hiding this comment

aPonza Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

aPonza commented Apr 2, 2020

SergioRAgostinho commented Apr 2, 2020 • edited Loading

aPonza commented Apr 2, 2020

aPonza commented Apr 20, 2020

kunaltyagi commented Apr 20, 2020

SergioRAgostinho left a comment

Choose a reason for hiding this comment

SergioRAgostinho commented Apr 28, 2020

aPonza commented Apr 28, 2020

aPonza commented Apr 28, 2020

kunaltyagi commented Apr 30, 2020 • edited Loading

aPonza commented Apr 30, 2020 • edited Loading

kunaltyagi left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

aPonza commented May 2, 2020

Add `pcl::weak_ptr` to have a single-switch move from `boost::` to `std::` weak pointers #3753

Add `pcl::weak_ptr` to have a single-switch move from `boost::` to `std::` weak pointers #3753

kunaltyagi commented Mar 15, 2020 •

edited

Loading

aPonza commented Mar 16, 2020 •

edited

Loading

aPonza Apr 2, 2020 •

edited

Loading

SergioRAgostinho commented Apr 2, 2020 •

edited

Loading

kunaltyagi commented Apr 30, 2020 •

edited

Loading

aPonza commented Apr 30, 2020 •

edited

Loading

SergioRAgostinho left a comment •

edited

Loading