-
Notifications
You must be signed in to change notification settings - Fork 279
Move ApriltagFieldLayout into the VisionSystemSim constructor #2295
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
base: main
Are you sure you want to change the base?
Move ApriltagFieldLayout into the VisionSystemSim constructor #2295
Conversation
| SmartDashboard.putData(tableName + "/Sim Field", dbgField); | ||
| this.tagLayout = tagLayout; | ||
| this.targetModel = targetModel; | ||
| addAprilTags(tagLayout); |
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.
This means we can delete some code from our examples right?
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.
Also the potential impact of doing this twice is rendering tag N twice, right?
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.
Perhaps, I was going to remove addApriltags from the constructor and move targetModel to a parameter to addApriltags
| SmartDashboard.putData(tableName + "/Sim Field", dbgField); | ||
| this.tagLayout = tagLayout; | ||
| this.targetModel = targetModel; | ||
| addAprilTags(tagLayout); |
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.
Also the potential impact of doing this twice is rendering tag N twice, right?
|
This feels a little weirdly architected now. In particular, I'm concerned about what happens if a team decides to add some extra AprilTags to their simulated vision system, because if that happens, PhotonCameraSim will basically ignore them. That's the behavior right now anyways, but I wonder if we want to change that now, or later. In my ideal world, |
|
Yes, the use of a tag layout has always been a little weird here. In the past we talked about getting/setting the layout over NT, which is why you see this: photonvision/photon-lib/src/main/java/org/photonvision/simulation/PhotonCameraSim.java Lines 572 to 576 in f461159
The camera sim awkwardly needs a layout because we are simulating the multitag estimation which happens on the coprocessor-- similarly, part of setting up the simulation should mirror the real coprocessor setup. Usually that just means using the default layout and nothing must be done, but users may alternatively upload any custom layout. I guess one question to answer is: should users be able to define only a subset of the actual field tag layout for multitag estimation? That was my original intention, and as an example many teams explicitly ignored the barge tags last year as they could throw off estimates (Tangentially, IIRC photon yells at you if it detects tags which aren't in your layout?). That would necessitate this distinction between Even before then, though, I'm not sure this change matches the intention of the classes. If people are confused by the difference in these tag layout uses because of the no-layout-arg constructors, would it help if we removed them (constructors)? |
| public VisionSystemSim(String visionSystemName) { | ||
| this(visionSystemName, AprilTagFieldLayout.loadField(AprilTagFields.kDefaultField)); | ||
| } |
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.
I don't think we should add a tag layout by default. That would impede creating simulations of object detection / color filtering only pipelines.
| public VisionSystemSim( | ||
| String visionSystemName, AprilTagFieldLayout tagLayout, TargetModel targetModel) { |
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.
What is the motivation for accepting a target model here? The only thing I can think of is teams with non-standard tag sizes, which seems a bit farfetched. It also may confuse users about other types of vision targets, especially if it blanketly says "The model to use for vision targets."
Description
See title - frequent confusion arises around kickoff because at this time of year, the
defaultapriltag layout is the prior year's layout. This causes confusion when using VisionSystemSim and PhotonCameraSim, because documentation around the use of PhotonCameraSim makes it non-obvious that the 3-arg constructor is necessary in order to use your own apriltag field layout, even if the VisionSystemSim it exists within is given reference to the apriltag field layout.WIP, having trouble running java + cpp tests so I'm hoping they run in CI.
Meta
Merge checklist: