wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.06k stars 614 forks source link

[Build] Consider breaking up wpimath / geometry classes #6743

Open pjreiniger opened 3 months ago

pjreiniger commented 3 months ago

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.

calcmogul commented 3 months ago

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.

The geometry classes depend on more than a small subset of code: Eigen, gcem, units, and wpiutil (which itself also contains a JSON library the geometry classes use). Making and managing separate libraries for these would be painful because our Gradle build system does no dependency tracking whatsoever; we have to list all transitive dependencies everywhere they're used, in the proper order.

This concept could go "all the way" like python does and basically break all of the folders into their own library

Given what I've said above, this seems unrealistic without an actual C++ package manager.

pjreiniger commented 3 months ago

For my experiment, I made a wpilibmath-core which had eigen, gcem, units, and compiled sleipnir (required by ellipse) as its own library, and then could make a wpilibmath-geometry which depended on that and wpiutil.

I'm not super familiar with gradlerio / native tools, but this seems to have some dependency resolution built in.

calcmogul commented 3 months ago

It actually doesn't. Notice how all the transitive dependencies (e.g., ni_libraries) are listed there, not just what it directly depends on.

If you look through allwpilib's build.gradles, you'll also notice when it lists the wpimath library target, it also lists wpiutil. wpiutil is a dependency of wpimath, so it shouldn't need to be listed if depedency resolution worked.

pjreiniger commented 3 months ago

Well, in so much as photonvision can do a one liner to get wpilib for example.

Yes, it has to list out transitive things, but in this case those deps would be well defined and could be added to the appropriate lists there. It also seems like something that could be modified to only have direct dependencies, and then recursed on to get the whole list.

calcmogul commented 3 months ago

It also seems like something that could be modified to only have direct dependencies, and then recursed on to get the whole list.

Someone who actually knows Gradle would have to write that, and @ThadHouse is pretty much the only Gradle wizard we have. He had to write a custom native library handler because Gradle's built-in native support sucks. None of our Maven artifacts include dependency information either.