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.

Sign up for GitHub

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

Jump to bottom

Enable arbitary size Indices for Octree module #4350

Merged
merged 12 commits into from
Mar 14, 2021
Merged

Enable arbitary size Indices for Octree module #4350

merged 12 commits into from
Mar 14, 2021

Conversation

kunaltyagi
Copy link
Member

@kunaltyagi kunaltyagi commented Aug 24, 2020 •
edited
Loading

And a bug fix for common module

This brings up the modules ready to:

  • common, octree
  • ml (since it is mostly independent of common)

Next targets: kdtree, sample_consensus, io

@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation needs: code review Specify why not closed/merged yet module: octree labels Aug 24, 2020
@kunaltyagi kunaltyagi marked this pull request as draft August 24, 2020 19:31
@kunaltyagi kunaltyagi marked this pull request as ready for review August 24, 2020 19:55
@kunaltyagi kunaltyagi changed the title Enable arbitary size Indices for Octree module Enable arbitary size Indices for Octree and IO modules Aug 24, 2020
SergioRAgostinho
SergioRAgostinho previously requested changes Aug 25, 2020
View reviewed changes
Copy link
Member

@SergioRAgostinho SergioRAgostinho left a comment

Choose a reason for hiding this comment

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

It doesn't work to simply look at method's interfaces. There's some rotten apples inside these methods which will still break functionality.

pcl/octree/include/pcl/octree/impl/octree_search.hpp

Line 107 in c52674e

unsigned int result_count = static_cast<unsigned int>(point_candidates.size());

I would rather take a more careful approach to this migration.

octree/include/pcl/octree/impl/octree_search.hpp Outdated Show resolved Hide resolved
octree/include/pcl/octree/impl/octree_search.hpp Outdated Show resolved Hide resolved
octree/include/pcl/octree/impl/octree_search.hpp Outdated Show resolved Hide resolved
octree/include/pcl/octree/impl/octree_search.hpp Outdated
int k,
std::vector<int>& k_indices,
std::vector<float>& k_sqr_distances)
const PointT& p_q, int k, Indices& k_indices, std::vector<float>& k_sqr_distances)
Copy link
Member

Choose a reason for hiding this comment

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

Not actionable: k should likely be made uindex_t in a future pass

Copy link
Member Author

Choose a reason for hiding this comment

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

k is currently signed. Should it be make index_t instead?

octree/include/pcl/octree/octree_search.h Outdated Show resolved Hide resolved
octree/include/pcl/octree/octree_search.h Outdated Show resolved Hide resolved
octree/include/pcl/octree/octree_search.h Outdated Show resolved Hide resolved
octree/include/pcl/octree/octree_search.h Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Aug 25, 2020
@kunaltyagi kunaltyagi marked this pull request as draft August 25, 2020 11:02
@kunaltyagi
Copy link
Member Author

Marking draft. Will return with vengeance (every line checked... 😢)

@kunaltyagi
Copy link
Member Author

Several use of int left in octree_pointcloud.hpp
PTAL

@kunaltyagi kunaltyagi mentioned this pull request Sep 19, 2020
Open
@kunaltyagi kunaltyagi force-pushed the octree.64 branch 3 times, most recently from ec343ab to b9e101b Compare September 19, 2020 12:58
@kunaltyagi kunaltyagi marked this pull request as ready for review September 19, 2020 15:24
@kunaltyagi kunaltyagi changed the title Enable arbitary size Indices for Octree and IO modules Enable arbitary size Indices for Octree module Sep 19, 2020
@kunaltyagi kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Sep 19, 2020
@kunaltyagi kunaltyagi requested a review from mvieth September 19, 2020 15:30
@kunaltyagi kunaltyagi mentioned this pull request Oct 1, 2020
Merged
@kunaltyagi kunaltyagi requested a review from larshg October 9, 2020 08:20
@mvieth mvieth mentioned this pull request Nov 14, 2020
Closed
@mvieth
Copy link
Member

mvieth commented Feb 18, 2021

@kunaltyagi Do you maybe want to check why the tests are failing and resolve the conflicts? Then we could hopefully merge this soon.

@kunaltyagi
Copy link
Member Author

@mvieth Thanks a lot for pinging me. I've been so busy, it's been hard to keep track of what's the priority here. :)

Just rebased to current master and will get back to this in this coming week.

@kunaltyagi kunaltyagi requested a review from larshg February 19, 2021 03:04
@kunaltyagi
Copy link
Member Author

@larshg I'll take a look at your existing comments on the weekend.

mvieth
mvieth reviewed Mar 4, 2021
View reviewed changes
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

I think it is sometimes ambiguous whether index_t or uindex_t should be used, but I generally agree with your choices.

io/include/pcl/compression/color_coding.h Show resolved Hide resolved
io/include/pcl/compression/point_coding.h Show resolved Hide resolved
octree/include/pcl/octree/impl/octree_iterator.hpp Show resolved Hide resolved
mvieth
mvieth approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

LGTM, assuming you fix the failing CI check.

@mvieth
Copy link
Member

mvieth commented Mar 13, 2021

@kunaltyagi Merge?

@kunaltyagi
Copy link
Member Author

Rearranged the comments a bit for an easy(er) merge

@kunaltyagi kunaltyagi merged commit ec147ae into PointCloudLibrary:master Mar 14, 2021
@kunaltyagi kunaltyagi deleted the octree.64 branch March 14, 2021 05:44
@mvieth mvieth mentioned this pull request Jul 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larshg larshg larshg approved these changes

@mvieth mvieth mvieth approved these changes

@SergioRAgostinho SergioRAgostinho SergioRAgostinho left review comments

Assignees
No one assigned
Labels
changelog: enhancement Meta-information for changelog generation module: io module: octree needs: code review Specify why not closed/merged yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kunaltyagi @larshg @mvieth @SergioRAgostinho