wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 614 forks source link

Add AprilTag Tag Info to Field JSON #6534

Open scarmain opened 6 months ago

scarmain commented 6 months ago

Is your feature request related to a problem? Please describe. I would like to make a custom field for testing robots on (possibly FLL sized with the 2" FTC tags). But by default, the AprilTag sizing and family information is not in the JSON file. I would like to do this to support custom fields better, and possibly also allow more interesting FRC fields at the same time.

The real problem is PhotonVision hard codes the field and tag sizes in their image, and I would like to make it dynamic there, but that is separate to WpiLib. This is only to start the ball rolling to then make an issue over there.

Describe the solution you'd like In the field.json files:

Then have supporting helper functions to get this data from the JSON file.

Describe alternatives you've considered Nothing really, in WpiLib stuff, when setting up AprilTags, you have to have constants for tag size and family, and these can be in the field specific logic now to be more generic. For PhotonVision, this would allow them to support custom playing fields. (LimeLight has their own proprietary field JSON format, but it does support per tag sizes.)

Additional context None

calcmogul commented 6 months ago

We only support tags the official FRC (and maybe eventually FTC) field supports. Furthermore, the AprilTag class only contains ID and pose because that's all robot code cares about; I don't see why tag family and tag size need to be added.

Since robot code would never use this, is this for printing tags instead? In general, we don't add features unless they have a demonstrated concrete use case.

scarmain commented 6 months ago

I mean so I can make a custom field. Obviously, I could provide my custom field JSON to the parser, and it could handle tag locations only. (And yes, I understand that WpiLib would never check in my custom field.)

These 2 fields to add are currently being used in the WpiLib AprilTag robot example, so I don't see how you can say they don't needed to be added. This would mean the robot program doesn't have to keep these field constants, as the JSON file could have them. 1) detector.addFamily("tag36h11", 1); - This could use the information from the JSON 2) var poseEstConfig = new AprilTagPoseEstimator.Config(0.1651, [camera focal info]); - This has the tag size in meters, which could come from the JSON file too, 0.1651m = 6.5".

I guess this brings up a problem in the example, how to handle multiple tag sizes on the field (like FTC did this season), as you would need 2 Pose estimators...

calcmogul commented 6 months ago

That example shows how to detect AprilTags with a USB camera plugged into the roboRIO (not recommended, but it's there for low-resource teams). It only produces camera-to-tag transformations, so it wants the tag family and tag size, but it doesn't care what the tag IDs, field-relative tag locations, or field dimensions are.

The AprilTag field layout JSON is intended to be used with the AprilTagFieldLayout class to provide pairs of tag IDs and field-relative poses. The field dimensions are included in the JSON to support the default red and blue alliance field origins. Here's some example usage with a DifferentialDrivePoseEstimator that assumes one tag. It wants the tag IDs and field-relative tag locations, but it doesn't care what the tag families or tag sizes are.

I suppose in general, we assume the detector is on a different processor than the consumer, and those programs have completely different information needs.

I guess this brings up a problem in the example, how to handle multiple tag sizes on the field (like FTC did this season), as you would need 2 Pose estimators...

You'd need a different AprilTagDetector for each tag family and a different AprilTagPoseEstimator for each tag size. Annotating each of the JSON's tags with family and size doesn't really help either class; it's up to the user to look up what kinds of tags the field has and make detector/estimator instances for them.

scarmain commented 6 months ago

I guess you said my whole point in your last comment:

it's up to the user to look up what kinds of tags the field has and make detector/estimator instances for them.

Why? These are field constants, not robot constants, and I feel the field JSON file should describe these constants.

Anyways, I made a prototype of what changes I would propose. It's not the cleanest code, but enough to prove the point and how it could be useful to someone else. See https://github.com/wpilibsuite/allwpilib/compare/main...scarmain:allwpilib:apriltag-sizes. There definitely could be some work on making the change more transparent with constructors that don't require these new fields. I updated the AprilTag example with how it could be used. I know it would need a C++ implementation too.

I could see people getting a benefit out of these functions. To my knowledge, if you have extra fields in a JSON file that the code doesn't care about, it doesn't hurt anything. But if multiple people of the WpiLib team don't see any value in this, I can stop the work on this change.

calcmogul commented 6 months ago

That change makes writing a detector (a rare event) more general at the expense of more unnecessary serialization and network I/O overhead for everyone else in the much more common case of just consuming tag poses over NetworkTables. Everyone will have to serialize another AprilTag field even though no robot code will actually use it. That change didn't save the detector writer any work either.

scarmain commented 6 months ago

I don't understand the comment about unnecessary network I/O, Where would this table go to impact network I/O? It should stay local to the cpu calculating it. Are you worried about the one time you upload it?

I slightly see the extra work deserializing it, but if you are doing this more than once a power cycle during init, you are doing this wrong. This change would be a couple microseconds once, and should be very negligible.

I will say this too. While I agree, not a lot of people are using the WpiLib AprilTag detection in the RoboRio, these AprilTag libraries ARE being used in PhotonVision. See https://github.com/PhotonVision/photonvision/blob/master/photon-core/src/main/java/org/photonvision/vision/pipeline/AprilTagPipeline.java. You can see they use the WpiLib field JSON too, here: https://github.com/PhotonVision/photonvision/blob/010688006a53d9ce06ea6d02c71ba02003717a5b/photon-core/src/main/java/org/photonvision/common/configuration/SqlConfigProvider.java#L290