Refactor `MaskMap` and deprecate several of its methods #3399

lightyear15 · 2019-10-04T18:39:32Z

Quick refactor to mask_map to make it more into C++14

"default" to mark default constructor and destructor
default values for member variables instead of default constructor
avoid output parameters passed by reference, RVO will make the job for
us

This PR deprecates void getDifferenceMask(const MaskMap&, const MaskMap&, MaskMap&) in favor of a new overload with return value instead of output parameter.

lightyear15 · 2019-10-06T20:29:00Z

Just a small pr to introduce myself.
I saw you have an ongoing project of modernizing the codebase and ( if I'm not mistaken) get read of boost dependencies and replace it with std wherever possible.
I would love to give an hand on this.

kunaltyagi

Hi! Sorry for the delay. Real life gets in the way for the maintainers sometimes :)

I've just taken a quick look Thanks for the contribution in advance

recognition/src/mask_map.cpp

taketwo

Hey, welcome to PCL, and thanks for contributing!

recognition/src/mask_map.cpp

taketwo · 2019-10-07T19:57:35Z

recognition/include/pcl/recognition/mask_map.h

@@ -65,10 +65,16 @@ namespace pcl
      inline const unsigned char* 
      getData () const { return (&data_[0]); }

+      [[deprecated("output parameter is not desirable,\


Suggested change

[[deprecated("output parameter is not desirable,\

[[deprecated("use getDifferenceMask() with return value instead")]]

For deprecation messages, we generally try to stick to "use %signature% instead" template. The %signature% part does not have to be a formal signature but should be sufficient to understand which function/overload is meant.

recognition/include/pcl/recognition/mask_map.h

taketwo

Sorry for the noise. Time and again, I press "approve" instead of "comment"/"request changes" when finishing a review, grrr.

kunaltyagi

This set of comments is just nitpicks. Not required. LGTM

recognition/include/pcl/recognition/mask_map.h

recognition/src/mask_map.cpp

kunaltyagi

One minor improvement for the PCL_NODISCARD macro please

common/include/pcl/pcl_macros.h

recognition/include/pcl/recognition/mask_map.h

kunaltyagi · 2019-10-11T16:13:57Z

🚀 if CI is a go

lightyear15 · 2019-10-14T10:05:44Z

Build.Ubuntu fails because of "no space left on the device"
Anyone knows what is going on?
As a side note regarding compilation: sometimes, compiling some CU takes up more than 4 GB of ram, which is definitely too much for my poor old laptop. Is there any intent to fix or mitigate this issue? Is it considered an issue at all?

kunaltyagi

Is there any intent to fix or mitigate this issue? Is it considered an issue at all?

Yes, this is an issue, which will get easier to debug after clang-9 is released. Feel free to address this since we have limited time and manpower.

Anyone knows what is going on?

That'd my fault. I added a cache to speed up compilations which succeeded, but now we are exceeding the memory limits. I'm hesitant to fiddle with cache size. Maybe reducing cache size to 7 GB or even smaller might work, but it might affect compilation times (which fell 45%). Meanwhile, @taketwo has created an issue with Azure to possibly give extra disk space for open source projects since it affects WIndows (without cache) too. See #3387 for details

For c++17 in converts to [[nodiscard]] othwerwise, just empty macro

Quick refactor to mask_map to make it more into C++14 - "default" to mark default constructor and destructor - default values for member variables instead of default constructor - avoid output parameters passed by reference, RVO will make the job for us

taketwo

I rebased on current master and added a commit that removes usage of deprecated signature within PCL.

kunaltyagi · 2019-10-14T22:09:36Z

@lightyear15 I've created #3414 so you can find the offending compilation units. There are some low lying fruits like per-initializing some templates which might interest you

kunaltyagi requested changes Oct 7, 2019

View reviewed changes

recognition/src/mask_map.cpp Outdated Show resolved Hide resolved

recognition/src/mask_map.cpp Outdated Show resolved Hide resolved

recognition/src/mask_map.cpp Outdated Show resolved Hide resolved

recognition/src/mask_map.cpp Outdated Show resolved Hide resolved

taketwo approved these changes Oct 7, 2019

View reviewed changes

taketwo requested changes Oct 7, 2019

View reviewed changes

kunaltyagi reviewed Oct 8, 2019

View reviewed changes

recognition/include/pcl/recognition/mask_map.h Outdated Show resolved Hide resolved

recognition/include/pcl/recognition/mask_map.h Outdated Show resolved Hide resolved

recognition/src/mask_map.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Oct 10, 2019

View reviewed changes

common/include/pcl/pcl_macros.h Outdated Show resolved Hide resolved

recognition/include/pcl/recognition/mask_map.h Outdated Show resolved Hide resolved

recognition/include/pcl/recognition/mask_map.h Outdated Show resolved Hide resolved

kunaltyagi approved these changes Oct 14, 2019

View reviewed changes

lightyear15 and others added 3 commits October 14, 2019 18:06

adding PCL_NODISCARD attribute

33d6e03

For c++17 in converts to [[nodiscard]] othwerwise, just empty macro

rejuvinating mask_map

8748acc

Quick refactor to mask_map to make it more into C++14 - "default" to mark default constructor and destructor - default values for member variables instead of default constructor - avoid output parameters passed by reference, RVO will make the job for us

Do not use deprecated getDifferenceMask()

8f15736

taketwo approved these changes Oct 14, 2019

View reviewed changes

taketwo added changelog: deprecation Meta-information for changelog generation module: recognition labels Oct 14, 2019

taketwo changed the title ~~rejuvinating mask_map~~ Rejuvinate MaskMap Oct 15, 2019

taketwo merged commit 86d6894 into PointCloudLibrary:master Oct 15, 2019

lightyear15 deleted the mask_map branch October 15, 2019 08:36

taketwo changed the title ~~Rejuvinate MaskMap~~ Refactor MaskMap and deprecate several of its methods Jan 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `MaskMap` and deprecate several of its methods #3399

Refactor `MaskMap` and deprecate several of its methods #3399

lightyear15 commented Oct 4, 2019 •

edited by taketwo

Loading

lightyear15 commented Oct 6, 2019

kunaltyagi left a comment

taketwo left a comment

taketwo Oct 7, 2019

lightyear15 Oct 7, 2019

taketwo left a comment

kunaltyagi left a comment

kunaltyagi left a comment

kunaltyagi commented Oct 11, 2019

lightyear15 commented Oct 14, 2019

kunaltyagi left a comment •

edited

Loading

taketwo left a comment

kunaltyagi commented Oct 14, 2019

	[[deprecated("output parameter is not desirable,\
	[[deprecated("use getDifferenceMask() with return value instead")]]

Refactor MaskMap and deprecate several of its methods #3399

Refactor MaskMap and deprecate several of its methods #3399

Conversation

lightyear15 commented Oct 4, 2019 • edited by taketwo Loading

lightyear15 commented Oct 6, 2019

kunaltyagi left a comment

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

taketwo Oct 7, 2019

Choose a reason for hiding this comment

lightyear15 Oct 7, 2019

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi commented Oct 11, 2019

lightyear15 commented Oct 14, 2019

kunaltyagi left a comment • edited Loading

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

kunaltyagi commented Oct 14, 2019

Refactor `MaskMap` and deprecate several of its methods #3399

Refactor `MaskMap` and deprecate several of its methods #3399

lightyear15 commented Oct 4, 2019 •

edited by taketwo

Loading

kunaltyagi left a comment •

edited

Loading