[wpimath] Add remaining struct and protobuf implementations

[KangarooKoala]

Closes #5888.

Implements struct and protobuf for all of the wpimath/src/main/proto classes except for those implemented in #5391 or #5935.

Notes:

• Adjusts shared/jni/setupBuilder.gradle to more precisely fit what's needed. (Specifically, adds protobuf dependencies to JNIShared because that was needed for Matrixd and Vectord, and removes a protobuf dependency on Dev that doesn't seem necessary (especially considering that there isn't a src dir dependency).
• SwerveDriveKinematics, Vector, Matrix, LinearSystem are protobuf-only because of their repeated members.
• Adds utility methods for dealing with array members, though for C++ I'm not sure how I want to handle the error- I seem to recall concerns about throwing an exception, but maybe that was just for JNI. These could be broken out into a separate PR if required.
• Adds storage to DifferentialDriveFeedforward to aid recover of the constructor arguments.
• Adds double arrays to CubicHermiteSpline and QuinticHermiteSpline. I'm not sure what to do about the mutability of the arrays- We could make them private and add a getter that copies them, but that would be computationally expensive.
• Adds a getter to SwerveDriveKinematics. As the PMD warning indicates, this allows users to mutate the internal array. Like the control vectors for the splines, we could make this getter copy them, but that would be computationally expensive.
• I'm not sure if LinearSystem.getNested() is implemented correctly- Is there one element for each defined protobuf class or for each different implementation instance?
• As MecanumDriveWheelVoltages is only in Java #5929 states, MecanumDriveMotorVoltages is only in Java, so there are no C++ implementations. Given the discussion in that issue, it might be removed anyways, but it's in this PR for now.
• For the C++ templates, I implemented the struct and protobuf in .inc files. Later I'll compare compilation times locally to see how much pulling in the protobuf headers affects things.
• I could add tests for different instantiations of the generic classes, and I do want to add tests to verify that mixing instantiations doesn't work.
• Adds ProtoTestBase and StructTestBase. The C++ versions are implemented a little oddly- I considered a macro, which would allow adding tests within the same group, but that felt like it would be a little too black-magic-y.
• There are some inconsistencies in format, both within this PR and with how [wpimath][proto] Add protobuf / struct serde for trivial types #5935 does things. Things could be adjusted to be consistent if desired.

[PeterJohnson]

I think the right answer where you need to pull the protobuf headers in is to make those .h files that the user has to explicitly pull in if they want that functionality. Compile times are already bad enough.

[KangarooKoala]

Sounds good, though now there's the question of whether or not all of the protobuf headers should be like that for consistency. I could potentially see people getting tripped up over when they need to include the protobuf headers and when they don't, though documentation could help with that.

[PeterJohnson]

Potentially, but we've put in a bunch of work to make sure it's cheap in those other cases.

[KangarooKoala]

I decided to remove the declaration includes from the main header files so that it would be a compilation error instead of a linkage error. I don't understanding what's going on with the Windows error or the clang-tidy error.
For the Windows error, I can see that it's ignoring the desired UnpackStruct overload because "expression did not evaluate to a constant", but I can't figure out what expression it's talking about. The "failure was caused by a read of a variable outside its lifetime" and "see usage of 'this'" isn't helping, because there I can't see what read would be out of the variable's lifetime, nor is there any usages of this.
For the clang-tidy error, the bugprone-forward-declaration-namespace warning isn't very helpful, since there aren't unnamed structs at the specified locations and there aren't any forward declarations.

[KangarooKoala]

Weird that Windows didn't recognize wpi::Protobuf<VectorProtoTestData::Type>::New earlier (see this CI run), especially since the Matrix version worked just fine. Maybe it had something to do with recognizing the using declaration? It did properly expand the using declaration within the function, but it didn't resolve VectorProtoTestData::Type as Vectord<2> or Eigen::Vector<double, 2>. That might just be the amount of using declaration resolution it'll do in messages, though.

[calcmogul]

Eigen types have a lot of defaulted template arguments. GCC and Clang use those default values for overload resolution, but MSVC usually requires that you list them explicitly, even if that's just a variadic template as a catch-all.

Our fmtlib formatter specialization used to do that, but then we started using concepts instead: https://github.com/wpilibsuite/allwpilib/blob/main/wpimath/src/main/native/include/frc/fmt/Eigen.h. .derived() will give you a reference to the derived type instance from a CRTP if you need it.

[KangarooKoala]

Thanks for that advice! I don't know how long it would've taken me to figure that out otherwise, though now I need to figure out why it can't find the protobuf method definitions for the templated types when linking. (I did some testing on a separate branch, with this failure, even after making sure to merge the protobuf build fixes)

[calcmogul]

It may have to do with DLL export. We don't use the export attribute on template classes because the user is going to be generating their own instantiation anyway, and MSVC doesn't like exporting template classes for some reason.

[KangarooKoala]

I tried removing the export in KangarooKoala@79d01c8 (on the other branch) and the error message stayed the exact same... Curious that the methods it can't find are defined by the protobuf library, even though it is able to find other methods:

SimpleMotorFeedforwardProtoTest.cpp.obj : error LNK2019: unresolved external symbol "private: static class wpi::proto::ProtobufSimpleMotorFeedforward * __cdecl google::protobuf::Arena::CreateMaybeMessage<class wpi::proto::ProtobufSimpleMotorFeedforward>(class google::protobuf::Arena *)" (??$CreateMaybeMessage@VProtobufSimpleMotorFeedforward@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufSimpleMotorFeedforward@proto@wpi@@PEAV012@@Z) referenced in function "public: static class wpi::proto::ProtobufSimpleMotorFeedforward * __cdecl google::protobuf::Arena::CreateMessage<class wpi::proto::ProtobufSimpleMotorFeedforward>(class google::protobuf::Arena *)" (??$CreateMessage@VProtobufSimpleMotorFeedforward@proto@wpi@@$$V@Arena@protobuf@google@@SAPEAVProtobufSimpleMotorFeedforward@proto@wpi@@PEAV012@@Z)

[PeterJohnson]

We've updated the build on main to export all wpimath protobuf symbols (#5957). Is this branch up to date with main?

[KangarooKoala]

The branch I've been testing on should be up to date on main (https://github.com/KangarooKoala/allwpilib/tree/tmp-more-serialization), though I could try updating this one as well if you'd like.

[KangarooKoala]

Merge conflicts are because of the changes to Spline, but I can't implement wpi::Struct<typename frc::Spline<Degree>::ControlVector> because it can't deduce Degree from frc::Spline<Degree>::ControlVector (https://stackoverflow.com/a/42391871). I might be able to make a workaround (e.g., define a SplineControlVector<Degree> that will have implicit conversions with Spline<Degree>::ControlVector), or I could just inline the ControlVector members into CubicHermiteSpline and QuinticHermiteSpline.

[PeterJohnson]

Struct implementations will need updating due to #5992.

re: ControlVector, inlining seems like the correct approach. I don’t think anyone will want to directly serialize just the ControlVector.

[PeterJohnson]

The cmake windows build is failing because cmake doesn't have the same export implementation that gradle does to export all symbols in the protobuf implementation files.

[Gold856]

diff --git a/wpimath/CMakeLists.txt b/wpimath/CMakeLists.txt
index a22ffc0b8..b341313a2 100644
--- a/wpimath/CMakeLists.txt
+++ b/wpimath/CMakeLists.txt
@@ -151,9 +151,7 @@ endif()
 file(GLOB_RECURSE wpimath_native_src src/main/native/cpp/*.cpp)
 list(REMOVE_ITEM wpimath_native_src ${wpimath_jni_src})
 
-set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS FALSE)
 add_library(wpimath ${wpimath_native_src} ${WPIMATH_PROTO_SRCS})
-set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
 set_target_properties(wpimath PROPERTIES DEBUG_POSTFIX "d")
 
 set_property(TARGET wpimath PROPERTY FOLDER "libraries")

If the problem is symbol exporting, it looks symbols are explicitly prevented from being exported. This patch might fix that (you might have to do it manually).

[PeterJohnson]

Could we make these package-private instead of public? I guess the problem is the struct/protobuf stuff is in a different package, ugh...

[PeterJohnson]

I think these should just ignore the incoming data if it's incorrectly sized. Either that or throw.

[KangarooKoala]

I don't know what ignoring the incoming data looks like, since we have to return a result, so I guess we throw. Should we define a custom exception type to throw or use a standard exception type?

[KangarooKoala]

To make life easier, I added kTypeName to wpi::Struct<frc::Matrixd<...>> so that wpi::Struct<frc::LinearStruct<...>> could refer to that in its schema. Do we want to add that member to the other structs with a templated type name? Do we want to add that to all structs? (If we add it to all structs, that probably would be in a different PR)

[PeterJohnson]

If we want that, it needs to be a function as well. It might be nice to have for nested use, sure. Agreed it should be a separate PR to do it broadly.

[KangarooKoala]

Reminder so I don't forget- Add period to SimpleMotorFeedforward protobuf and struct definitions because of #6647.

[PeterJohnson]

Now that #6872 has been merged, this should be updated.