Skip to content

Fix used corners being returned from mrcal and persist corner ids - #2574

Open
thatcomputerguy0101 wants to merge 9 commits into
PhotonVision:mainfrom
thatcomputerguy0101:mrcal-id-fix
Open

Fix used corners being returned from mrcal and persist corner ids#2574
thatcomputerguy0101 wants to merge 9 commits into
PhotonVision:mainfrom
thatcomputerguy0101:mrcal-id-fix

Conversation

@thatcomputerguy0101

Copy link
Copy Markdown
Contributor

Description

What changed?

This updates to a fixed version of mrcal-java that returns a list of corners used corresponding to the same subset of corners it was provided. Additionally, this persists the corner ids so that further processing of the board knows which subset of corners it is working with, and updates one of the test calibrations with the ids. The storage space could probably be optimized slightly by dropping the list of ids if all corners are detected, but that gets slightly complicated if the ids are unsorted so it wasn't implemented yet.

Why?

#2344 depends on the stored calibration point information being accurate. It wasn't before.

Testing

  • I have tested this change locally
  • Test evidence (screenshots, videos, or test results):

Related Issues

Blocked on PhotonVision/mrcal-java#31.

AI Disclosure

  • This PR was authored entirely by me
  • This PR includes AI-generated code (e.g., from GitHub Copilot, ChatGPT)
    • If yes, I have reviewed all AI-generated code for correctness
    • If yes, please describe which parts were AI-assisted:

Describe AI involvement here if applicable


Merge Checklist

Additional Checks (if applicable)

  • User-facing changes? User documentation is updated
  • Breaking changes? Migration guide is included in description
  • Bug fix? Regression test is added
  • New dependency? License compatibility is verified and steps have been taken to follow it
  • Serde changes? All messages are regenerated with no unexpected hash changes
  • Configuration changes? Changes are backwards compatible with previous season's last release
  • Pipeline/data exchange changes? Frontend types are updated in ./photon-client/src/types

@thatcomputerguy0101
thatcomputerguy0101 requested a review from a team as a code owner August 2, 2026 01:35
@github-actions github-actions Bot added the backend Things relating to photon-core and photon-server label Aug 2, 2026
Comment thread photon-core/src/main/java/org/photonvision/vision/pipe/impl/Calibrate3dPipe.java Outdated
mcm001
mcm001 previously approved these changes Aug 3, 2026

@mcm001 mcm001 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise lgtm

@thatcomputerguy0101 thatcomputerguy0101 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of why the old implementation seemed to work is that OpenCV outputs sorted corners, so most partial detections were probably being marked as outliers.

level = 0.0f;

if (ids.rows() == params.boardWidth * params.boardHeight) {
// OpenCV detector always outputs corners sorted by ID, so it is safe to not sort the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorts are generally very cheap anyways

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now has an extra-cheap sort implementation that takes advantage of the ids being a continuous range.

// Decimation was not used
level = 0.0f;

if (ids.rows() == params.boardWidth * params.boardHeight) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new sorting algorithm here, but I don't think any of our unit tests make assertions directly against findBoardCorners -- we just do it in a roundabout way that the calibration parameters are vaguely correct. If this code is no longer just plumbing lists out of opencv, let's add some sort of tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. I was considering writing tests, but decided not to at the time since isolating the sorting algorithm was a little hard. I'll give it another try though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend Things relating to photon-core and photon-server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants