-
Notifications
You must be signed in to change notification settings - Fork 767
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
Add a MagPoseFactor for using magnetometer measurements with Pose2/Pose3 #752
Add a MagPoseFactor for using magnetometer measurements with Pose2/Pose3 #752
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some comment and minor syntax updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry for the ridiculous amounts of reviews. Github was unresponsive for a while and I have no idea what happened. |
@dellaert should we consider moving both |
Thanks for the review, @varunagrawal! Made the changes you requested and will wait on the decision about moving out of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@miloknowles we can always consider moving it out of |
Sorry to comment on a closed PR, but glad this got merged! I had actually built a custom factor to do this in my own work and this factor is way more robust, tested, and flexible than the one I had made, so thanks Milo! I just wanted to throw in my 2 cents that this would be great to move out of |
Awesome. Fork, move, PR :-) |
@tmcg0 feel free to open the |
You can go ahead and take care of it! I haven't started it and I'm sure I'd mess up something with the build scripts or python bindings and have to spend a lot of time backtracking fixes. |
This PR adds a new
MagPoseFactor<POSE>
which extends the existingMagFactor
to work withPose2
andPose3
. The factor also includes an optionalbody_P_sensor
in case the magnetometer does not coincide with the body frame. I implemented this factor so that I could use magnetometer measurements within a VIO system, and thought it might be useful to others. It doesn't seem like theMagFactor
gets much use, so no worries if you don't think this factor should be added.