CMake integration for PCL_INDEX_SIZE and PCL_INDEX_SIGNED

[kunaltyagi]

This will allow downstream users to configure the index size to their preferred size and save memory on projects with smaller indices and enable use of PCL for really large point clouds.

• CMake integration for PCL_INDEX_SIZE and PCL_INDEX_SIGNED. Might make sense to have a separate PR for this and keep this strictly related to new type

Agreed

Originally posted by @SergioRAgostinho in #3651 (comment)

[stale]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[mvieth]

I think this was added in 0dfb3fc

Although I am not sure about setting the index size to 32 bit fixed for PCL 1.12.0 and later. Would that not technically break everything on systems where sizeof(int) != 4 (if such a system exists)? What is the reason behind this?

[kunaltyagi]

(if such a system exists)

I think Windows would be that platform

What is the reason behind this?
The data type are not the same between platforms, so files written by one are not portable (from windows to Linux).

Actual benefits:

• Internal API uses constant sizes, opening up a few type trait tricks for serialization, memory mapping io files, etc.(faster IO)
• Guarantee that the files written on windows can be read on linux and vice-versa if the same compile flags are chosen (platform agnostic artifacts from PCL based tools)
• smaller memory consumption due to 50% smaller index vectors, and allow us to filter issues with big clouds (since they'd be built with custom flags)
• Focus on better support for large clouds in modules that assumed 32 bit index (octree and kdtree). The situation is better now, but not tested at all.

Debatable side-effects: Adding this would being Windows to parity with Linux (default: 32 bit index)
Cons: Some people on windows might observe regression, but they would have to be in HPC domain (>2GB RAM for indices alone needed for needing a >32 bit index)

[mvieth]

I think Windows would be that platform

Really? This table (and everywhere else I looked) says that most systems including Windows use 32 bit ints.
I generally agree with your assessment. But if there is a system where sizeof(int) != 4 (so int != pcl::index_t), since most user programs probably still use std::vector<int> instead of pcl::Indices for indices vectors, they would not be compatible and that would be a rather big break.

[kunaltyagi]

Oh, I was mistaken. Windows has long int as 32 bit same as int. That was the difference in behavior I was thinking of.

they would not be compatible and that would be a rather big break.

If the users are on SILP64 or ILP64, we can't really support them. (We don't even have CI for them. We extensively use int32_t explicitly in our codebase, so they must be facing a huge issue anyways) Since it'd be a major release, I do hope the users and platform distribution maintainers would note it from the release note highlights.

We're including a CMake flag to set the default size, and this has been in the code for quite a long time now. They (SILP64, ILP64) are also HPC environments, so definitely a pro user-segment. Most of HPC environments have a dedicated sys-admin managing the system configuration (at least till Singularity and Sarus catch on everywhere). It's not a perfect solution, but it's better than continuing with the current non-standard size system.

PS: We declared this in our previous release notes (1.11 and 1.11.1) as well. I think the original discussion started in 2019 end, and was finalized in 2020, a while before the 1.11 release. I might be wrong about the timelines though. Ofc, someone not following the discussions/release notes would be caught by surprise. Not debating on this at all.

[mvieth]

👍 Alright, that is convincing. Then I am okay with setting PCL_INDEX_SIZE to fixed 32 bit. Feel free to close this issue if you also think no further work on this is needed.

[kunaltyagi]

Thanks. I didn't actually know the impact of this change in detail before you restarted the conversation :)