Skip to content
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

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

Conversation

KangarooKoala
Copy link
Contributor

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)

Also change AprilTagFields.loadFromResource() to use the new method
@KangarooKoala KangarooKoala requested a review from a team as a code owner January 31, 2024 23:41
@calcmogul
Copy link
Member

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

@KangarooKoala
Copy link
Contributor Author

KangarooKoala commented Feb 1, 2024

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?

@calcmogul
Copy link
Member

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.

@KangarooKoala KangarooKoala requested a review from a team as a code owner February 2, 2024 05:24
@KangarooKoala KangarooKoala changed the title [apriltag] Add AprilTagFieldLayout.loadStandardField() [apriltag] Add AprilTagFieldLayout.loadField() and deprecate AprilTagFields.loadAprilTagLayoutField() Feb 2, 2024
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After internal discussion, we don't want to deprecate the function in the middle of the season. What we could do instead is merge this without the deprecation into main, then merge the deprecation on the development branch.

@KangarooKoala
Copy link
Contributor Author

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
Copy link
Contributor Author

Closing in favor of #6550.

@KangarooKoala KangarooKoala deleted the add-apriltagfieldlayout-factory branch April 28, 2024 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The AprilTagFieldLayout/AprilTagFields initialization split is confusing
2 participants