Move ApriltagFieldLayout into the VisionSystemSim constructor#2295
Move ApriltagFieldLayout into the VisionSystemSim constructor#2295jlmcmchl wants to merge 14 commits intoPhotonVision:mainfrom
Conversation
| SmartDashboard.putData(tableName + "/Sim Field", dbgField); | ||
| this.tagLayout = tagLayout; | ||
| this.targetModel = targetModel; | ||
| addAprilTags(tagLayout); |
There was a problem hiding this comment.
This means we can delete some code from our examples right?
There was a problem hiding this comment.
Also the potential impact of doing this twice is rendering tag N twice, right?
There was a problem hiding this comment.
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.
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: 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.
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.
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: