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

[photon-core] 2D Detection data accuracy #896

Merged
merged 7 commits into from
Oct 15, 2023

Conversation

amquake
Copy link
Member

@amquake amquake commented Aug 19, 2023

  • Use calibration data for 2d target info when available (principal point, FOV)
  • Correct perspective distortion in 2d yaw/pitch info
    • Explanation: The pitch traditionally calculated from pixel offsets do not correctly account for non-zero values of yaw because of perspective distortion (not to be confused with lens distortion)-- for example, the pitch angle is naively calculated as:
      pitch = arctan(pixel y offset / focal length y)
      However, using focal length as a side of the associated right triangle is not correct when the pixel x value is not 0, because the distance from this pixel (projected on the x-axis) to the camera lens increases. Projecting a line back out of the camera with these naive angles will not intersect the 3d point that was originally projected into this 2d pixel. Instead, this length should be:
      focal length y ⟶ (focal length y / cos(arctan(pixel x offset / focal length x)))
      This cannot, however, account for perspective distortion in the calculation of the target center (averaging the contour corners).
  • Undistort detected target corners when calibration data is available?
    • We currently only do this for AprilTag detections when passing to pose estimation. Not sure how correct it would be for all targets or if it's even helpful. Relevant: Undistorting 2d detection results #643 Nah
  • Verified on hardware

@amquake
Copy link
Member Author

amquake commented Sep 19, 2023

@mcm001 Thoughts on inverting 2d target yaw / pitch here to match wpilib?

@mcm001
Copy link
Contributor

mcm001 commented Sep 19, 2023

I'm not sure. I see arguments on both sides. If we yell very loudly that it changed, it'll be fine? Like it needs to be at the top of our release notes.

Or @Bankst should we leave it as is?

@mdurrani808 mdurrani808 added this to the 2024 Beta milestone Sep 26, 2023
@mdurrani808
Copy link
Contributor

I'm not sure. I see arguments on both sides. If we yell very loudly that it changed, it'll be fine? Like it needs to be at the top of our release notes.

Or @Bankst should we leave it as is?

I say we leave it as is and maintain parity with what LL has (large amount of users are coming from there or have experience with LL system anyways).

@amquake amquake marked this pull request as ready for review October 15, 2023 10:09
@amquake amquake requested a review from a team as a code owner October 15, 2023 10:09
@mcm001 mcm001 merged commit c8c9e77 into PhotonVision:master Oct 15, 2023
20 checks passed
@mcm001 mcm001 deleted the 2d-use-calibration branch October 15, 2023 17:44
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