AnyiLin / Pedro-Pathing-Quickstart

The quickstart for the Pedro Pathing path following program developed by FTC team 10158 Scott's Bots.
BSD 3-Clause Clear License
22 stars 30 forks source link

PoseUpdater constructors should not require a HardwareMap #7

Closed alwoodd closed 1 month ago

alwoodd commented 1 month ago

The PoseUpdater constructors requiring a HardwareMap instance appears to be pointless at best, and confusing at worst. Dealing with a HardwareMap isn't a PoseUpdater concern. The only related class that cares about a HardwareMap is the Localizer the PoseUpdater is managing. The only reason a PoseUpdater constructor might take a HardwareMap is simply as a passthrough to the hard-coded ThreeWheelLocalizer constructor.

I suggest you simplify and clarify the PoseUpdater by having these constructors: 1) Take a single Localizer instance arg. 2) Take no args. This constructor either sets this.localizer with a an instance constructed in the (for example) localization/LocalizationConstants class, or perhaps uses a LocalizerFactory to return a configured Localizer instance.

I am happy to discuss if you'd like.

AnyiLin commented 1 month ago

The PoseUpdater requires a HardwareMap to feed to the localizer that is being used. Since it is improbable that teams will be using 2 localizers at the same time, there is one localizer hard coded into the PoseUpdater class. There is a TODO comment above the line where the localizer is instantiated telling users to replace the ThreeWheelLocalizer with whatever they're using. HardwareMap is only accessible within OpModes and PoseUpdater is used in multiple OpModes besides the Follower, so having to replace a Localizer argument in each use of PoseUpdater would be impractical and unnecessary. Somehow each localizer would need a new HardwareMap argument for each OpMode, so it would need to be passed in anyways. Therefore, changing the structure of the PoseUpdater would not reduce the number of HardwareMaps that would need to be passed in to the code.