Provide proper EuclideanClusterComparator method depreciation. New Pragma macro. New Deprecated type. #2096

SergioRAgostinho · 2017-11-21T19:56:31Z

Here's something for us to discuss: in order allow us saving time cleaning things in the future, I created an intermediate class (which can't be directly instantiated), and implements what's gonna be the final functionality of the ECC. It will just suffer a rename later on.

I replaced the map for a set as pointed out before.

Closes #2091

- Restored the previous ECC API - Defined a "protected" base class which will replace the original in a future release.

taketwo · 2017-11-23T13:32:15Z

I'm not sold by the EuclideanClusterComparator2 class, I think it introduces confusion for those who are not aware of all this deprecation business. It will show up generated documentation (but without any apparent explanation what it is). Some people may try to use it and next PCL release will break their code. To simplify cleaning this up in the future, maybe just group changes that "bring back" old interface into a single commit that can be easily reverted afterwards?

Other random comments:

Now that we use pragma messages to warn about deprecation, maybe we should disable -W#pragma-messages when building PCL? (But not user code using PCL, of course)
What does "changes: behavior" label mean?

SergioRAgostinho · 2017-11-23T15:15:03Z

To simplify cleaning this up in the future, maybe just group changes that "bring back" old interface into a single commit that can be easily reverted afterwards?

👍

Now that we use pragma messages to warn about deprecation, maybe we should disable -W#pragma-messages when building PCL?

Fair enough. Although I just did it like that because I can't seem find a way to deprecate a template parameter, because the entire class is not gonna be deprecated. Normally, upon deprecation there's a transient period in which the old definition and the new one coexist and you tell people to migrate. In our case we still want to preserve the class name and that is messing up the process.

What does "changes: behavior" label mean?

Something which didn't break the ABI/API but in which the class behavior/output changed compared to before. Originally it was to signal that we are no longer using normals in the clustering process. In this particular PR it was misused, because I'm only adding the old interface.

taketwo · 2017-11-23T15:21:28Z

Something which didn't break the ABI/API but in which the class behavior/output changed compared to before.

But even the original PR did not change behavior, because normals and angular threshold were not used at all? Or did I misunderstand something?

SergioRAgostinho · 2017-11-23T15:59:01Z

But even the original PR did not change behavior, because normals and angular threshold were not used at all? Or did I misunderstand something?

That's right. The class had normals as class members but did nothing with them.

taketwo · 2017-11-23T16:01:20Z

By the way, I think I have figured out a way to deprecate template parameter. Will post a proof-of-concept soon.

SergioRAgostinho · 2017-11-23T16:03:05Z

Awesome! Should we delete the macros I added or shall we leave them?

taketwo · 2017-11-23T16:03:55Z

Wait, maybe you won't like my solution :)

taketwo · 2017-11-23T16:10:40Z

Here we go: http://cpp.sh/2pyit

SergioRAgostinho · 2017-11-23T16:20:08Z

Oh that looks cool. I actually thought that would be considered a redefinition of A but not. That way it's still two different classes but now with the "same name" just different amount of templates.

SergioRAgostinho · 2017-11-25T13:57:53Z

I had another look at this and your approach definitely improved mine but the point regarding the documentation is still present.

Won't this also show up on the documentation? Maybe the workaround is to actually prevent this entry to be documented
New deprecated namespace?

With variadic templates we would have cleaner options 😞 http://cpp.sh/9ef2s

taketwo · 2017-11-25T14:16:19Z

AFAIK Doxygen has some markers or macros to exclude certain classes or functions from documentation... And yeah, modern C++ solution looks way better!

SergioRAgostinho · 2017-11-25T16:52:58Z

I adopted the strategy you devised. I added 2 (!) inner namespaces to pcl namespace: experimental, deprecated.

The idea behind experimental is to define be able to define classes or implementations which might or might not make it to next version. I'm copying what I see done in the standard library for experimental features. In our case it works really well, encapsulating what's going to be new EuclideanClusterComparator class signature.
The deprecated namespace was created just to encapsulate that dummy struct which works as a generic type. This way we don't add useless types to the common namespace.

taketwo

