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

rolling shutter projection factor #751

Merged
merged 8 commits into from
Aug 28, 2021
Merged

Conversation

shteren1
Copy link

Hi @dellaert as promised here is a PR for the rolling shutter projection factor you helped me write.
I'm sorry it contains on unit tests, unfortunately i'm mainly a python developer and i have very little experience with working with cpp code (I'm also sorry if my cpp coding is a bit messy).

I don't know how to compile and run the unit tests in cpp, my use case was always compiling gtsam with python wrappers and using gtsam from python.

I did use this factor a lot running SLAM from calibrated monocular rolling shutter camera on many data sets and it works well.

@ProfFan ProfFan requested a review from dellaert April 22, 2021 18:58
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.

Awesome!!! But without unit tests we cannot merge this. If you have unit tests in python, then please check those in?

@shteren1
Copy link
Author

yes I will add python test, also I'll try to see why it failed to compile, perhaps my clone of gtsam is slightly older and some pieces of code changed.

@ProfFan
Copy link
Collaborator

ProfFan commented Jul 7, 2021

Ping @shteren1

@shteren1
Copy link
Author

shteren1 commented Jul 7, 2021

@ProfFan sorry i've been really busy lately, i will get to it.

@lucacarlone
Copy link
Contributor

@shteren1 @dellaert : I'm planning to implement a smart projection factor for rolling shutter measurements as well. @shteren1: If you want, merge this fork into feature/rollingShutterSmartFactors and I'll make it compile and add unit tests on my side (I'll need to do that for my factor anyways).

@shteren1
Copy link
Author

shteren1 commented Jul 20, 2021

@lucacarlone that would be great! have you looked at my branch? did you figure out what I did here?

PR created: #826

@lucacarlone
Copy link
Contributor

@lucacarlone that would be great! have you looked at my branch? did you figure out what I did here?

PR created: #826

excellent! yes, I read your code and looks great. I already have a test for the interpolation Jacobians and will work on the rest in the next few days. I'll keep you posted!

@dellaert dellaert merged commit b5286d3 into borglab:develop Aug 28, 2021
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.

4 participants