Skip to content

Raise minimum images for calibration to 100#2437

Open
spacey-sooty wants to merge 16 commits into
PhotonVision:mainfrom
spacey-sooty:update-minimum-images
Open

Raise minimum images for calibration to 100#2437
spacey-sooty wants to merge 16 commits into
PhotonVision:mainfrom
spacey-sooty:update-minimum-images

Conversation

@spacey-sooty
Copy link
Copy Markdown
Member

@spacey-sooty spacey-sooty commented Apr 22, 2026

Description

We can see on
https://mrcal.secretsauce.net/docs-2.0/tour-choreography.html that more images appear to always be better, but diminishing returns kick in around 100 images. This shouldn't be too time consuming as a minimum and should get users the best effort to quality ratio so I think to makes sense as the default.

This PR also adds a bypass method for developers looking to test that will lower the limit to 10 images.

image

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why, including events that led to this PR
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with all settings going back to the previous seasons's last release (seasons end after champs ends)
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added
  • If this PR adds a dependency, the license has been checked for compatibility and steps taken to follow it

@spacey-sooty spacey-sooty requested a review from a team as a code owner April 22, 2026 02:23
@github-actions github-actions Bot added the frontend Having to do with PhotonClient and its related items label Apr 22, 2026
@spacey-sooty spacey-sooty requested a review from mcm001 April 22, 2026 02:23
samfreund
samfreund previously approved these changes Apr 22, 2026
@samfreund samfreund changed the base branch from main to 2027 April 22, 2026 02:23
@samfreund samfreund dismissed their stale review April 22, 2026 02:23

The base branch was changed.

@samfreund samfreund force-pushed the update-minimum-images branch from 58c5d67 to c85b45f Compare April 22, 2026 02:23
samfreund
samfreund previously approved these changes Apr 22, 2026
@mdurrani808
Copy link
Copy Markdown
Contributor

This shouldn't be too time consuming

Is this true?

@samfreund
Copy link
Copy Markdown
Member

This shouldn't be too time consuming

Is this true?

I've done like 200 in 10 minutes or so, assuming you have someone else to click the button that's about a picture every three seconds. For only 100, that's a picture every six seconds.

@spacey-sooty
Copy link
Copy Markdown
Member Author

I quite comfortably did ~250 image calibrations in the pits at worlds in 2025 between matches so I don't see 100 being a huge barrier

@samfreund
Copy link
Copy Markdown
Member

Some sort of bypass option may be useful

@samfreund samfreund dismissed their stale review May 2, 2026 19:10

The merge-base changed after approval.

@samfreund samfreund requested a review from a team as a code owner May 2, 2026 19:10
@samfreund samfreund force-pushed the 2027 branch 4 times, most recently from 439f5fc to b8e9e4f Compare May 3, 2026 18:37
@samfreund samfreund deleted the branch PhotonVision:main May 5, 2026 16:43
@samfreund samfreund closed this May 5, 2026
@samfreund samfreund reopened this May 5, 2026
@samfreund samfreund changed the base branch from 2027 to main May 5, 2026 16:47
@samfreund samfreund force-pushed the update-minimum-images branch from c85b45f to b615e69 Compare May 5, 2026 18:24
@github-actions github-actions Bot added the backend Things relating to photon-core and photon-server label May 6, 2026
@samfreund samfreund force-pushed the update-minimum-images branch 2 times, most recently from 49f300e to 8c974b1 Compare May 6, 2026 01:12
@samfreund
Copy link
Copy Markdown
Member

image image

@RKBoss6
Copy link
Copy Markdown

RKBoss6 commented May 6, 2026

I'd say 50 is a nice compromise. For our team at least, calibrations take longer to make sure we get proper images, and having it raised to 100 will improve frustrations for quite a few teams (including ours) that calibrate slower and need to quickly calibrate. To me, being limited by the OS to collect 100 images rather than 12 doesn't seem like a good change for usability and quick setup.

That being said though, maybe we could have a limit of 50 and then a small banner in the calibration menu somewhere that says 100 images is strongly recommended...

