allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

Refactor inbuilt AprilTagFieldLayout loading process

Open brennenputh opened this issue 2 years ago • 4 comments

Opening PR as draft now to start discussion, but this would contain breaking changes so it should be saved for 2024.

This PR is intended to clean up the API usage when loading official AprilTagFieldLayouts. Right now, there are many ways to accomplish the same task, when there really don't need to be. This PR aims to add methods specifically for loading official field layouts for each year, allowing the total API surface (and therefore confusion) to shrink.

Open to other solutions as well, but I saw this as the simplest way to clearly present the user with the options they should be aware of (constructor for a custom one, specific methods for official layouts).

brennenputh avatar Feb 09 '23 20:02 brennenputh

I like changing from throwing an IOException to a RuntimeException, but I'm uncertain how adding methods reduces the ways to accomplish the same task and/or shrinks the total API surface- Is the end goal to completely deprecate AprilTagFields? (Also, having the alias for the current year seems important)

KangarooKoala avatar Apr 24 '23 17:04 KangarooKoala

but I'm uncertain how adding methods reduces the ways to accomplish the same task and/or shrinks the total API surface- Is the end goal to completely deprecate AprilTagFields?

Yea, seems like we should deprecate AprilTagFields.

having the alias for the current year seems important

Depends on whether we care about the tag layout user code expects changing out from under them when they migrate to the next year's release. Imo, changing the layout should be an explicit decision by the user.

calcmogul avatar Apr 24 '23 17:04 calcmogul

Changing the layout definitely needs to be a conscious decision. Running your bot on a Charged Up field just to have it think it's in the wrong place because the layout went to a Crescendo field without noticing is bad. If something breaks, it needs to be noisy.

brennenputh avatar Apr 24 '23 23:04 brennenputh

Unsure how to go about this for C++, as I am unfamiliar with the code generation that's happening there. Can the methods from AprilTagFields just be moved to AprilTagFieldLayout and aliased in some way?

brennenputh avatar Apr 26 '23 02:04 brennenputh

Since #6326 was closed by other PRs, can this be closed?

Gold856 avatar May 30 '24 04:05 Gold856