allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

[apriltag] Add AprilTagFieldLayout.loadField() and deprecate AprilTagFields.loadAprilTagLayoutField()

Open KangarooKoala opened this issue 1 year ago • 4 comments

Fixes #6326.

AprilTagFields.loadFromResource() uses AprilTagFieldLayout.loadStandardField() instead of the other way around so that it's easier for people to follow the call path. (Don't need to go to a different class)

KangarooKoala avatar Jan 31 '24 23:01 KangarooKoala

Java version looks good. Just needs a C++ port.

calcmogul avatar Feb 01 '24 04:02 calcmogul

This PR is technically breaking because now including AprilTagFields.h doesn't include the declarations in AprilTagFieldLayout.h. I *could* fix this by adding #include "frc/apriltag/AprilTagFieldLayout.h" at the bottom of AprilTagFields.h (I tested this locally and it works), but I don't think that's behavior we want to support in the first place. On another note, do we still want to deprecate the factories outside of AprilTagFieldLayout (as mentioned in the original issue)? If the core problem is discoverability, then I don't see a particular reason to deprecate and remove the other factories (though I don't have a particularly strong reason to keep them either)- Maybe there's something I'm missing?

KangarooKoala avatar Feb 01 '24 05:02 KangarooKoala

I'd prefer there be only one obvious way to do it, so the old, more hidden way would be deprecated. I also dislike how the old function uses LayoutField, which is just asking for users to get it backwards.

calcmogul avatar Feb 01 '24 05:02 calcmogul

I don't know if it'd be easier (for when development and main are reconciled after the season) to a) merge #6377 and this PR separately into their respective branches or b) to merge #6377 into both and then have a different PR targeting development that's just the deprecation. For now I'm going with the separate PRs approach. (And side note, git cherry-pick made #6377 absurdly easy)

KangarooKoala avatar Feb 17 '24 16:02 KangarooKoala

Closing in favor of #6550.

KangarooKoala avatar Apr 27 '24 19:04 KangarooKoala