Cal3 Base Class #626

varunagrawal · 2020-12-01T22:46:16Z

I went ahead and created the common base class for all calibration models, which is Cal3.
This base class helps remove A LOT of redundancy and copy-pasta. I also took advantage of this opportunity to modernize the code as much as possible, using better conventions (such as base-class equality checking and std::fabs).

Also did a lot of formatting for parts that I've touched. As a result, this is a big PR and may need to go through multiple iterations.

jlblancoc

Good! I just left some random comments...

jlblancoc · 2020-12-02T09:26:19Z

gtsam/geometry/Cal3.cpp

+
+/* ************************************************************************* */
+Cal3::Cal3(const std::string& path)
+    : fx_(320), fy_(320), s_(0), u0_(320), v0_(140) {


I would prefer double fx_ = 320.0f; etc. in the .h (C++11 feature) instead of here at the .cpp. It's easier for the reader to see what are the defaults, and makes harder to be inconsistent between different ctors.

I like your suggestion, but the default constructor uses fx_ = 1, fy_ = 1 etc. Should I replace that with the defaults from this constuctor (from string path)?

Hmm... good point. I think you should leave them like double fx_ = 1.0; etc. for consistency with the default ctor, then here in this ctor from a file, the defaults actually shouldn't matter since at the ctor should either: a) return with the new values from the file, or b) throw an exception if error loading... right?

That's true. Let me update that really quick.

jlblancoc · 2020-12-02T09:28:37Z

gtsam/geometry/Cal3.cpp

+    : fx_(320), fy_(320), s_(0), u0_(320), v0_(140) {
+  char buffer[200];
+  buffer[0] = 0;
+  sprintf(buffer, "%s/calibration_info.txt", path.c_str());


Security issue: potential out-of-buffer write.
Why not?

const auto buffer = path + std::string("/calibration_info.txt");

(would be shorter with c++14's s-operator, but gtsam uses c++11 only).

gtsam/geometry/Cal3.cpp

jlblancoc · 2020-12-02T09:30:26Z

gtsam/geometry/Cal3.h

+  /// @{
+
+  /// Create a default calibration that leaves coordinates unchanged
+  Cal3() : fx_(1), fy_(1), s_(0), u0_(0), v0_(0) {}


refer to comment above. If defaults are like double fx_ = 1; then : Cal3() = default;.

jlblancoc · 2020-12-02T09:30:48Z

gtsam/geometry/Cal3.h

+
+  /// constructor from vector
+  Cal3(const Vector& d)
+      : fx_(d(0)), fy_(d(1)), s_(d(2)), u0_(d(3)), v0_(d(4)) {}


throw is unexpected vector length?

Nice catch! I updated all the vector constructors to have fixed size vectors so this is caught at compile-time (aka no nasty surprises).

jlblancoc · 2020-12-02T09:31:07Z

gtsam/geometry/Cal3.h

+  /// @name Advanced Constructors
+  /// @{
+
+  /// load calibration from location (default name is calibration_info.txt)


Document expected file content format...

jlblancoc · 2020-12-02T09:31:53Z

gtsam/geometry/Cal3.h

+#endif
+
+  /// return inverted calibration matrix inv(K)
+  Matrix3 matrix_inverse() const {


This method name is all lowercase, principalPoint is camel case... (??).

Ah great catch. It would be an API change, but renaming it to inverse() makes more sense?

dellaert

A bunch of comments, but awe-inspiring nice PR !

dellaert · 2020-12-02T19:17:21Z

gtsam/geometry/Cal3.cpp

+Cal3::Cal3(double fov, int w, int h)
+    : s_(0), u0_((double)w / 2.0), v0_((double)h / 2.0) {
+  double a = fov * M_PI / 360.0;  // fov/2 in radians
+  // old formula: fx_ = (double) w * tan(a);


Yeah I should just kill that.

dellaert · 2020-12-02T19:19:26Z

gtsam/geometry/Cal3.h

+#endif
+
+  /// Return inverted calibration matrix inv(K)
+  Matrix3 inverse() const {


move to cpp?

dellaert · 2020-12-02T19:21:50Z

gtsam/geometry/Cal3Bundler.h

-  double u0_, v0_; ///< image center, not a parameter to be optimized but a constant
-  double tol_; ///< tolerance value when calibrating
+ private:
+  double f_ = 1.0f;               ///< focal length


why do we still have this?

Cal3Bundler doesn't use fx_ or fy_ like the base class, just a single, common f value. Though now that I think about it, I can do something about it.

dellaert · 2020-12-02T19:22:55Z

gtsam/geometry/Cal3Bundler.h

@@ -109,6 +111,11 @@ class GTSAM_EXPORT Cal3Bundler {
    return v0_;
  }

+  Matrix3 K() const; ///< Standard 3*3 calibration matrix


this is in base class, should be omitted or made override?

Should override, since we should force skew to be 0, even if it is read from a file.

dellaert · 2020-12-02T19:26:59Z

gtsam/geometry/Cal3Bundler.cpp

-      || std::abs(v0_ - K.v0_) > tol)
-    return false;
-  return true;
+  return (std::fabs(f_ - K.f_) < tol && std::fabs(k1_ - K.k1_) < tol &&


what about base? And I think f should go :-)

Nice catch. Don't know how I missed that, then again this is a ginormous PR.

dellaert · 2020-12-02T19:28:32Z

gtsam/geometry/Cal3DS2_Base.h

@@ -34,65 +34,58 @@ namespace gtsam {
 * but using only k1,k2,p1, and p2 coefficients.
 * K = [ fx s u0 ; 0 fy v0 ; 0 0 1 ]
 * rr = Pn.x^2 + Pn.y^2
- * \hat{Pn} = (1 + k1*rr + k2*rr^2 ) Pn + [ 2*p1 Pn.x Pn.y + p2 (rr + 2 Pn.x^2) ;
+ * \hat{Pn} = (1 + k1*rr + k2*rr^2 ) Pn + [ 2*p1 Pn.x Pn.y + p2 (rr + 2 Pn.x^2)


What, no unicode??? ;-)

Aye aye, Cap'n.

dellaert · 2020-12-02T19:30:53Z

gtsam/geometry/Cal3Fisheye.h

  /// Return dimensions of calibration manifold object
-  virtual size_t dim() const { return 9; }
+  virtual size_t dim() const { return dimension; }


add to base and override if not just return Dim();

Each model has a different dimension (though some have common values for dimension), so I'll have to pick and choose.

Actually, I can just get rid of the subclass dim() methods and use your suggestion of return Dim().

Just checked with a unit test: we will need to override dim(). 😞

dellaert · 2020-12-02T19:31:15Z

gtsam/geometry/Cal3Fisheye.h


  /// Return dimensions of calibration manifold object
-  static size_t Dim() { return 9; }
+  static size_t Dim() { return dimension; }


THis might be harder to put in base class, with static involved?

Yes especially since we are using this to differentiate the various dimensionalities.

dellaert · 2020-12-02T19:43:28Z

gtsam/geometry/Cal3_S2.cpp

+  double delta_u = u - u0_, delta_v = v - v0_;
+  double inv_fx = 1 / fx_, inv_fy = 1 / fy_;
+  double inv_fy_delta_v = inv_fy * delta_v,
+         inv_fx_s_inv_fy = inv_fx * s_ * inv_fy;


dellaert · 2020-12-02T19:46:11Z

gtsam/geometry/tests/testCal3_S2Stereo.cpp

+  CHECK(assert_equal((Vector)intrinsic, (Vector)K.calibrate(image)));
+}
+
+//TODO(Varun) Add calibrate and uncalibrate methods


Stick these in a different branch, PR, rather than landing commented out code.

varunagrawal · 2020-12-02T22:17:10Z

All done! I'll merge once CI passes. 🎉

varunagrawal added 7 commits December 1, 2020 16:02

Base class for all calibration models

04fb339

Refactor Cal3_S2 and Cal3_S2Stereo classes

ad66a59

Refactor all Cal3D based models

42b0537

Refactor Bundler and Fisheye models

17e9b73

Minor updates

8daa6a7

Remove deprecated calibration method from wrapper

f904184

replace typedef with using

9bc63fa

varunagrawal added enhancement Improvement to GTSAM design Design choices cleanup Help clean up old/obsolete aspects of GTSAM refactor Refactoring of code to remove rot labels Dec 1, 2020

varunagrawal requested review from thduynguyen, ProfFan, jlblancoc and dellaert December 1, 2020 22:46

varunagrawal self-assigned this Dec 1, 2020

jlblancoc reviewed Dec 2, 2020

View reviewed changes

varunagrawal added 7 commits December 2, 2020 06:10

modernized default constructors

04597fe

Cal3 code improvements

355d2cf

matrix_inverse() -> inverse()

25b6146

remove using-namespace and fix print test

f7d9710

Added tests for printing, plus small formatting

916771c

stream printing for all calibration models

e488dc8

Improved constructor for loading parameters from file

ecb0af5

varunagrawal requested a review from jlblancoc December 2, 2020 16:06

dellaert approved these changes Dec 2, 2020

View reviewed changes

Override dim(), cleanup, and add unicode

fc0fd1a

varunagrawal requested a review from dellaert December 2, 2020 22:02

varunagrawal added 2 commits December 2, 2020 17:14

Added tests for checking calibration model dimensions

0a55d31

Consistent and better formatting

70bab8e

additional formatting

3ddc999

varunagrawal merged commit 091fa9b into develop Dec 3, 2020

varunagrawal deleted the feature/cal3 branch December 3, 2020 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cal3 Base Class #626

Cal3 Base Class #626

varunagrawal commented Dec 1, 2020

jlblancoc left a comment

jlblancoc Dec 2, 2020

varunagrawal Dec 2, 2020

jlblancoc Dec 2, 2020

varunagrawal Dec 2, 2020

jlblancoc Dec 2, 2020

jlblancoc Dec 2, 2020

jlblancoc Dec 2, 2020

varunagrawal Dec 2, 2020

jlblancoc Dec 2, 2020

jlblancoc Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert left a comment

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

varunagrawal Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal Dec 2, 2020

dellaert Dec 2, 2020

dellaert Dec 2, 2020

varunagrawal commented Dec 2, 2020

Cal3 Base Class #626

Cal3 Base Class #626

Conversation

varunagrawal commented Dec 1, 2020

jlblancoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dellaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varunagrawal commented Dec 2, 2020