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

spherical camera #861

Merged
merged 35 commits into from
Dec 5, 2021
Merged

spherical camera #861

merged 35 commits into from
Dec 5, 2021

Conversation

lucacarlone
Copy link
Contributor

@lucacarlone lucacarlone commented Aug 28, 2021

Added spherical camera to model generic wide-field of view cameras.
Generalized triangulation functions to allow the development of arbitrary cameras.
Adjusted smart factors to remove pinhole assumption.

@dellaert : I suggest merging the PR "smart factors for arbitrary cameras" first. If possible, can you also add my student Marcus Abate [email protected] to GTSAM and add as reviewer here?

@lucacarlone lucacarlone added the wip This is still a work in progress label Aug 28, 2021
@varunagrawal
Copy link
Collaborator

Awesome. I was hoping to add spherical camera support in the future (did a lot of learning about rectification and the double sphere model this past summer).

@lucacarlone Would it be possible to reduce the noise by not having the extra unrelated commits?

@lucacarlone
Copy link
Contributor Author

Awesome. I was hoping to add spherical camera support in the future (did a lot of learning about rectification and the double sphere model this past summer).

@lucacarlone Would it be possible to reduce the noise by not having the extra unrelated commits?

thanks @varunagrawal ! I suggest reviewing PRs in this order:

@dellaert
Copy link
Member

I'll take a look at #827, which seems to be blocked on my templating comment. I'll check it out on my side and then merge if I can't fix it.

@lucacarlone
Copy link
Contributor Author

I'll take a look at #827, which seems to be blocked on my templating comment. I'll check it out on my side and then merge if I can't fix it.

perfect- thanks!

@lucacarlone lucacarlone added feature New proposed feature and removed wip This is still a work in progress labels Aug 28, 2021
@dellaert dellaert removed the request for review from varunagrawal August 28, 2021 22:08
@dellaert
Copy link
Member

I removed @varunagrawal as reviewer. He (should have) no free cycles :-)

Base automatically changed from feature/rollingShutterSmartFactors to develop August 28, 2021 22:43
@lucacarlone lucacarlone mentioned this pull request Aug 29, 2021
lcarlone added 7 commits November 6, 2021 22:25
# Conflicts:
#	gtsam/geometry/CameraSet.h
#	gtsam_unstable/slam/SmartProjectionPoseFactorRollingShutter.h
#	gtsam_unstable/slam/tests/testSmartProjectionPoseFactorRollingShutter.cpp
…sphericalCamera

# Conflicts:
#	gtsam/slam/SmartProjectionFactorP.h
#	gtsam/slam/tests/smartFactorScenarios.h
#	gtsam/slam/tests/testSmartProjectionRigFactor.cpp
#	gtsam_unstable/slam/SmartProjectionPoseFactorRollingShutter.h
#	gtsam_unstable/slam/tests/testSmartProjectionPoseFactorRollingShutter.cpp
…sphericalCamera

# Conflicts:
#	gtsam_unstable/slam/SmartProjectionPoseFactorRollingShutter.h
…sphericalCamera

# Conflicts:
#	gtsam/slam/tests/testSmartProjectionRigFactor.cpp
@lucacarlone lucacarlone mentioned this pull request Nov 23, 2021
14 tasks
@ProfFan
Copy link
Collaborator

ProfFan commented Nov 23, 2021

@lucacarlone The CI error seems to be a missing typedef Measurement in Cal3Bundler

@lucacarlone
Copy link
Contributor Author

@lucacarlone The CI error seems to be a missing typedef Measurement in Cal3Bundler

woohoo! this helps a lot!! Feel free to push the fix if you want (I may not be able to get to this before the weekend)

@dellaert
Copy link
Member

@ProfFan could you help out with CI?

@lucacarlone
Copy link
Contributor Author

@ProfFan : I feel the problem is that there is a missing template in some function call that gets misinterpreted during compilation: If you look at the log, the error refers to using "[with CAMERA = gtsam::Cal3_S2]" which does not make sense since gtsam::Cal3_S2 is a CALIBRATION, not a CAMERA.

@ProfFan
Copy link
Collaborator

ProfFan commented Dec 1, 2021

@lucacarlone Let me take a look :)

@dellaert
Copy link
Member

dellaert commented Dec 2, 2021

@lucacarlone , @ProfFan and I diagnosed and fixed the template substitution issue. Now there is only one remaining failing test:

 57/264 Test  #58: testTriangulation .............................***Failed    0.07 sec
Not equal:
expected:
[
	0.00180422;
	0.00180422
]
actual:
[
	-0.00180422;
	0.00180421
]
/home/runner/work/gtsam/gtsam/gtsam/geometry/tests/testTriangulation.cpp:605: Failure: "assert_equal(expectedErrorSpherical, actualErrorSpherical, 1e-7)" 
There were 1 failures

on some platforms. Maybe two solutions are possible but the choice is not deterministic?

@lucacarlone
Copy link
Contributor Author

thanks for doing that @dellaert and @ProfFan ! I'll take a look at the test between tonight and tomorrow night.

gtsam/slam/ReadMe.md Outdated Show resolved Hide resolved
gtsam_unstable/slam/ReadMe.md Outdated Show resolved Hide resolved
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.

Some small nits left, otherwise: amazing!

gtsam/geometry/PinholeCamera.h Outdated Show resolved Hide resolved
gtsam/geometry/PinholeCamera.h Outdated Show resolved Hide resolved
gtsam/geometry/SphericalCamera.h Show resolved Hide resolved
gtsam/geometry/SphericalCamera.h Outdated Show resolved Hide resolved
@lucacarlone
Copy link
Contributor Author

@dellaert @ProfFan : this seems to be all set (thanks for your help!!). I'll take a final look and then merge to develop. thanks!

@lucacarlone lucacarlone merged commit c778196 into develop Dec 5, 2021
@ProfFan ProfFan deleted the feature/sphericalCamera branch December 5, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New proposed feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants