allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

[Build] Consider breaking up wpimath / geometry classes

Open pjreiniger opened this issue 2 months ago • 5 comments

The geometry classes are important core functionality used throughout the project and users code. They currently live inside the monolithic and heavyweight wpimath library, which then forces lots of other libraries to pull in this huge thing in, either directly or transitively.

If a the geometry classes along with a small subset of other base utility classes could be pulled out into its own library, build times should decrease. If there is a good caching mechanism in the build system, less actions will be marked dirty by unrelated changes to higher level C++ classes such as the pose estimators, more tests can be run in parallel, etc. robotpy already does a concept similar to this. This concept could go "all the way" like python does and basically break all of the folders into their own library, but I know Peter doesn't like lots of small libraries (I disagree, but I don't make decisions)

As an example of dependencies chains that can be broken, I ran a query on my bazel version of the project.

# Before separation
bazel query "rdeps(//..., //wpimath/src/main/native:cpp/estimator/SwerveDrivePoseEstimator.cpp)" -k --noenable_bzlmod | wc -l
    425

# After geometry separation
bazel query "rdeps(//..., //wpimath/src/main/native:cpp/estimator/SwerveDrivePoseEstimator.cpp)" -k --noenable_bzlmod | wc -l
    385

The bazel query lists every intermediate step necessary to run everything, including actions to copy the .dll libraries to the right space for all the java tests / examples, so in reality the improvement feels like way more than the ~10% shown.

Some of my favorite wacky dependents affected by modifying the swerve drive pose estimator

//apriltag/...
//glass/...
//datalogtool/...
//outlineviewer/...
//roborioteamnumbersetter/...

Glass needs Pose2d, so it needs wpimath. Meaning when the swerve drive estimator gets updated, the Robot Team Number Setter needs to get re-linked. This seems ridiculous to me. The case is the same for the april tag library, which simply runs april tag detection, no actual pose estimation.

pjreiniger avatar Jun 15 '24 00:06 pjreiniger