Change `points.size()` to `size()` to make transition to `index_t` easier #4190

kunaltyagi · 2020-06-15T14:35:10Z

No description provided.

kunaltyagi · 2020-06-16T06:37:49Z

Needs testing for GPU and CUDA modules

SergioRAgostinho

Partial (33/357)

...ramework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/colorshot_local_estimator.h

...rec_framework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/fpfh_local_estimator.h

...framework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/fpfh_local_estimator_omp.h

apps/3d_rec_framework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/local_estimator.h

...rec_framework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/shot_local_estimator.h

common/include/pcl/common/impl/common.hpp

common/include/pcl/common/impl/io.hpp

kunaltyagi · 2020-06-23T16:21:37Z

Issues remaining to discuss (repeating because of GitHub's awesome interface):

Format strings:
- We might need explicit casts to comply with the format strings
- index_t might change, so... what to do about them?
removing redundant casts
sprinkling auto

Issues split off from this PR: #4212, #4213, #4214

PR split off from here: #4216

SergioRAgostinho · 2020-06-24T07:38:25Z

Format strings:

We might need explicit casts to comply with the format strings

index_t might change, so... what to do about them?

I'm not very experienced with string manipulation in C++. This is what I considered

We explicitly cast to uint64_t (since we're guaranteed that the number represented will always be positive) and we'll always be able to print the corresponding number. Simple. I'm just unsure if there are meaningful performance penalties.
We ignore formatting entirely and use a string stream to build the string. I suspect this is slow.
We use boost format and transition to std::format in C++20.

SergioRAgostinho

Partial (119/357). We should address all for-range conversions here. It will suppress three problems in one a single place:

no direct use of points.size() or even size() for that matter
no need to know the return type of size()
no issues with potential range limit of the index type used.

doc/tutorials/content/sources/don_segmentation/don_segmentation.cpp

doc/tutorials/content/sources/extract_indices/extract_indices.cpp

doc/tutorials/content/sources/cluster_extraction/cluster_extraction.cpp

doc/tutorials/content/sources/model_outlier_removal/model_outlier_removal.cpp

features/include/pcl/features/impl/esf.hpp

features/include/pcl/features/impl/feature.hpp

kunaltyagi · 2020-06-24T13:21:54Z

125 instances of cast found and replaced using

grep 'width\s*=.*(.*size\s*()\s*)' * -nrl | xargs -n1 sed -i 's,\(width\s*=\s*\).*(\(.*size\s*()\)\s*),\1\2,g'

SergioRAgostinho · 2020-06-25T08:25:32Z

Answering #4190 (comment)

Isn't floor(x) same as ceil(x) - 1, regardless of sign of x?

No. The corner case is when x is a natural number.

x = 2
floor(x) -> 2
ceil(x) - 1 -> 1

SergioRAgostinho

Partial (128/371). Be very conservative with your force-pushes. This last one you did offset my review progress by 50 files.

...ramework/include/pcl/apps/3d_rec_framework/feature_wrapper/local/colorshot_local_estimator.h

apps/src/pcd_organized_multi_plane_segmentation.cpp

cuda/io/src/extract_indices.cu

features/include/pcl/features/impl/moment_of_inertia_estimation.hpp

features/include/pcl/features/impl/our_cvfh.hpp

SergioRAgostinho

Partial (213/371)

features/include/pcl/features/impl/ppfrgb.hpp

features/src/narf.cpp

filters/include/pcl/filters/impl/approximate_voxel_grid.hpp

filters/include/pcl/filters/impl/filter.hpp

io/include/pcl/io/impl/point_cloud_image_extractors.hpp

kdtree/include/pcl/kdtree/impl/kdtree_flann.hpp

keypoints/include/pcl/keypoints/impl/smoothed_surfaces_keypoint.hpp

octree/include/pcl/octree/octree_pointcloud_occupancy.h

outofcore/include/pcl/outofcore/impl/octree_disk_container.hpp

recognition/include/pcl/recognition/impl/hv/hv_go.hpp

tracking/include/pcl/tracking/impl/particle_filter.hpp

visualization/include/pcl/visualization/impl/image_viewer.hpp

apps/src/pcd_organized_multi_plane_segmentation.cpp

filters/include/pcl/filters/impl/sampling_surface_normal.hpp

filters/include/pcl/filters/impl/shadowpoints.hpp

kunaltyagi · 2020-07-03T21:23:43Z

So much more work left.... sigh

Regarding casts, I don't see a point in replacing the casts now and adding them later when the loop index variable is changed to index_t

kunaltyagi · 2020-07-13T00:44:07Z

Addressing conflicts too

SergioRAgostinho

If you addressed all the items, this is good to go from my side.

kunaltyagi · 2020-07-13T15:44:42Z

I did. Please approve and merge 😄

…ral fixes: * Fix warning: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Wimplicit-exception-spec-mismatch] RealSense2Grabber::~RealSense2Grabber () * Fix error: use of undeclared identifier 'size' (introduced in PointCloudLibrary#4190) * Fix error: variable 'cloud_texture_ptr' must have explicitly specified data sharing attributes

…ral fixes: * Fix warning: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Wimplicit-exception-spec-mismatch] RealSense2Grabber::~RealSense2Grabber () * Fix error: use of undeclared identifier 'size' (issue introduced in PointCloudLibrary#4190) * Fix error: variable 'cloud_texture_ptr' must have explicitly specified data sharing attributes

…ral fixes: * Fix warning: function previously declared with an explicit exception specification redeclared with an implicit exception specification [-Wimplicit-exception-spec-mismatch] RealSense2Grabber::~RealSense2Grabber () * Fix warning: comparison of integers of different signs: 'int' and 'std::size_t' * Fix error: use of undeclared identifier 'size' (issue introduced in PointCloudLibrary#4190) * Fix error: variable 'cloud_texture_ptr' must have explicitly specified data sharing attributes

kunaltyagi added needs: code review Specify why not closed/merged yet priority: gsoc Reason for prioritization labels Jun 15, 2020

kunaltyagi marked this pull request as ready for review June 16, 2020 06:51

SergioRAgostinho reviewed Jun 23, 2020

View reviewed changes

SergioRAgostinho self-requested a review June 23, 2020 12:00

kunaltyagi mentioned this pull request Jun 23, 2020

Access to points should be suppressed in PCL code #4213

Open

SergioRAgostinho reviewed Jun 24, 2020

View reviewed changes

kunaltyagi force-pushed the points.size branch from d0f2036 to 865223f Compare June 24, 2020 13:30

kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Jun 24, 2020

kunaltyagi force-pushed the points.size branch 6 times, most recently from e36e70b to 4b3a9ed Compare June 25, 2020 00:43

kunaltyagi added the needs: feedback Specify why not closed/merged yet label Jun 25, 2020

kunaltyagi force-pushed the points.size branch from 299aeb9 to eaa5230 Compare June 25, 2020 05:47

SergioRAgostinho reviewed Jun 25, 2020

View reviewed changes

SergioRAgostinho reviewed Jun 26, 2020

View reviewed changes

SergioRAgostinho reviewed Jun 29, 2020

View reviewed changes

apps/src/pcd_organized_multi_plane_segmentation.cpp Show resolved Hide resolved

filters/include/pcl/filters/impl/sampling_surface_normal.hpp Outdated Show resolved Hide resolved

filters/include/pcl/filters/impl/shadowpoints.hpp Outdated Show resolved Hide resolved

kunaltyagi removed the needs: feedback Specify why not closed/merged yet label Jul 3, 2020

kunaltyagi mentioned this pull request Jul 3, 2020

[filters] Use Eigen instead of handwritten logic in SamplingSurfaceNormal::findXYZMaxMin #4243

Merged

kunaltyagi added 17 commits July 13, 2020 09:43

Format string changes (2/N)

a5294a7

Range based for loop (4/N)

8a1e09f

Range for loop and Eigen usage

b8d49b3

GPU for loop and auto

850c6b8

GPU and kinfu

36f127f

OpenMP doesn't like range for loop

e244344

Format string changes (3/N)

35c234a

I definitely didn't want to do this

151497e

Auto (3/N)

797f4b3

Range based for loop (6/N)

ce2f33d

Cast changes (5/N)

5a41840

Fixing silly error

843f897

Reverting change to index_t for loop variable. Too soon

7a89298

Review changes

5554206

Fixing errors and warnings

02a6fe8

Fixing errors and warnings (2/N)

e18345a

Addressing review

0fc39b6

kunaltyagi force-pushed the points.size branch from cbdcb36 to 0fc39b6 Compare July 13, 2020 00:43

SergioRAgostinho reviewed Jul 13, 2020

View reviewed changes

SergioRAgostinho approved these changes Jul 13, 2020

View reviewed changes

SergioRAgostinho merged commit 009d73d into PointCloudLibrary:master Jul 13, 2020

kunaltyagi deleted the points.size branch July 13, 2020 16:27

SunBlack mentioned this pull request Oct 27, 2020

Enable Real Sense 2 grabber for all platforms #4471

Merged

Uh oh!

Change points.size() to size() to make transition to index_t easier #4190

Change points.size() to size() to make transition to index_t easier #4190

Uh oh!

Conversation

kunaltyagi commented Jun 15, 2020

Uh oh!

kunaltyagi commented Jun 16, 2020

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunaltyagi commented Jun 23, 2020

Uh oh!

SergioRAgostinho commented Jun 24, 2020

Uh oh!

SergioRAgostinho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunaltyagi commented Jun 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergioRAgostinho commented Jun 25, 2020

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SergioRAgostinho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Change `points.size()` to `size()` to make transition to `index_t` easier #4190

Change `points.size()` to `size()` to make transition to `index_t` easier #4190

SergioRAgostinho left a comment •

edited

Loading

kunaltyagi commented Jun 24, 2020 •

edited

Loading