-
Notifications
You must be signed in to change notification settings - Fork 614
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
Add AprilTagFieldLayout JSON file and move class to edu.wpi.first.apriltag #4578
Add AprilTagFieldLayout JSON file and move class to edu.wpi.first.apriltag #4578
Conversation
* @return The deserialized layout | ||
* @throws IOException If the resource could not be loaded | ||
*/ | ||
public static AprilTagFieldLayout loadFromResource(String resourcePath) throws IOException { |
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.
If this and the predefined enum end up living together, I would also make a loadFromField
function like I did for the field images
The tags are all defined globally relative to the blue alliance, so all that alliance flag does is change where the origin is to be consistent between alliances (i.e., to the right of your alliance wall). It also reduces the amount of work for autonomous programmers in the typical case of a rotationally symmetric field. The tags have unique IDs, so there's no rotational symmetry to uphold with the tags themselves. |
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.
edu.wpi.first.apriltags has inconsistent pluralization with AprilTagFieldLayout's location of edu.wpi.first.wpilibj.apriltag.
The rotation stuff makes sense, but the real question is does that preclude the dependency chain break by renaming the function? It is absolutely wild to me that this simple library needs to pull in 4 extra shared libraries simply to get an enum value. Due to the static nature of the driver station you can't even cheat in java by using it without bringing the native stuff along since at the very least the HAL gets loaded immediately. |
Does it make sense to move I'm guessing the approval without addressing I'd rather have this in the next beta than debate and miss it, but wanted to share my opinion. |
This is an idealism vs pragmatism thing. Having an enum for alliance color makes dealing with the tag poses as easy as possible for users. Teams get shipped the entire library anyway, so it doesn't really matter if the class is lower in the dependency hierarchy. With that said, an alternative would be for users to change their starting pose themselves based on what DriverStation.getAlliance() says, and removing that functionality from AprilTagFieldLayout (i.e., always assume tag poses are relative to blue alliance). They'd also have to rotate all their trajectories based on the alliance color. |
After thinking about it more and discussing it on Chief Delphi, the coordinate system selection would probably be clearer if we renamed
Users could use DriverStation.getAlliance() to pick a pose matching the color, making rotational symmetry exploitable. if (DriverStation.getAlliance() == DriverStation.Alliance.kBlue) {
fieldLayout.setOrigin(AprilTagFieldLayout.kBlueAllianceWallRightSide);
} else if (DriverStation.getAlliance() == DriverStation.Alliance.kRed) {
fieldLayout.setOrigin(AprilTagFieldLayout.kRedAllianceWallRightSide);
} This should at least be more intuitive than setMirrored(), but eliminate the library dependency on wpilibc/j. |
How likely is it that teams will want to modify the locations of the april tags based on specific field measurements? |
I think that'll depend on how accurately the field is constructed. Seems like measuring their true location directly would be difficult. |
I like this idea. Leaves it open for extension. I implemented it slightly differently than mentioned, but it follows the same concept.
I think your average team doesn't worry about this. This PR also does not prevent you from using the exiting functions to load from a file on disk. If you need that much fine grained control, I would recommend rolling your own altered pair of files. The big thing I was trying to achieve is to have a single source of truth, with easy access for the most common use cases |
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.
Besides minor docs nits, the API parts look good to me.
apriltags/src/main/java/edu/wpi/first/apriltag/AprilTagFieldLayout.java
Outdated
Show resolved
Hide resolved
apriltags/src/main/native/include/frc/apriltag/AprilTagFieldLayout.h
Outdated
Show resolved
Hide resolved
* This changes the GetTagPose(int) method to return the correct pose for your | ||
* alliance. | ||
* | ||
* @param alliance The alliance to mirror poses for. |
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 it a mirror or a 180 degree rotation?
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 latter.
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.
We should probably say that then.
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.
Specifically, it's a 180° rotation around the center of the field because the alliance stations are rotationally symmetric.
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.
Let's PR this fix separately.
I think there is a pretty large benefit in having the april tags distributed with the code (obviously it wouldn't come bundled until the post-kickoff release, but that seems to work fine for glass / shuffleboard / pathweaver). I envision this will greatly reduce the number of "why doesn't the code find my file" if the user doesn't put it in their deploy folder. I see that frequently come up for pathweaver.
Now, the approach can be debated:
Since
AprilTagFieldLayout
lives inwpilibc/j
, bundle the file there. I think this would require duplicating the json file in both projects, which isn't impossible, but it feels icky(current pr) Make a new project that contains the field layout. This way you only have one copy of the file, but GradleRio will need to be updated to implicitly pull in the new dependency.
(my favorite) Same as 2 but
AprilTagFieldLayout
moves to this new project as well, and breaks theDriverStation
dependency (which also breaks deps onhal, ntcore, cscore, cameraserver
. I mentioned this in the original PR.setMirrored
isn't that match of a mental hurdle. I can also envision some teams might not want to do that if they create separate paths for red / blue due to field build inconsistencies. Furthermore, if the game is like 2017 the field isn't rotationally symetrical, the logic in that function falls apart.The json file I'm using here is from the unnoficial apriltag layout mentioned on CD and used at a couple of offseasons. Once the dust settles on how / where you can decide if you want to leave the 2022 example in there for beta teams to use, or wait until kickoff and have 2023 be the first entry in the enum