Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CMake] Add option to choose pcl::index_t while compiling #4166

Merged
merged 18 commits into from
Jun 10, 2020

Conversation

haritha-j
Copy link
Contributor

Add cmake configuration to change indices type and sign.

@SergioRAgostinho SergioRAgostinho added module: cmake priority: gsoc Reason for prioritization labels Jun 4, 2020
pcl_config.h.in Outdated

#cmakedefine HAVE_TBB 1

#cmakedefine HAVE_OPENNI 1

#cmakedefine HAVE_OPENNI2 1


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded?

@@ -64,3 +64,9 @@ set_property(GLOBAL PROPERTY USE_FOLDERS ON)
option(BUILD_tools "Useful PCL-based command line tools" ON)

option(WITH_DOCS "Build doxygen documentation" OFF)

# set index size
set(PCL_SIZE_INDEX 32 CACHE STRING "Set index size. Available options are: 8 16 32 64")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the note in types.h regarding the default size. It can't be 32 for PCL 1.11 due to compatibility. It can be 32 for PCL 1.12 since the user can then override the size at compile time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would modifying types.h to be

#if PCL_MINOR_VERSION <= 11
// sizeof returns bytes, while we measure size by bits in the template
#undef PCL_INDEX_SIZE
#define PCL_INDEX_SIZE (sizeof(int) * 8)
#else
#ifndef PCL_INDEX_SIZE
#define PCL_INDEX_SIZE 32
#endif  // PCL_INDEX_SIZE
#endif  // PCL_MINOR_VERSION

be an option or should the changes be made solely in the cmake configuration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can modify anything as long as it keeps the behavior (and is sane) 😄

@kunaltyagi kunaltyagi added the needs: more work Specify why not closed/merged yet label Jun 4, 2020
@larshg
Copy link
Contributor

larshg commented Jun 4, 2020

Maybe squash the commits to a single commit :) ?

cmake/pcl_options.cmake Outdated Show resolved Hide resolved
cmake/pcl_options.cmake Outdated Show resolved Hide resolved
@haritha-j haritha-j changed the title Indices Integrate index type into cMake Jun 5, 2020
@haritha-j
Copy link
Contributor Author

@kunaltyagi I moved the version check for changing indices type to the config file, so that all of the configuration happens in one place.

cmake/pcl_options.cmake Outdated Show resolved Hide resolved
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except inline

pcl_config.h.in Outdated Show resolved Hide resolved
// sizeof returns bytes, while we measure size by bits in the template
#define PCL_INDEX_SIZE (sizeof(int) * 8)
#if (PCL_INDEX_SIZE_TEMP > 0)
#define PCL_INDEX_SIZE ${PCL_INDEX_SIZE}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using PCL_INDEX_SIZE_TEMP might be better instead of ${PCL_INDEX_SIZE}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like I can't follow both of these suggestions together. If I undefine PCL_INDEX_SIZE_TEMP, then at compile time PCL_INDEX_SIZE cannot be set to it, because it is no longer available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah. Can we just remove the temporary variable in that case? It's sole use seems to be comparison with 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unresolving the conversation, because PCL_INDEX_SIZE_TEMP still needs to be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I thought by remove @kunaltyagi meant I should undef PCL_INDEX_SIZE_TEMP, not get rid of it altogether.
Is this what you had in mind?

#if (${PCL_INDEX_SIZE} > 0)
  #define PCL_INDEX_SIZE ${PCL_INDEX_SIZE}
#else
  #if PCL_MINOR_VERSION <= 11
    // sizeof returns bytes, while we measure size by bits in the template
    #define PCL_INDEX_SIZE (sizeof(int) * 8)
  #else
    #define PCL_INDEX_SIZE 32
  #endif //PCL_MINOR_VERSION
#endif 

Copy link
Member

@SergioRAgostinho SergioRAgostinho Jun 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I interpreted, because there's no need to actually define anything.

@kunaltyagi kunaltyagi removed the needs: more work Specify why not closed/merged yet label Jun 7, 2020
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kunaltyagi kunaltyagi added the changelog: enhancement Meta-information for changelog generation label Jun 10, 2020
@kunaltyagi kunaltyagi changed the title Integrate index type into cMake [CMake] Add option to choose pcl::index_t while compiling Jun 10, 2020
@SergioRAgostinho SergioRAgostinho merged commit 0dfb3fc into PointCloudLibrary:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: enhancement Meta-information for changelog generation module: cmake priority: gsoc Reason for prioritization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants