Octree Precompile #1639

stefanbuettner · 2016-06-21T09:24:49Z

Hey there,

based on the mailing list issue Defining Octree Containers it seemed that the PCL_NO_PRECOMPILE mechanism was not fully implemented in the octree module.

This PR should fix that and also tries to minimize the included headers by removing unnecessary ones and removing #include <pcl/octree/octree.h> throughout the library.

For a non-working example prior to this PR see https://gist.github.com/stefanbuettner/0dcde8c6593dbb667ef74c60662cfafb.

Cheers,
Stefan

taketwo · 2016-06-21T09:41:42Z

octree/include/pcl/octree/octree_pointcloud_adjacency.h

@@ -247,9 +247,8 @@ namespace pcl

 }

-//#ifdef PCL_NO_PRECOMPILE
+// Note: Don't precompile this octree type to speed up compilation. It's probably rarely used.


This octree type drives the Supervoxel Clustering algorithm, which is extremely popular according to my feelings.

So it should be added to the precompilation unit?

It's being instantiated here. Not really where I would look for it normally. I would say for now leave it as it was, unless there's a solid reason to not precompile it.

I think the comment should rather says something like:

Do not precompile this octree type because it is typically used with custom leaf containers.

taketwo · 2016-06-21T09:46:45Z

LGTM

SergioRAgostinho · 2016-06-21T12:54:09Z

octree/include/pcl/octree/octree_container.h

@@ -39,7 +39,7 @@
 #ifndef PCL_OCTREE_CONTAINER_H
 #define PCL_OCTREE_CONTAINER_H

-#include <string.h>
+//#include <string.h>


Oh, right. Thx

SergioRAgostinho · 2016-08-20T04:11:45Z

So, the final PR that I need to review. This is personally the most challenging PR to understand out of all selected for 1.8.1. Please bear with me if some of the comments/questions seem noobish.

SergioRAgostinho · 2016-08-20T04:13:19Z

octree/include/pcl/octree/impl/octree2buf_base.hpp

@@ -39,6 +39,8 @@
 #ifndef PCL_OCTREE_2BUF_BASE_HPP
 #define PCL_OCTREE_2BUF_BASE_HPP

+#include <pcl/octree/octree2buf_base.h>


Is this really necessary? I normally only see the corresponding *.hpp being included at the end of the *.h file and the *.hpp never includes the corresponding *.h .

Yeah, you might be right. Still compiles without including anything here.
I'll remove it.

So, remove this?

Sorry, I've just realized that you remove it in a later commit.

SergioRAgostinho · 2016-08-26T19:14:08Z

Ping @stefanbuettner. I was wanting to merge in the short future to give enough time to "explode".

stefanbuettner · 2016-09-09T11:53:36Z

Hey Sergio,
sorry for the late reply. Hadn't much time lately. Hope I can have a look at it again over the weekend.

SergioRAgostinho · 2017-06-27T07:59:08Z

Hey @stefanbuettner. Do you mind rebasing this to the current HEAD? I'm a little out of context since the last time I checked this thoroughly but this should be good to merge.

Edit: Also if possible squash this into three commits:

One fixing the error linking (1)
Another aggregating all tweaks to include files and comments (2, 3, 4, 6, 7)
Setting the PCL_NO_PRECOMPILE definition in modules which need it (5)

stefanbuettner · 2017-06-27T09:16:55Z

Done.

The octree module didn't respect the PCL_NO_PRECOMPILE mechanism. This commit fixes that by implementing it for percompiled classes and fixing includes for non-precompiled classes.

* Remove unnecessary includes and fix compilation error in OctreePointcloudAdjacency. * Add OctreePointCloudAdjacency to octree.h and add comment for instantiation. * Replace general octree.h includes by specific ones. Only include what is necessary to minimize dependencies and reduce build times. * Remove leftover string comment

…ners. Using PCL_NO_PRECOMPILE in cpp files makes sense, since code has to be generated anyway. The scope of the preprocessor definition is the cpp file itself, so it doesn't pollute something. And since all classes are instantiated with other template parameter values, there is no duplicate code.

stefanbuettner · 2017-06-27T13:17:31Z

Just changed the comment you suggested, @taketwo.

taketwo · 2017-06-27T13:25:35Z

Thanks. The pipeline succeeded last time, so I cancelled this new build after changing comment.

SergioRAgostinho · 2017-06-28T09:48:53Z

segmentation/src/supervoxel_clustering.cpp

-template class pcl::SupervoxelClustering<pcl::PointXYZRGBA>;
+template class PCL_EXPORTS pcl::SupervoxelClustering<pcl::PointXYZ>;
+template class PCL_EXPORTS pcl::SupervoxelClustering<pcl::PointXYZRGB>;
+template class PCL_EXPORTS pcl::SupervoxelClustering<pcl::PointXYZRGBA>;


@stefanbuettner Related #1909, why do we need to export these instantiation?

Just gave it a try and I can compile this without PCL_EXPORTS on linux, with both PCL_NO_PRECOMPILE=ON and OFF.

AFAIR PCL_EXPORTS is mostly relevant for Windows. Every time you forget to add it to class declaration Windows users come and report unresolved symbols :D. But in this case PCL_EXPORTS is already present in the declaration of SupervoxelClustering, so indeed I don't see why we need it here.

Maybe we should consider applying the same mechanism to *nix. See https://gcc.gnu.org/wiki/Visibility

At least we would be able to identify problems in advance in linux.

Maybe... do you want this to become a 236th issue in this repository? 😢

If I add it now it would only be 234 :) Nevertheless I think we should leave the idea there, so that "future people" remember it.

I thought this should solve some unresolved symbols but from what I read while catching up on this issue, __declspec(dllexport) is only needed for template function specializations and not for template class specializations. So the PCL_EXPORTS probably shouldn't be there.

taketwo reviewed Jun 21, 2016
View reviewed changes

SergioRAgostinho reviewed Jun 21, 2016
View reviewed changes

stefanbuettner force-pushed the octreePrecompile branch from 32e6502 to 0959f5a Compare June 21, 2016 13:43

SergioRAgostinho added this to the pcl-1.8.1 milestone Jul 14, 2016

SergioRAgostinho added the needs: testing Specify why not closed/merged yet label Jul 14, 2016

SergioRAgostinho reviewed Aug 20, 2016
View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: testing Specify why not closed/merged yet labels Aug 20, 2016

stefanbuettner force-pushed the octreePrecompile branch from 0959f5a to c9b1816 Compare October 19, 2016 20:24

stefanbuettner force-pushed the octreePrecompile branch from 2ae6e51 to 0e980d4 Compare June 27, 2017 09:16

taketwo removed the needs: author reply Specify why not closed/merged yet label Jun 27, 2017

taketwo approved these changes Jun 27, 2017

View reviewed changes

stefanbuettner added 3 commits June 27, 2017 15:15

Fix linking error when using Octree with custom leaf container

85d93ab

The octree module didn't respect the PCL_NO_PRECOMPILE mechanism. This commit fixes that by implementing it for percompiled classes and fixing includes for non-precompiled classes.

stefanbuettner force-pushed the octreePrecompile branch from 0e980d4 to 633343b Compare June 27, 2017 13:15

SergioRAgostinho merged commit ca664ce into PointCloudLibrary:master Jun 27, 2017

SergioRAgostinho reviewed Jun 28, 2017

View reviewed changes

This was referenced Jun 29, 2017

Revise symbol visibility in gcc #1913

Open

pcl_segmentation occur compile error on MSVC2010 #1909

Closed

eisoku9618 mentioned this pull request Jul 15, 2017

KinFu Large Scale build fails #1927

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Octree Precompile #1639

Octree Precompile #1639

stefanbuettner commented Jun 21, 2016

taketwo Jun 21, 2016

stefanbuettner Sep 21, 2016

SergioRAgostinho Sep 26, 2016

taketwo Jun 27, 2017

taketwo commented Jun 21, 2016

SergioRAgostinho Jun 21, 2016

stefanbuettner Jun 21, 2016

SergioRAgostinho commented Aug 20, 2016

SergioRAgostinho Aug 20, 2016

stefanbuettner Sep 21, 2016

taketwo Jun 27, 2017

taketwo Jun 27, 2017

SergioRAgostinho commented Aug 26, 2016

stefanbuettner commented Sep 9, 2016

SergioRAgostinho commented Jun 27, 2017 •

edited

Loading

stefanbuettner commented Jun 27, 2017

stefanbuettner commented Jun 27, 2017

taketwo commented Jun 27, 2017

SergioRAgostinho Jun 28, 2017

SergioRAgostinho Jun 28, 2017

taketwo Jun 28, 2017

SergioRAgostinho Jun 28, 2017

taketwo Jun 28, 2017

SergioRAgostinho Jun 29, 2017

stefanbuettner Jun 29, 2017 •

edited

Loading

Octree Precompile #1639

Octree Precompile #1639

Conversation

stefanbuettner commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taketwo commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Aug 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Aug 26, 2016

stefanbuettner commented Sep 9, 2016

SergioRAgostinho commented Jun 27, 2017 • edited Loading

stefanbuettner commented Jun 27, 2017

stefanbuettner commented Jun 27, 2017

taketwo commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanbuettner Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho commented Jun 27, 2017 •

edited

Loading

stefanbuettner Jun 29, 2017 •

edited

Loading