-
Notifications
You must be signed in to change notification settings - Fork 182
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
[photonlib] Simulation robustness #874
Conversation
Still kinda stumped on the cause of the NaN results.. here are the offending inputs used in the
outputs:
Changing the input by even an unnoticeably small amount causes the error to go away. I've confirmed that duplicating the code and inputs in another project causes the same NaN result. There were only two instances of this in the tests, and one of them was in Rather than spending more effort trying to get to the bottom of this, its easier to just... add some negligible noise to the input when this happens and retry 😅. |
Perhaps. If protobuf performance doesn't suck, we'll go back to Optional, I think. I don't super care either way |
public static PNPResults estimateCamPosePNP( | ||
public static Optional<PNPResults> estimateCamPosePNP( |
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.
The reason I split these functions is that the behavior the origin of the Pnpresult transform was based on if it sees 1 target or more than one, previously. Is that no longer the case? It looks like still if you see one tag it puts the tag at the origin, not at where it is on the field?
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.
There was a bug in VisionEstimation
previously where that method would give kTagModel.getFieldVertices(knownTags.get(0).pose)
to solvePNP_SQUARE instead of TargetModel.kTag16h5.vertices
(which is required for that solvepnp method and is assumed in the transforms following it). It should now correctly return the origin-to-camera transform for both single and multi tag.
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.
Is there a unit test proving this?
Updated summary |
Makes solvePNP methods returnOptional
s to handle bad inputs and report failures. This reveals two test cases which have NaN solvePNP results-- uncertain yet to the cause of this...Also overwrites java changes from #817 since #742 had duplicate fixesPNPResults
can now be empty (isPresent
= false)PNPResults
solvePNP_SQUARE()
resulted in an estimated transform with NaN values, and attempts to handle it