-
Notifications
You must be signed in to change notification settings - Fork 611
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] Add cosineScale and instance optimize to SwerveModuleState #7113
Conversation
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
/format |
I'm not a fan of this. We chose to only provide immutable APIs because they're much easier to reason about than mutable APIs. In particular mutable variables can be modified by library functions in surprising ways, which requires the user spamming clone() (and incuring a copy) to avoid the side effects. This PR doubles the maintenance overhead for Rotation2d as well. The GC implications of immutable APIs will likely be moot in 2027 (2 years from now) when we have a robot controller with more CPU and RAM (value classes notwithstanding). |
No problem. It was fun to play around with this. I had my doubts too. I just saw many instances of new objects being made and wondered what the impact would be if some were made mutable. Would there still be value in an instance method for cosineScaling? And could the optimize method be made an instance method? |
Not really. It's a one-liner.
I'm ambivalent on this. Is there a benefit to making it an instance method? If we made that change, we'd have to go through a deprecation cycle as usual. |
True but it's easy to mess up.
The only benefit I can see would be less code clutter with |
It's a bit cleaner: SwerveModuleState.optimize(state, angle); Instance method: state.optimize(angle); |
|
It would also be manipulating the state of the SwerveModuleState inside the class. |
I'd accept instance methods for SwerveModuleState.cosineScale() and SwerveModuleState.optimize() as a separate PR (with deprecations for the static methods). |
Please note this a very early draft. I am aware that providing for mutability can lead to some unforeseen results. I'm keeping this simple in the beginning.
Inspiration for this PR
A Swerve Drive typically has 4 modules with getter methods providing
SwerveModulePosition
andSwerveModuleState
objects for the purposes of odometry. In most cases these objects are created and disposed of. This also requires creating and disposing ofRotation2d
objects. These objects are relatively small, but would consume some amount of the processor's time with Garbage Collection when taken in the aggregate.Changes this PR provides.
Rotation2d
class as mutable version calledMutRotation2d
. Mutable methods comparable toRotation2d
's method exist. Note this has not been checked for documentation adjustments. I started with a clone ofRotation2d
and began modifications.SwerveModuleState
's angle field is now aMutRotation2d
object.SwerveModuleState
to alter the state fields.SwerveModuleState
'soptimize
method an instance method. I realized now that I need to leave the original as deprecated, but that can be handled after feedback.cosineScaling
method toSwerveModuleState
instances.Work to Do
SwerveModulePosition
.This PR now only adds a cosineScale method and an instance optimize method for SwerveModuleState