[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 #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 #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 https://github.com/KangarooKoala/allwpilib/commit/79d01c8cda1d55219f38eb3e8267c15336377be5 (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).

[KangarooKoala]

Thanks for the help, but we can't export all symbols in wpimath because there'd be too many. I've messed around trying to fix this (see KangarooKoala/try-export-macro, KangarooKoala/tmp-more-serialization, and KangarooKoala/try-protobuf-generate), but haven't succeeded yet. (Though if you have any ideas I missed, that'd be great!)

For posterity: Something that took me a while to figure out (in part because I'm new to CMake, but also since it's kind of tricky) is how the PROTOC_OUT_DIR argument in our call to protobuf_generate_cpp works. protobuf_generate_cpp forwards all processed arguments to protobuf_generate, which allows a PROTOC_OUT_DIR argument to be propagated to protobuf_generate(). (protobuf_generate_cpp(srcs headers PROTOC_OUT_DIR ${out_dir} PROTOS ${protos}) will call something like protobuf_generate(APPEND_PATH LANGUAGE cpp EXPORT_MACRO OUT_VAR out_var PROTOS PROTOC_OUT_DIR ${out_dir} PROTOS ${protos}).)

[Gold856]

I managed to fix the build by explicitly exporting the missing symbols. Create an exports.def file (I placed it under wpimath/src/main/native). Add this to it:

EXPORTS
    ??$CreateMaybeMessage@VProtobufSimpleMotorFeedforward@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufSimpleMotorFeedforward@proto@wpi@@PEAV012@@Z
    ??$CreateMaybeMessage@VProtobufTranslation2d@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufTranslation2d@proto@wpi@@PEAV012@@Z
    ?Clear@ProtobufTranslation2d@proto@wpi@@UEAAXXZ
    ??$CreateMaybeMessage@VProtobufSwerveDriveKinematics@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufSwerveDriveKinematics@proto@wpi@@PEAV012@@Z
    ??$CreateMaybeMessage@VProtobufMatrix@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufMatrix@proto@wpi@@PEAV012@@Z
    ??$CreateMaybeMessage@VProtobufVector@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufVector@proto@wpi@@PEAV012@@Z
    ??$CreateMaybeMessage@VProtobufLinearSystem@proto@wpi@@$$V@Arena@protobuf@google@@CAPEAVProtobufLinearSystem@proto@wpi@@PEAV012@@Z
    ?_ProtobufMatrix_default_instance_@proto@wpi@@3UProtobufMatrixDefaultTypeInternal@12@A

In the CMakeLists.txt file, in the if(MSVC) block, add target_link_options(wpimath PRIVATE /DEF:$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src/main/native/exports.def>). This tells MSVC to use our .def file when linking wpimath in addition to other symbol export methods. In this case, we're telling MSVC to export these symbols, resolving CMake builds on Windows. This does have the cost of manually maintaining a symbol list, but since only templated protos are affected, and we don't have many templated protos, I think this is a good enough workaround.

[KangarooKoala]

Thanks! Currently I'm swamped with worlds and school, but hopefully I can update this PR (merge with main, update style, and implement your fix) in around two weeks.

[KangarooKoala]

Thanks for the CMake fix! Now that we know the CMake Windows build passes, I'll push the docs fixes and hopefully that will be everything.

[KangarooKoala]

The Arm32 run and the Arm64 run failed in the upload artifact stage, which I'm guessing is just a network error. The MacOS failure is more confusing, since the errors are for wpiutil/src/test/native/cpp/llvm/Chrono.cpp, which this shouldn't have affected. I did find this StackOverflow post that suggests it may have to due with Mac 13.3 compatibility, but running :wpiutil:compileWpiutilTestOsxuniversalReleaseGoogleTestExeWpiutilTestCpp on my Mac (12.7.4) works. I also found https://review.couchbase.org/c/kv_engine/+/208461 which worked around the same issue with the following diff:

     // Now out of tolerance, every 1000ms only advance system by 100ms. When
     // the check triggers, it will look like system time went backwards.
     EXPECT_EQ(2s, tick(2, 1000ms, 100ms));
     EXPECT_EQ(1, uptimeClock.getSystemClockWarnings());
     EXPECT_EQ(1, uptimeClock.getSystemClockChecks());
     // epoch has moved backwards to account for the apparent system clock jump
     // back. Expected change accounts for the 200ms advance of system time for
     // the 2000ms advance of steady time
     epoch -= 1800ms;
-    EXPECT_EQ(epoch, uptimeClock.getEpoch());
+    EXPECT_EQ(epoch.time_since_epoch().count(),
+              uptimeClock.getEpoch().time_since_epoch().count());
     // Now out of tolerance (system clock is rushing)
     EXPECT_EQ(4s, tick(2, 1000ms, 2000ms));
     EXPECT_EQ(2, uptimeClock.getSystemClockWarnings());
     EXPECT_EQ(2, uptimeClock.getSystemClockChecks());
-    EXPECT_EQ(epoch + 2s, uptimeClock.getEpoch());
+    EXPECT_EQ((epoch + 2s).time_since_epoch().count(),
+              uptimeClock.getEpoch().time_since_epoch().count());
 }

I'm not sure what we want to do (especially since I'm not even sure why it's starting to fail now)...

[KangarooKoala]

Weird, I don't know why some of the CI jobs got cancelled or skipped.

[calcmogul]

Actions has been randomly cancelling the Ubuntu runners since last night for the SleipnirGroup and PhotonVision orgs as well.

[KangarooKoala]

Regarding the mutability of the array members in CubicHermiteSpline, QuinticHermiteSpline, and SwerveDriveKinematics, it would be possible to return an immutable list (either an immutable copy created at construction or an immutable view either created at construction or done per-call) to use instead, though I don't know whether that would be worth the overhead (mostly allocations). Immutability would be really good to have, though.

I'm also not sure I ever got an answer about whether LinearSystem.getNested() was implemented correctly, since I can't find any comments about it in here or in Discord. I also still don't know how to handle dimension mismatching for Vector, Matrix, and LinearSystem.

[KangarooKoala]

Is anything blocking this PR? (Aside from quantity of code to review) The failing CMake build seems to just be vcpkg.

[Gold856]

Per #6772, you'll need to include <wpi/ProtoHelper.h> and switch out every instance of google::protobuf::Arena::CreateMessage with wpi::CreateMessage.

[KangarooKoala]

Thanks for reminding me! I'm still a little unsure about whether I should wait for #6760 or try to go on ahead before it, though.

[PeterJohnson]

GetNested() only needs to return one value per class type, not per instantiation, as this is used just to make sure that the nested class protobuf definitions are published.

[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.