@samfreund
Copy link
Copy Markdown
Member

I'd say 50 is a nice compromise. For our team at least, calibrations take longer to make sure we get proper images, and having it raised to 100 will improve frustrations for quite a few teams (including ours) that calibrate slower and need to quickly calibrate. To me, being limited by the OS to collect 100 images rather than 12 doesn't seem like a good change for usability and quick setup.

That being said though, maybe we could have a limit of 50 and then a small banner in the calibration menu somewhere that says 100 images is strongly recommended...

Have you read the linked document? We're choosing 100 images because that's the empirically defined point of diminishing returns, but if you can provide evidence that you'll be able to get sufficient results with 50 images all the time, we might be willing to reconsider. Something to keep in mind here is that we want to help teams fall into success, and that means forcing them to get a good calibration. If we give them a recommendation, many teams won't follow it.

@samfreund
Copy link
Copy Markdown
Member

We're also going to try exploring some new calibration methods/guides over the offseason, TBD what that looks like

@samfreund samfreund force-pushed the update-minimum-images branch from 1248520 to 64a6641 Compare May 6, 2026 17:14
@spacey-sooty
Copy link
Copy Markdown
Member Author

spacey-sooty commented May 6, 2026

Current issues with bypass:

  • idk if this is an existing bug but if i reload the ui resets snapshots to 0 until i take another snapshot
  • with bypass off i can click calibrate after 10 images
  • bypass isn't sticky between reloads
  • the minimum image in the snapshots counter on the left doesnt update

@samfreund samfreund force-pushed the update-minimum-images branch from 64a6641 to 0896631 Compare May 6, 2026 17:45
@samfreund
Copy link
Copy Markdown
Member

samfreund commented May 7, 2026

I hath discovered that this is quite cursed.

Basically we send stuff to the backend for calib using this guy:

export interface WebsocketCalibrationData {
patternWidth: number;
boardType: number;
hasEnough: boolean;
count: number;
minCount: number;
videoModeIndex: number;
patternHeight: number;
squareSizeIn: number;
markerSizeIn: number;
}
. That's kinda bad, cause when we want an update, it's gotta get pushed back through, and we can't really request it or anything. It also doesn't persist if we reload the page, which is hella cooked. Anyways, I propose we just merge everything that changes during the calib process into the pipeline settings.

@dkogan
Copy link
Copy Markdown

dkogan commented May 7, 2026

Hi. I'm the mrcal; just stumbled on this. The numbers on the choreography page are scenario-specific, and you'll get something different with different cameras (fov, resolution) and different geometry and a different chessboard. Sometimes 50 images is more than enough, and sometimes not. mrcal gives you uncertainty maps, so you can get 50 (or whatever) images, look at the uncertainty, and use that to decide if you need more images or not. I don't know if your tooling makes that difficult, but that would be my recommendation.

@samfreund samfreund force-pushed the update-minimum-images branch from 109c5b7 to ffd386d Compare May 11, 2026 03:13
Comment on lines +561 to +562
:label-cols="6"
:switch-cols="6"
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.

Image

Wraps weirdly for me at 1920x1080.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

has this been fixed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread photon-client/src/components/cameras/CameraCalibrationCard.vue Outdated
@samfreund samfreund requested a review from mcm001 May 11, 2026 04:29
};

const drawAllSnapshots = ref(true);
// We really gotta fix our typing system
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

is this a legit todo? in that case cut an issue with more details

spacey-sooty and others added 16 commits May 16, 2026 17:40
We can see on
https://mrcal.secretsauce.net/docs-2.0/tour-choreography.html that more
images appear to always be better, but diminishing returns kick in
around 100 images. This shouldn't be too time consuming as a minimum and
should get users the best effort to quality ratio so I think to makes
sense as the default.
Co-authored-by: Matt Morley <matthew.morley.ca@gmail.com>
@samfreund samfreund force-pushed the update-minimum-images branch from 3517f61 to d97a532 Compare May 16, 2026 21:40
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 frontend Having to do with PhotonClient and its related items

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants