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

[registration] Better Generalized ICP optimizer gradient check management #3854

Merged
merged 10 commits into from
Apr 30, 2020
Merged

[registration] Better Generalized ICP optimizer gradient check management #3854

merged 10 commits into from
Apr 30, 2020

Conversation

blackccpie
Copy link
Contributor

Closes #3813

@blackccpie
feat : started dissociating linear and angular gradient tolerance. bo…
f517e9e 
…th are now configurable through gicp interface. still some work to get rid of euler angles vector norm.
@blackccpie
refactor : angular gradient test is now based on rotation axis angula…
383a183 
…r magnitude.
@blackccpie
refactor : nested class has access to enclosing class attributes.
35a7b54 
refactor : added inline method attributes.
@blackccpie
Merge pull request #1 from PointCloudLibrary/master
c81272b 
merge from upstream
@blackccpie
Copy link
Contributor Author

I intentionnally left bfgs.h unformatted for now for the sake of review clarity, but a clang formatting pass would be needed.

@SergioRAgostinho SergioRAgostinho self-requested a review April 3, 2020 12:58
@SergioRAgostinho SergioRAgostinho added changelog: enhancement Meta-information for changelog generation module: registration needs: code review Specify why not closed/merged yet labels Apr 3, 2020
kunaltyagi
kunaltyagi approved these changes Apr 3, 2020
View reviewed changes
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. Let's get 1 more pair of eyes.

@SergioRAgostinho SergioRAgostinho added the changelog: API break Meta-information for changelog generation label Apr 21, 2020
SergioRAgostinho
SergioRAgostinho reviewed Apr 29, 2020
View reviewed changes
registration/include/pcl/registration/bfgs.h Outdated Show resolved Hide resolved
registration/include/pcl/registration/bfgs.h Show resolved Hide resolved
registration/include/pcl/registration/gicp.h Outdated Show resolved Hide resolved
registration/include/pcl/registration/gicp.h Outdated Show resolved Hide resolved
registration/include/pcl/registration/gicp.h Outdated Show resolved Hide resolved
registration/include/pcl/registration/impl/gicp.hpp Outdated Show resolved Hide resolved
registration/include/pcl/registration/impl/gicp.hpp Outdated Show resolved Hide resolved
registration/include/pcl/registration/impl/gicp.hpp Outdated Show resolved Hide resolved
registration/include/pcl/registration/gicp.h Outdated Show resolved Hide resolved
@SergioRAgostinho SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Apr 29, 2020
SergioRAgostinho
SergioRAgostinho requested changes Apr 29, 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.

Just flagging it to highlight that this should not be merged just yet.

@blackccpie @kunaltyagi
Update registration/include/pcl/registration/bfgs.h
73e6b41 
fix : remove API break in favor of deprecation label.

Co-Authored-By: Kunal Tyagi <tyagi.kunal@live.com>
@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Apr 29, 2020
@SergioRAgostinho
Copy link
Member

SergioRAgostinho commented Apr 30, 2020 •
edited
Loading

🚀 if CI is green(ish)

@kunaltyagi kunaltyagi merged commit 0feed18 into PointCloudLibrary:master Apr 30, 2020
@blackccpie blackccpie deleted the features/gicp_issue3813 branch April 30, 2020 16:21
@taketwo taketwo removed the needs: more work Specify why not closed/merged yet label May 6, 2020
@jiancong0204 jiancong0204 mentioned this pull request Mar 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larshg larshg larshg left review comments

@kunaltyagi kunaltyagi kunaltyagi approved these changes

@taketwo taketwo taketwo approved these changes

@SergioRAgostinho SergioRAgostinho SergioRAgostinho approved these changes

Assignees
No one assigned
Labels
changelog: API break Meta-information for changelog generation changelog: enhancement Meta-information for changelog generation module: registration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[registration] GICP not estimating rotation transform correctly
5 participants
@blackccpie @SergioRAgostinho @larshg @taketwo @kunaltyagi