Return self reference from ChassisSpeeds modifiers

[mjansen4857]

This PR is basically identical to #7418, which allows chaining of the toFieldRelativeSpeeds, toRobotRelativeSpeeds, and discretize methods. I'm not exactly sure why that PR was closed, but I think the current implementation makes things a bit more verbose, especially when converting usage of the deprecated static methods to the new instance methods.

For example, a common usage of the static methods could look something like this:

swerve.driveRobotRelative(ChassisSpeeds.fromFieldRelativeSpeeds(
    someXValue,
    someYValue,
    someOmegaValue,
    robotRotation
));

The current implementation that returns void would require the above example to be converted like this:

ChassisSpeeds speeds = new ChassisSpeeds(someXValue, someYValue, someOmegaValue);
speeds.toRobotRelativeSpeeds(robotRotation);
swerve.driveRobotRelative(speeds);

Returning a self reference from these methods allows this conversion to be simplified back down to:

swerve.driveRobotRelative(new ChassisSpeeds(someXValue, someYValue, someOmegaValue)
    .toRobotRelativeSpeeds(robotRotation));

This also has the added benefit of allowing method chaining, but I'm mainly concerned about being able to directly pass the result of these methods to another method without having to create a variable and call a method as an intermediate step.

[mjansen4857]

Honestly I think a better solution here is probably to just make these methods return a new ChassisSpeeds object instead of modifying the original, or un-deprecate the static methods so the user can choose if they want a new object or not. There are a lot of situations where the user may not want the original object modified because it gets used again further down in some method. This happens a bunch in PPLib, for example. The current implementation makes working with those situations pretty rough, since you need to create a copy of the original speeds which is very verbose in java:

ChassisSpeeds prevRobotSpeeds = new ChassisSpeeds(
    prevState.fieldSpeeds.vxMetersPerSecond, 
    prevState.fieldSpeeds.vyMetersPerSecond, 
    prevState.fieldSpeeds.omegaRadiansPerSecond);
prevRobotSpeeds.toRobotRelativeSpeeds(prevState.pose.getRotation());

instead of just:

ChassisSpeeds prevRobotSpeeds = prevState.fieldSpeeds.toRobotRelativeSpeeds(prevState.pose.getRotation());

[KangarooKoala]

I'm not exactly sure why that PR was closed

Here's my understanding of the history:

• [wpimath] Add cosineScale and instance optimize to SwerveModuleState #7113 wanted to aggressively reduce GC allocations by adding a mutable version of Rotation2d and making ChassisSpeeds a mutable API.
• From discussion in that PR, it was decided that only making ChassisSpeeds a mutable API would be done. This was done in [wpimath] ChassisSpeeds fromRelative and discretize methods made instance methods #7115.
• The move from static factories to instance mutators was done purely to reduce Java GC overhead. (See [wpimath] ChassisSpeeds Discretize() and FromRelative() instance methods missing in C++ #7379)
• Allow for inlining of ChassisSpeeds transformation methods #7418 was closed after discussion in the FRC Discord #programming-discussion channel. The main point of the discussion is that mutable APIs in Java are confusing (because of what you've alluded to about the potential of method calls to unexpectedly modifying their arguments), and making the methods return a reference to the modified object makes things even worse because the general expectation is for chaining (common in functional programming) to be without side effects (which is how functional programming is).

I think a better solution here is probably to just make these methods return a new ChassisSpeeds object instead of modifying the original

That would be better than returning a self reference from the ChassisSpeeds modifiers. It comes at the expense of memory allocations, though, so it'd be up to a library maintainer (i.e., @calcmogul) whether the nicer API is worth the memory allocations.

un-deprecate the static methods so the user can choose if they want a new object or not

That's my personal preferred approach (especially since from shortly after the #7418 discussion the change will be reverted for 2027). However, it comes with the drawback of increased maintenance burden.

Thinking more about this, though, the static methods could be implemented in terms of the instance mutators (Just call the methods on a copy/new object), which would mean we would keep only one implementation of the logic. There would still be a lot more overloads, though.

[mjansen4857]

That's my personal preferred approach (especially since from shortly after the #7418 discussion the change will be reverted for 2027). However, it comes with the drawback of increased maintenance burden.

Thinking more about this, though, the static methods could be implemented in terms of the instance mutators (Just call the methods on a copy/new object), which would mean we would keep only one implementation of the logic. There would still be a lot more overloads, though.

Yeah I think removing the deprecation from the factory methods and implementing them to use the instance methods is the best solution. I can rework this PR to do that if a maintainer is on board.

[narmstro2020]

Since I had a hand or two on the previously mentioned PR's. I think these changes are totally fine. I missed this whole discussion due to both prepping for the next season and some medical issues.

[Daniel1464]

Wait, doesn't the current ChassisSpeeds.fromFieldRelativeSpeeds implementation need new allocations anyways?
If you're passing in a ChassisSpeeds supplier to your swerve drivetrain class, you have to make new allocations anyways.
I think if reducing allocations is the goal, it's better to have a builder alternative, i.e:

var newSpeeds = ChassisSpeeds.builder()
    .toFieldRelative(rotation)
    .discretized(0.02)
    .build(xVelocity, yVelocity, zVelocity);

With some optimizations, this would only create a single new ChassisSpeeds object while being more intuitive than ChassisSpeeds.fromFieldRelativeSpeeds and such.

[KangarooKoala]

Wait, doesn't the current ChassisSpeeds.fromFieldRelativeSpeeds implementation need new allocations anyways?

Yes, and it's deprecated for 2025: ChassisSpeeds.fromFieldRelativeSpeeds(double,double,double,Rotation2d)

If you're passing in a ChassisSpeeds supplier to your swerve drivetrain class, you have to make new allocations anyways.

Correct. However, instead of every transformation making a new allocation, if the transformations modify the object in place, then we only make one ChassisSpeeds instance at the start.

I think if reducing allocations is the goal, it's better to have a builder alternative, i.e:

var newSpeeds = ChassisSpeeds.builder()
    .toFieldRelative(rotation)
    .discretized(0.02)
    .build(xVelocity, yVelocity, zVelocity);

With some optimizations, this would only create a single new ChassisSpeeds object while being more intuitive than ChassisSpeeds.fromFieldRelativeSpeeds and such.

Could you explain how this is better? From my second point, what's currently on main (transformations modify the object in place) only allocates once, while this would allocate twice (one for the builder, another for the ChassisSpeeds). I also would be hesitant to say a builder approach is more intuitive, because then you have an intermediate type and a final type, instead of always using the same type. Maybe you're talking about pass by reference causing modifications to show up far from the source?

[Daniel1464]

On second note, it might be better to have mutating versions of the instance methods while making the regular methods immutable. For instance:

var speeds = new ChassisSpeeds(0.0, 0.0, 0.0).toFieldRelative(Rotation2d.fromDegrees(0.0)).mut_discretize(0.02);

I was thinking about the builder as a lower-overhead variant of a mutating toFieldRelative that could be used in the internal library/teams who want lower allocations, but i realized that just having mut_ methods(in a similar vein to the units library) is the better option here.