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

[wpimath] Alliance Coordinate Symmetry Utility #7118

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

oh-yes-0-fps
Copy link
Contributor

@oh-yes-0-fps oh-yes-0-fps commented Sep 23, 2024

Motivations behind this pr:

  • First and foremost is to provide a way for teams to flip Pose2d based on the alliance, this is borderline required for modern autos and having to reset odometry.
  • A standard API for this type of "flipping" action, other things besides the geometry types can be flipped. For example, there are quite a few types in choreolib that can be "flipped" and keeping API continuity for doing the same action would be nice.
  • Most code works year to year, you could have used most 2023 vendordeps and wpilib in 2024 if it weren't for the fact of version requirements in places. One place this could change is with vendordeps that handle alliance flipping for you, this allows teams to specify old years for flipping functionality. This isn't a massive concern, it's unlikely teams will update old bots code to a new years vendordeps but it is a good thing to consider.

The reason I made Flippable an interface, beyond what is said above abt API continuity, is to help with discoverability. Having a flip() method on these objects will make it much easier to find this functionality than locking it behind a utility class(also helps with verbosity). Moving to this interface meant moving the utility to be part of wpimath instead of wpilibj/c.

I am unsure if all of the flipping in this is mathematically correct, they were just put together quickly to show what would go in this util.

* @return If you are on red alliance.
*/
public static boolean onRed() {
return DriverStation.getAlliance().orElse(Alliance.Blue) == Alliance.Red;
Copy link
Member

Choose a reason for hiding this comment

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

This should throw or something if there isn't an alliance color, not assume blue.

Copy link
Contributor Author

@oh-yes-0-fps oh-yes-0-fps Sep 23, 2024

Choose a reason for hiding this comment

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

I disagree, the driver station is often not available when robot code is first booting. This would result in many ds connectivity checks or the user bypassing these functions altogether to prevent silly crashes. So it's really either they don't throw or we don't include these methods.

Copy link
Member

Choose a reason for hiding this comment

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

this would result in many ds connectivity checks or the user bypassing these functions altogether to prevent silly crashes.

No, that's exactly the point. Throwing and then the code restarting while the DS is disconnected is far better than the code starting up and latching to the wrong alliance. See #5229, #5230

@calcmogul
Copy link
Member

calcmogul commented Sep 23, 2024

This library addition seems a bit overly complex. The minimum API you could get away with is 6 AllianceFlipper member functions, one for each geometry type.

public class AllianceFlipper {
  public AllianceFlipper(int year);
  public Rotation2d flip(Rotation2d);
  ...
};
template <int Year>
class AllianceFlipper {
 public:
  static Rotation2d Flip(const Rotation2d&) const;
  ...
};

In Java, they'd use an if statement on the year to decide which flipping method to use. In C++, they can use constexpr if.

Here's the usage (note that we recommend sampling the alliance color when autonomous starts, not when the robot program starts):

constexpr AllianceFlipper<2024> flipper;
Rotation2d rot = ...;
if (auto alliance = frc::DriverStation::GetAlliance(); alliance.has_value() && alliance.value() == kRed) {
  rot = flipper.Flip(rot);
}

I don't see a reason to support custom flippers because unlike custom AprilTag field layouts at home, alliance flipping only really makes sense to do on regulation fields. Also, if your regulation field isn't in-spec, you'll have other problems like the provided AprilTagFieldLayouts not working.

Copy link
Contributor

@pjreiniger pjreiniger left a comment

Choose a reason for hiding this comment

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

I like the simplicity of commit 1 as opposed to tendriling this to all of the geometry classes

*
* @param year The year to set the flipper to. [2022 - 2024]
*/
public static void setYear(int year) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The other libraries (fieldImages / AprilTagLayout) do this with a game enum.

Comment on lines 131 to 141
public static boolean onRed() {
return onRed.getAsBoolean();
}

/**
* Returns if you are on blue alliance, if the alliance is unknown it will return true.
*
* @return If you are on blue alliance.
*/
public static boolean onBlue() {
return !onRed.getAsBoolean();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be part of the library. It doesn't buy the user / vendordep much over having the user control it.

@oh-yes-0-fps
Copy link
Contributor Author

oh-yes-0-fps commented Sep 24, 2024

Moved text to first comment

@oh-yes-0-fps
Copy link
Contributor Author

/format

@oh-yes-0-fps oh-yes-0-fps changed the title [wpilib] Alliance Flipper Utility [wpilib] Alliance Coordinate Sysmetry Utility Sep 24, 2024
@oh-yes-0-fps oh-yes-0-fps changed the title [wpilib] Alliance Coordinate Sysmetry Utility [wpilib] Alliance Coordinate Symetry Utility Sep 25, 2024
@oh-yes-0-fps oh-yes-0-fps changed the title [wpilib] Alliance Coordinate Symetry Utility [wpimath] Alliance Coordinate Symmetry Utility Sep 25, 2024
@katzuv
Copy link
Contributor

katzuv commented Sep 25, 2024

I think this is cool! Perhaps add another flip() method that uses the current year? It will be much simpler to not need to include the year everywhere.

@oh-yes-0-fps
Copy link
Contributor Author

oh-yes-0-fps commented Sep 25, 2024

It will be much simpler to not need to include the year everywhere.

As of the current implementation, the user never has to provide the year if they don't want to, the year is set as static and defaults to the current year.

@oh-yes-0-fps oh-yes-0-fps marked this pull request as ready for review September 28, 2024 03:24
@oh-yes-0-fps oh-yes-0-fps requested a review from a team as a code owner September 28, 2024 03:24
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.

7 participants