-
Notifications
You must be signed in to change notification settings - Fork 611
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
[draft] switch wpical backend to gtsam #7606
base: main
Are you sure you want to change the base?
Conversation
18330e1
to
7385556
Compare
7385556
to
6f2a99d
Compare
6f2a99d
to
d34b60f
Compare
it.cppCompiler.args.add("-Wno-deprecated-copy") | ||
it.cppCompiler.args.add("-Wno-redundant-move") |
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.
we should probably fix these upstream instead
inline cameracalibration::CameraModel load_camera_model(wpi::json json_data) { | ||
inline CameraModel load_camera_model(wpi::json json_data) { |
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.
This function feels like it doesn't belong in fieldcalibration code?
struct Pose3WithVariance { | ||
frc::Pose3d pose; | ||
// in order rx ry rz tx ty tz. Units are SI units | ||
std::array<double, 6> covariance; |
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.
member is named covariance but holds variance? swap to cov
|
||
struct TagDetection { | ||
int32_t id; | ||
std::vector<TargetCorner> corners; |
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.
always 4 corners - consider an array
return worldTtags; | ||
} | ||
|
||
static std::vector<Point3> MakeTagModel(double width) { |
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.
consider templating this
// cv::Mat R; | ||
// cv::Rodrigues(rvec, R); // R is 3x3 | ||
// R = R.t(); // rotation of inverse | ||
// cv::Mat tvecI = -R * tvec; // translation of inverse |
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.
dead code
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.
Since we're not in a rush for this, and I know you want some of calibration things in here, considering splitting wpical into app
and lib
directories like Glass, so you can have a libwpical
you can pull in.
I was trying to keep the diff small, but now that wpical is in main im less scared about merge conflicts unless @ElliotScher is planning a similar refactor already? |
I wasn't planning on it yet, but I can if it would help. |
I don't think it would - I think it makes more sense here to swap out the bones and fight the build system, then we can think about other plans? The wpical docs PR going in and getting some user feedback post kickoff seem like plenty of work to me |
This comment was marked as outdated.
This comment was marked as outdated.
ed7c8d7
to
c20e3e5
Compare
Signed-off-by: Jade Turner <[email protected]>
cb0027d
to
41dc097
Compare
Draft MR switching the wpical backend from ceres to GTSAM via the photonvision thirdparty repo.