Change immutable wpimath types to use records #7828
Conversation
Signed-off-by: Jade Turner <spacey-sooty@proton.me>
spacey-sooty
force-pushed
the
records-geometry
branch
from
cf3d350 to
6bdbf22
Compare
There was a problem hiding this comment.
I know this is still a draft, but here's some thoughts (that you were maybe already planning to do):
- Accessors should be changed across the board to use
name()instead ofgetName(), including classes that aren't using records. Mixing both is just asking for confusion. - The implementations should consistently use either the component field directly or the accessor method, not both.
| pose.getTranslation().getX(), | ||
| pose.getTranslation().getY(), | ||
| pose.getRotation().getRadians()); | ||
| pose.getTranslation().y(), pose.getTranslation().x(), pose.getRotation().getRadians()); |
There was a problem hiding this comment.
| pose.getTranslation().y(), pose.getTranslation().x(), pose.getRotation().getRadians()); | |
| pose.getTranslation().x(), pose.getTranslation().y(), pose.getRotation().getRadians()); |
To be fair this is deprecated for removal, but still.
| @JsonProperty | ||
| public double getX() { | ||
| return m_x; | ||
| } |
There was a problem hiding this comment.
I'll note that the old accessor is annotated with @JsonProperty while the new accessor is annotated with @JsonProperty(required = true, value = "x") because that's what the record component is annotated with in the class definition. Not sure if that'll be an issue, but worth noting.
| } | ||
|
|
||
| /** Translation2d protobuf for serialization. */ | ||
| public static final Translation2dProto proto = new Translation2dProto(); | ||
|
|
||
| /** Translation2d struct for serialization. */ | ||
| public static final Translation2dStruct struct = new Translation2dStruct(); | ||
| public static final Struct<Translation2d> struct = StructGenerator.genRecord(Translation2d.class); |
There was a problem hiding this comment.
I'm still a little bit iffy about using the struct generator since the packing and unpacking process uses reflection (which has overhead) and struct is intended to be a simple, low-overhead logging solution. Efficiency is extra important for these geometry classes, which many other things use and can easily be published many times (think of how many poses are in a trajectory). Of course, we'll have to defer to runtime testing to see if the overhead is actually significant- It might very well not be.
However, there's also the fact that the struct implementations are already written and some of them can't be removed since not everything will be turned into records (i.e. classes with preconditions on their member variables), so I don't see much reason to change the status quo for something that's slower.
There was a problem hiding this comment.
I feel like this lowers the maintainence burden for these classes which is why I made the change.
As for performance it's a one time cost at startup so I don't think it's an issue really
There was a problem hiding this comment.
I feel like this lowers the maintainence burden for these classes which is why I made the change.
Fair. The lack of parity with the other geometry types and with C++ would be annoying, but probably manageable.
it's a one time cost at startup
The bulk of it is a one time cost at startup, but the packing and unpacking processes also use reflection:
There was a problem hiding this comment.
I don't really buy the maintenance burden argument here--these geometry classes are stable and will never accumulate additional members.
No description provided.