I like your solution 👍

One small thing. I'd propose better deprecation warnings, see comments.

taketwo · 2017-11-26T10:38:35Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h


      /** \brief Empty constructor for EuclideanClusterComparator. */
+      PCL_DEPRECATED ("EuclideanClusterComparator will no longer be templated on a PointNormal type in future minor release")


"Remove PointNT from template parameters."

taketwo · 2017-11-26T10:43:31Z

segmentation/include/pcl/segmentation/euclidean_cluster_comparator.h

-        distance_threshold_ = distance_threshold;
-        depth_dependent_ = depth_dependent;
-      }
+      PCL_DEPRECATED ("EuclideanClusterComparator no longer makes use of normals.")


Actually, it never used normals in the first place. I think it can be useful to let the users know this. Otherwise the line of thinking when someone sees this message is: "Oh crap, what should I do now? I still need to compare normals. Is there another class now? Which one, how to find it?"

So perhaps a better deprecation message would be something along these lines: "EuclideadClusterComparator never actually used normals and angular threshold, this function has no effect on the behavior of the comparator. Therefore it is deprecated and will be removed in future releases."

Same for other functions.

SergioRAgostinho · 2017-11-26T12:40:11Z

Will do. There's one thing which worries me though. I'm not seeing deprecation warnings popping up in the build and I'm quite sure I should seem them, because there's code using the three template parameter signature.

taketwo · 2017-11-26T13:14:23Z

because there's code using the three template parameter signature.

Oh right, speaking of which... Since we already support the future two-parameter signature, there is no need to bring back normals to the library sources.

I'm not seeing deprecation warnings popping up in the build and I'm quite sure I should seem them

Hm, indeed. I went through your changes again and they implement the proof-of-concept exactly, don't see any difference.

SergioRAgostinho · 2017-11-26T13:16:15Z

I'm looking into it. I left originally the three template signature exactly to confirm the verbose, with the intention to remove after.

SergioRAgostinho · 2017-11-26T14:06:28Z

They're popping on my system for gcc but not for clang. Since the apps task is clang built we get no warnings.

taketwo · 2017-11-26T14:10:11Z

On my machine clang3.8 reports warnings for my PoC.

SergioRAgostinho · 2017-11-26T14:11:33Z

clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

SergioRAgostinho · 2017-11-26T14:36:53Z

It was only introduced in http://releases.llvm.org/3.9.0/tools/clang/docs/AttributeReference.html#deprecated-gnu-deprecated so everything should be ok.

I'll proceed with the removing the template parameters from our code and it should be good to go.

SergioRAgostinho · 2017-11-26T14:38:44Z

Oh... so you say it should actually work in 3.8 as well... hmm...

taketwo · 2017-11-26T14:45:45Z

Interesting. We have the same environment

$ clang++ --version
clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

and I do get warnings

warning: 'A' is deprecated: Second template parameter is
      deprecated, remove it [-Wdeprecated-declarations]

taketwo · 2017-11-26T14:47:30Z

Also, on Travis we have 3.9.0 which definitely has this attribute according to the link you posted.

SergioRAgostinho · 2017-11-26T15:03:08Z

Some interesting findings:

Your PoC generates warnings on it's own.
If in my cmake project I add find_package(PCL 1.8.1.99 REQUIRED) it no longer prints the warnings. Just to be clear, I'm just finding the package. I'm not even linking to the pcl to the target or anything.

We're messing up some flags for sure. The compiler flags in each case
1.

/usr/bin/clang++      -o CMakeFiles/deprecation.dir/deprecation.cpp.o -c <path_to>/pcl_test_cases/deprecation/deprecation.cpp

/usr/bin/clang++   -Dqh_QHpointer -I/usr/include/vtk-5.10  -Wno-deprecated   -o CMakeFiles/deprecation.dir/deprecation.cpp.o -c <path_to>/pcl_test_cases/deprecation/deprecation.cpp

That -Wno-deprecated looks suspicious

taketwo · 2017-11-26T15:23:06Z

I guess with clang we fall under this branch:

pcl/CMakeLists.txt

Line 122 in cd302dc

SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated")

