[wpimath] All non-deprecated SimpleMotorFeedforward `calculate` methods require units

[jwbonner]

I haven't been closely following all of the changes to SimpleMotorFeedforward, but it seems like the current set of calculate methods is a bit odd in Java. Here's are the current options:

• calculate(velocity, acceleration) (non-units, deprecated)
• calculate(velocity) (non-units, deprecated)
• calculate(velocity) (units, non-deprecated)
• calculate(currentVelocity, nextVelocity) (units, non-deprecated)

I understand the move from accepting acceleration values to two velocity values, but it looks like the parity between units and non-units methods was lost. It seems like there should be a calculate(currentVelocity, nextVelocity) method that doesn't use units and the calculate(velocity) method should wrap that new method and be un-deprecated (the same way that the the units version of calculate(velocity) wraps the units version of calculate(currentVelocity, nextVelocity)).

[rzblue]

This is because calculate(double velocity, double acceleration) and calculate(double currentVelocity, double nextVelocity) are ambiguous, and same for the velocity-only functions. I believe the plan is to add calculate(double currentVelocity, double nextVelocity) next year to replace the double velocity, double acceleration version.

I agree that this isn't a great situation; I think we should add a note to the documentation explaining the above. I'm also concerned about silent breakage for teams that use the double functions this year, since the code would still compile with the new implementation, but the behavior would be wildly different.

[jwbonner]

At a minimum, it seems odd that calculate(double velocity) is deprecated given that the new implementation would have the same behavior. Regardless, would the documented solution for using FF with kA for non-units be the deprecated method?

[jwbonner]

Also, was there a reason that the new overloads weren't simply given a different name? It seems like that would have solved this issue without breaking parity between units and non-units.

[calcmogul]

Because calculate() is the ideal name. It's short and consistent with the rest of the library.

[jwbonner]

Sure, but that leaves us in the current situation where there is literally no way to use the class without units. Unless, again, the documented solution is to use deprecated methods.

[calcmogul]

there is literally no way to use the class without units

That's intentional for those methods.

[jwbonner]

Why? My understanding is that the plan (before 2027 at least) is to not require that teams use units. Why does no other part of WPILib do the same thing?

[calcmogul]

Because the Java feedforward functions in particular need types for disambiguation. C++ doesn't have this issue because they've always used units.

[calcmogul]

Also, using a different name means it'll take 4+ years to get back to the better name with the deprecation/removal cycles being what they are. Command-based went through this and is still stuck in a wpilibj2 namespace because it's "breakage for the sake of breakage".

[jwbonner]

That's fine, my point is just that there needs to be some documented solution for non-units. If that solution is using deprecated methods that's fine, in which case the Javadocs should clearly state that using the deprecated methods is intended in that situation.

And again, calculate(double velocity) doesn't have a conflict so it seems like that doesn't need to be deprecated at all.