wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.06k stars 614 forks source link

[wpimath] Remove or deprecate unused WheelPositions interface/concept #6706

Closed KangarooKoala closed 2 months ago

KangarooKoala commented 3 months ago

6673 migrated Kinematics, Odometry, and PoseEstimator to not use the WheelPositions interface (Java)/concept (C++). The only remaining uses are as superinterfaces of DifferentialDriveWheelPositions and MecanumDriveWheelPositions. We probably could completely remove the interface/concept since it should only impact kinematics/odometry/pose estimator implementations for custom drivetrain types who would be impacted by #6673 already, but I can also see the case for deprecating first for a year.

Once we decide which of removing and deprecating we want to do, this would be a great first PR for somebody looking to start contributing to WPILib!

Alextopher commented 3 months ago

I think removing is the right choice since that is what was done in #6673.

Once we decide which of removing and deprecating we want to do, this would be a great first PR for somebody looking to start contributing to WPILib!

I'm interesting in taking this on

KangarooKoala commented 3 months ago

I think removing is the right choice since that is what was done in #6673.

I'm not sure I get what you mean by this- #6673 removed all usages of WheelPositions in our kinematics, odometry, and pose estimator classes, but it didn't touch WheelPositions itself.

That said, WheelPositions should only be used by kinematics implementations for custom drivebase types, so it's probably fine to remove it without warning (like we did to SwerveDriveWheelPositions). @calcmogul what do you think?

I'm interesting in taking this on

Great! Looks you've already opened a wpilib PR, but let us know if you have any questions! (e.g., how to run formatting and compile locally)

Alextopher commented 3 months ago

Ah - I misunderstood the scope of the change. I understand what you are looking for now.

calcmogul commented 3 months ago

Yea, WheelPositions was only used by us, so it can be removed now that nothing references it.