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.

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

Two fundamental matrix classes #1884

Merged
merged 14 commits into from
Oct 26, 2024
Merged

Two fundamental matrix classes #1884

merged 14 commits into from
Oct 26, 2024

Conversation

dellaert
Copy link
Member

@dellaert dellaert commented Oct 24, 2024

Overview

Two F matrix classes for GTSFM:

  • a general class based on [Sweeney15] F = USV with S=diag(1,s,0)
  • a "simple" class based on E and two unknown focal lengths (and known principal points)
  • a TripleF class that can do epipolar transfer

The idea is that the latter can be used to immediately optimize the [Sweeney15] objective and optimize for focal lengths at the same time.

Tests

Many unit tests, but key test is

  // Check that transfer works
  EXPECT(assert_equal<Point2>(p[0], triplet.transfer0(p[1], p[2]), 1e-9));
  EXPECT(assert_equal<Point2>(p[1], triplet.transfer1(p[0], p[2]), 1e-9));
  EXPECT(assert_equal<Point2>(p[2], triplet.transfer2(p[0], p[1]), 1e-9));

Next PR

"Someone" should write a TransferFactor and (TransferFactor implemented using numerical derivatives here) implement the derivatives for the transfer. The latter might not be super-trivial, as there is a cross product and normalization from homogeneous coordinates. It's possible that this is easier with expressions. Maybe the next PR should just try to create derivatives for calculating the epipolar lines, i.e., the 3*7 derivative of

leftK().transpose().inverse() * E_.matrix() * rightK().inverse() * p

This might not be as daunting as it seems, as E_.matrix() is just R*[t], and the calibrations are actually rather simple and are just functions of fa_ and fb_, respectively.

Reference

[Sweeney15] https://sites.cs.ucsb.edu/~holl/pubs/Sweeney-2015-ICCV.pdf

gtsam/geometry/FundamentalMatrix.cpp Show resolved Hide resolved
gtsam/geometry/FundamentalMatrix.h Show resolved Hide resolved
Comment on lines +108 to +109
const Point2& ca = Point2(0.0, 0.0),
const Point2& cb = Point2(0.0, 0.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

so these are not in pixel space, but with respect to image center?

Copy link
Member Author

Choose a reason for hiding this comment

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

ca and cb are the pixel centers. If you do not provide ca and cb, then your are promising to provide coordinates with respect to the pixel center, yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to have ca and cb be required inputs so users don't unknowingly make this promise

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, will do

}

//*************************************************************************
Matrix3 SimpleFundamentalMatrix::leftK() const {
Copy link
Contributor

@travisdriver travisdriver Oct 25, 2024

Choose a reason for hiding this comment

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

nit: maybe this should be Ka() (Kb()) since that's how variables from the left (right) view are referred to everywhere else in the code? E.g., fa, fb, Fca, etc. Don't feel super strongly about this though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like it. Also make them private

@dellaert
Copy link
Member Author

I'll incorporate remaining comments in follow-up PRs. Thanks!

@dellaert dellaert merged commit 3d62226 into develop Oct 26, 2024
33 checks passed
@dellaert dellaert deleted the feature/F_matrix branch October 26, 2024 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants