Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cleaning SmartStereoProjectionPoseFactor #673

Merged

Conversation

ToniRV
Copy link
Contributor

@ToniRV ToniRV commented Jan 17, 2021

  • ToDo:
  • Split .h/.cpp
  • Use const&, avoid needless and costly copies
  • Lint everything to 80 char otw it's annoying to read.
  • Add asserts where the user could mess up badly (i.e. different sizes for measurements and keys, etc)
  • had to add gtsam_unstable to examples... Can I avoid that? Yes, moved the example to gtsam_unstable
  • calibration() returns a weird const vector, if const of the K_ elements was the intended purpose, the const should be inside the shared_ptr... otherwise the original const is useless.
  • Add a unique Id (Key) to the smart factor, otw there is a ton of bookkeeping to be done by the user (perhaps in another PR)

@ProfFan
Copy link
Collaborator

ProfFan commented Jan 17, 2021

@ToniRV There is a dedicated examples for unstable:
https://github.com/borglab/gtsam/tree/develop/gtsam_unstable/examples

@ToniRV
Copy link
Contributor Author

ToniRV commented Jan 17, 2021

@ProfFan I guess the problem then is that examples/ISAM2_SmartFactorStereo_IMU.cpp
should be in the gtsam_unstable/examples folder instead of examples?
Since it uses:

#include <gtsam_unstable/slam/SmartStereoProjectionPoseFactor.h>

@dellaert
Copy link
Member

I think that’s right. Feel free to move it!
thanks for this PR!

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

I’d move the example to unstable, but other than that, looks great!

@ToniRV ToniRV marked this pull request as ready for review January 18, 2021 19:49
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Cool. I'll merge.

@dellaert dellaert merged commit c383b6c into borglab:develop Jan 18, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants