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

Updated PhotonPoseEstimator examples #1765

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

PaarkG
Copy link

@PaarkG PaarkG commented Feb 8, 2025

Attempts to resolve #1759 and #1757

Updated the AprilTags and PhotonPoseEstimator docs page to reflect the new PhotonPoseEstimator which no longer takes a camera in the constructor, and now uses getAllUnreadResults rather than getLatestResult.

First time writing C++ so please review my syntax :)
Not sure about the third python section either, as I couldn't find an updated version of the python documentation.

@PaarkG PaarkG requested a review from a team as a code owner February 8, 2025 00:52
@mcm001
Copy link
Contributor

mcm001 commented Feb 8, 2025

Broadly looks good! Do you think we should move towards RLIs instead of code snippets?

@PaarkG
Copy link
Author

PaarkG commented Feb 8, 2025

Broadly looks good! Do you think we should move towards RLIs instead of code snippets?

I would be happy to update it to do so from the photonvision examples if that makes sense to you. It does for me now that I'm considering it.

In the PV repo examples (or at least Java), there are like 10 lines for sim before the method is closed. Should I do two RLIs around it, or just manually write the end bracket?

@spacey-sooty
Copy link
Member

Broadly looks good! Do you think we should move towards RLIs instead of code snippets?

We'd need to update the examples for this

@PaarkG
Copy link
Author

PaarkG commented Feb 10, 2025

@mcm001 I've started working on using RLIs and so far my only concern is how certain values are defined and referenced in the examples. For instance, kRobotToCam and kTagLayout are defined in Constants.java, but referenced in Vision.java. When I do an RLI, I would have to leave out kRobotToCam or do two separate code blocks to include it.

I am leaning towards modifying the page so that it aligns more with the model of having the camera defined separately, and implementing separate sections for kRobotToCam, kTagLayout, camera, and pose estimator definitions which would more thoroughly explain them. On the other hand, I could keep the current model of three categories and just double up on code blocks to add necessary code. The only other option I see would be leaving out something like the transform, but that information seems valuable.

I wanted to check with you that the first idea (modifying the section structure) is okay before doing it so that I don't put time into it if it's not something you'd like in the end. I think it would be better overall for people to understand how to use the pose estimator anyways.

@mcm001
Copy link
Contributor

mcm001 commented Feb 10, 2025

Yeah feel free to rip and tear at organization if you think it makes more sense.

Cc @Gold856 re: inspector and RLIs

@PaarkG PaarkG marked this pull request as draft February 10, 2025 15:51
@PaarkG PaarkG marked this pull request as ready for review February 10, 2025 22:05
@PaarkG
Copy link
Author

PaarkG commented Feb 10, 2025

https://github.com/PhotonVision/photonvision/blob/main/photon-lib/py/photonlibpy/photonPoseEstimator.py

From this it looks like python still uses the previous model of providing the camera in the constructor. With that in mind, I think these docs are accurate and I used RLIs for everything.


```

You should be updating your [drivetrain pose estimator](https://docs.wpilib.org/en/latest/docs/software/advanced-controls/state-space/state-space-pose-estimators.html) with the result from the `RobotPoseEstimator` every loop using `addVisionMeasurement()`. TODO: add example note
You should be updating your [drivetrain pose estimator](https://docs.wpilib.org/en/latest/docs/software/advanced-controls/state-space/state-space-pose-estimators.html) with the result from the `PhotonPoseEstimator` every loop using `addVisionMeasurement()`. TODO: add example note

## Additional `PhotonPoseEstimator` Methods
Copy link
Contributor

@mcm001 mcm001 Feb 13, 2025

Choose a reason for hiding this comment

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

Consider directly linking to https://javadocs.photonvision.org/org/photonvision/PhotonPoseEstimator.html and https://cppdocs.photonvision.org/classphoton_1_1_photon_pose_estimator.html here. If we need to beef up these javadocs, let's do that instead?

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.

Docs PhotonPoseEstimator Discrepancy
3 participants