Allow file_io to read large point clouds depending on PCL config #4331
Conversation
Changed unsigned int point_index to std::size_t point_index to enable pcl::PCLPointCloud2 types with more than 2^28 points to be read and saved in ASCII format. Issues still persist with reading and saving in binary format and conversion to pcl::PointCloud<T> types. Related to issue #4326
kunaltyagi
changed the title
kunaltyagi
added
changelog: enhancement
|
This is a complicated one, because we have not extended and tested pcl::index support in |
| @@ -234,7 +234,7 @@ namespace pcl | |||
| template <typename Type> inline | |||
| std::enable_if_t<std::is_floating_point<Type>::value> | |||
| copyValueString (const pcl::PCLPointCloud2 &cloud, | |||
| const unsigned int point_index, | |||
| const pcl::index_t point_index, | |||
There was a problem hiding this comment.
I assume implicit conversion will happen here with no issues API wise. There will be ABI breakage. Since there's no ABI guarantees for between minor versions I would say let's go with it and not care about deprecation.
There was a problem hiding this comment.
Replacing unsigned int with uindex_t will make sure no ABI issues, but it's technically index_t. I'll mark it as ABI break here.
There was a problem hiding this comment.
Why was uindex_t actually introduced? Indices should always(?) be nonnegative anyway. When should index_t be used and when uindex_t? I did not find any information on this in the documentation.
There was a problem hiding this comment.
it's an alias to an unsigned representation of whatever index_t is, even if index_t is unsigned. It's a helper type for assisting in the widespread migration to index_t.
Always use index_t. Use uindex_t only you need a guaranteed unsigned type with the same bit length of index_t.
There was a problem hiding this comment.
Indices should always(?) be nonnegative anyway
Yes, but 2 counterpoints:
- the behaviour till 1.11 was to use
intmuch more thanuintfor indices - openmp pragma needs the index to be signed, which means this should be an opt-in for the extreme case scenario where casting to signed will overflow
Maybe a PR with these 2 reasons in documentation would be nice 😆
Why was uindex_t actually introduced?
Some places, the value has to be unsigned, such as size, but they are still used for accessing index. In those places, we need to signal to the user via the API that this is 100% not negative (not that indices can be negative), and any negative value will blast (such as conversion warning due to lower API).
We are actually thinking of introducing sindex_t to compliment uindex_t for the explicit reason and make index_t explicitly ambiguous in it's signed needs for places which are ok with both signed and unsigned values.
Sorry. I'm a bit busy reviewing loads of code now and was tardy with my code before.
|
I'm ok for merge if CI goes green. |
kunaltyagi
added
the
changelog: ABI break
kunaltyagi
added
kind: todo
kunaltyagi
changed the title
…point_field_types * origin/new_point_field_types: (211 commits) [CI] Make windows build on c:\ drive to fix out-of-disk-space errors (PointCloudLibrary#4382) Replace '.points.' with just '.' (PointCloudLibrary#4217) [Surface/Poisson4] Always update counter and prevent overflow access in poisson4 octree (PointCloudLibrary#4316) [surface/3rdParty] Add stdlib header for malloc in poisson (bugfix for gcc-5) (PointCloudLibrary#4376) [Misc] Deprecate unused ease-of-internal-use headers (PointCloudLibrary#4367) Use nth_element in median filter nth_element is faster than partial_sort, and should be used for median finding enable CUDA on 16.04 CI Optimize includes As recommended by include-what-you-use Update gpu_install.rst Fix getColor() Variable needs to be expanded. Optimize includes As recommended by include-what-you-use [gpu] Add square distances to ApproxNearestSearch (PointCloudLibrary#4340) [common] Allow conversion of PointCloud with more than 32-bit size rows/columns (PointCloudLibrary#4343) Change the macro to detect and remove deprecations before release update docstring clang format approx_nsearch.cu another indentation fix fix indentation and getBitsNum function Allow file_io to read large point clouds depending on PCL config (PointCloudLibrary#4331) ...
Changed unsigned int point_index to std::size_t point_index to enable pcl::PCLPointCloud2 types with more than 2^28 points to be read and saved in ASCII format. Issues still persist with reading and saving in binary format and conversion to pcl::PointCloud types. Related to issue #4326