Not sure how it gets propagated to a project using PCL.

SergioRAgostinho · 2017-11-26T15:44:22Z

It’s not sure it is that. I commented that line before and I’m still getting the flag.

…

On 26 Nov 2017, at 15:23, Sergey Alexandrov ***@***.***> wrote: I guess with clang we fall under this branch: https://github.com/PointCloudLibrary/pcl/blob/cd302dc8b6bc779c3fb279f2b18fe411631dff26/CMakeLists.txt#L122 Not sure how it gets propagated to a project using PCL. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

SergioRAgostinho · 2017-11-26T16:59:00Z

It gets added when we request the visualization component in find package.

SergioRAgostinho · 2017-11-26T17:23:37Z

The issue comes from here

pcl/visualization/CMakeLists.txt

Line 12 in 5d18757

include("${VTK_USE_FILE}")

Which is adding this vtk use file. This is provided from the vtk package and in my system this looks like this vtk-5.10

#
# This module is provided as VTK_USE_FILE by VTKConfig.cmake.  It can
# be INCLUDEd in a project to load the needed compiler and linker
# settings to use VTK.
#

IF(NOT VTK_USE_FILE_INCLUDED)
  SET(VTK_USE_FILE_INCLUDED 1)

  # Update CMAKE_MODULE_PATH so includes work.
  SET (CMAKE_MODULE_PATH
    ${CMAKE_MODULE_PATH}
    ${VTK_CMAKE_DIR})

  # Load the compiler settings used for VTK.
  IF(VTK_BUILD_SETTINGS_FILE AND NOT SKIP_VTK_BUILD_SETTINGS_FILE)
    INCLUDE(${CMAKE_ROOT}/Modules/CMakeImportBuildSettings.cmake)
    CMAKE_IMPORT_BUILD_SETTINGS(${VTK_BUILD_SETTINGS_FILE})
  ENDIF(VTK_BUILD_SETTINGS_FILE AND NOT SKIP_VTK_BUILD_SETTINGS_FILE)

  # Add compiler flags needed to use VTK.
  SET(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${VTK_REQUIRED_C_FLAGS}")
  SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${VTK_REQUIRED_CXX_FLAGS}")
  SET(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} ${VTK_REQUIRED_EXE_LINKER_FLAGS}")
  SET(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} ${VTK_REQUIRED_SHARED_LINKER_FLAGS}")
  SET(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} ${VTK_REQUIRED_MODULE_LINKER_FLAGS}")

  # Add include directories needed to use VTK.
  INCLUDE_DIRECTORIES(${VTK_INCLUDE_DIRS})

  # Import the VTK_LOAD_CMAKE_EXTENSIONS macro.
  INCLUDE(vtkMakeInstantiator)

ENDIF(NOT VTK_USE_FILE_INCLUDED)

The contents of VTK_REQUIRED_CXX_FLAGS include the -Wno-deprecate

-- VTK_USE_FILE: /usr/lib/vtk-5.10/UseVTK.cmake
-- VTK_REQUIRED_CXX_FLAGS:  -Wno-deprecated
-- VTK_REQUIRED_C_FLAGS:

SergioRAgostinho · 2017-11-26T17:43:15Z

What to do in this scenario? This is an upstream compilation option of whom packaged vtk5 for apt.

I was now experimenting with not using that VTKUse file but I'm not fully sure this is the way to address this.

taketwo · 2017-11-26T17:44:12Z

I suspected VTK and even grepped for "no-deprecated" in /usr/lib/cmake/vtk-6.2 but it came back with nothing.

taketwo · 2017-11-26T17:47:36Z

I would conjecture that VTK 6 and above do not set this flag. As for VTK 5... well, bad luck, what can we do.

SergioRAgostinho · 2017-11-26T17:51:30Z

Yet to be explained is why it pops only on clang.

SergioRAgostinho · 2017-11-27T02:20:04Z

Just pushed the final things. I can't perform any more reorganizing of commits easily due to the indentation changes.

taketwo · 2017-11-27T10:25:11Z

Thanks!

SergioRAgostinho added changelog: behavior change Meta-information for changelog generation changelog: ABI break Meta-information for changelog generation labels Nov 21, 2017

SergioRAgostinho added this to the pcl-1.9.0 milestone Nov 21, 2017

SergioRAgostinho added 3 commits November 21, 2017 22:09

Defined new pragma and pragma_warning macros

2682870

Deprecation of some EuclideanClusterComparator methods

06111ca

- Restored the previous ECC API - Defined a "protected" base class which will replace the original in a future release.

Replace exclude_labels map for a set in EuclideanClusterComparator

3f91a88

SergioRAgostinho force-pushed the ecc-deprecated branch from 5db45b0 to 3f91a88 Compare November 21, 2017 22:10

SergioRAgostinho removed the changelog: behavior change Meta-information for changelog generation label Nov 23, 2017

taketwo reviewed Nov 26, 2017

View reviewed changes

New ECC deprecation strategy

c3f4710

SergioRAgostinho force-pushed the ecc-deprecated branch from 3bff000 to c3f4710 Compare November 27, 2017 02:18

taketwo merged commit 47629c1 into PointCloudLibrary:master Nov 27, 2017

SergioRAgostinho deleted the ecc-deprecated branch November 30, 2017 19:18

SergioRAgostinho mentioned this pull request Feb 12, 2018

Add SphericalVoxelGrid Filter #2171

Open

This was referenced Aug 25, 2018

Change Log automation #2385

Closed

Change Log generation tool. Automates change log generation. #2396

Merged

SergioRAgostinho changed the title ~~Provide proper EuclideanClusterComparator method depreciation~~ Provide proper EuclideanClusterComparator method depreciation. New Pragma macro. New Deprecated type. Aug 25, 2018

SergioRAgostinho added module: common module: segmentation labels Aug 25, 2018

SergioRAgostinho added the changelog: deprecation Meta-information for changelog generation label Sep 2, 2018

SergioRAgostinho mentioned this pull request Sep 25, 2018

Removed normal related accessors and types from EuclideanClusterComparator #1542

Merged


		/** \brief Empty constructor for EuclideanClusterComparator. */
		PCL_DEPRECATED ("EuclideanClusterComparator will no longer be templated on a PointNormal type in future minor release")

Provide proper EuclideanClusterComparator method depreciation. New Pragma macro. New Deprecated type. #2096

Provide proper EuclideanClusterComparator method depreciation. New Pragma macro. New Deprecated type. #2096

Conversation

SergioRAgostinho commented Nov 21, 2017

taketwo commented Nov 23, 2017

SergioRAgostinho commented Nov 23, 2017

taketwo commented Nov 23, 2017

SergioRAgostinho commented Nov 23, 2017

taketwo commented Nov 23, 2017

SergioRAgostinho commented Nov 23, 2017

taketwo commented Nov 23, 2017

taketwo commented Nov 23, 2017

SergioRAgostinho commented Nov 23, 2017

SergioRAgostinho commented Nov 25, 2017

taketwo commented Nov 25, 2017 via email • edited Loading

SergioRAgostinho commented Nov 25, 2017

taketwo left a comment

Choose a reason for hiding this comment

taketwo Nov 26, 2017

Choose a reason for hiding this comment

taketwo Nov 26, 2017

Choose a reason for hiding this comment

SergioRAgostinho commented Nov 26, 2017

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

taketwo commented Nov 26, 2017 • edited Loading

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017 • edited Loading

taketwo commented Nov 26, 2017 • edited Loading

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017 • edited Loading

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017 via email

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017 • edited Loading

taketwo commented Nov 26, 2017

taketwo commented Nov 26, 2017

SergioRAgostinho commented Nov 26, 2017

SergioRAgostinho commented Nov 27, 2017

taketwo commented Nov 27, 2017

taketwo commented Nov 25, 2017 via email •

edited

Loading

taketwo commented Nov 26, 2017 •

edited

Loading

SergioRAgostinho commented Nov 26, 2017 •

edited

Loading

taketwo commented Nov 26, 2017 •

edited

Loading

SergioRAgostinho commented Nov 26, 2017 •

edited

Loading

SergioRAgostinho commented Nov 26, 2017 •

edited

Loading