allwpilib icon indicating copy to clipboard operation
allwpilib copied to clipboard

Increase constexpr support in geometry data types

Open dkt01 opened this issue 2 years ago • 13 comments

Makes geometry primitives (Pose2d, Pose3d, etc) more usable in constexpr context. This is useful for using these types as compile-time constants for path waypoints, measurements, and configurations. Tests are included to ensure constructors and functions can be evaluated at compile time.

Only significant implementation change is that Sin(), Cos(), and Tan() are lazy-evaluated in frc::Rotation2d so the angle can be used in constexpr context to support compile-time construction of frc::Pose2d. This should have no impact to public API.

The main limitation to making more functions and classes constexpr is the limited constexpr math capabilities. cmath changes have been proposed, but this would be C++23 or later. Alternative compile-time math libraries exist, but it would be best to only adopt these once std::is_constant_evaluated could be used to choose the compile-time evaluated implementation only when running in a constexpr context.

dkt01 avatar May 14 '22 22:05 dkt01

I think clang-tidy is failing due to some #include statements missing from the new .inc files.

Any recommendation on a clean fix? Seems like a messy solution to include in the .inc files and the .h files even though it should work.

dkt01 avatar May 14 '22 23:05 dkt01

We already add #includes to .inc files for other classes, so we're fine with it.

calcmogul avatar May 14 '22 23:05 calcmogul

We want to upgrade to GCC 11 or 12 on Athena for 2023, which would give us access to std::is_constant_evaluated() and make constexpr Rotation2d cleaner. The earliest versions that have it are GCC 9, Clang 9, and MSVC 2019 v16.11.14 or MSVC 2022 v17.2.

calcmogul avatar May 14 '22 23:05 calcmogul

Great! I'd be happy to revisit this once the new compiler versions are available. Rotation2d is most reliant on these changes in the 2d geometry classes. I didn't really touch 3d due to the can of worms eigen opens.

dkt01 avatar May 14 '22 23:05 dkt01

Yea... Eigen is making some stuff constexpr nowadays, but they're waiting on std::is_constant_evaluated() as well.

I tried making a constexpr Matrix class when working on the CoordinateSystem class to make it and all the 3D geometry classes constexpr, but it was buggy and didn't have API parity with Eigen (some function overloads should only work with Vector). I assume they implemented that with template specializations and derived classes of MatrixBase.

calcmogul avatar May 14 '22 23:05 calcmogul

Maybe we can revisit Rotation2d at a later date, and get Translation and Transform in now?

auscompgeek avatar May 15 '22 01:05 auscompgeek

/azp run

calcmogul avatar May 15 '22 01:05 calcmogul

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 15 '22 01:05 azure-pipelines[bot]

Transform2d uses Rotation2d, so the only thing we can make constexpr without changing Rotation2d is the Translation2d constructor.

calcmogul avatar May 15 '22 01:05 calcmogul

I don’t like the runtime pessimization of this change. It adds a conditional branch and codegen to every operation and increases the size of the structure by 8 bytes.

PeterJohnson avatar May 20 '22 22:05 PeterJohnson

We could wait for the roboRIO compiler upgrade and C++20 to use std::is_constant_evaluated(). What's the timeline on that?

calcmogul avatar May 20 '22 22:05 calcmogul

I’m thinking the upgrade will happen this summer?

PeterJohnson avatar May 20 '22 22:05 PeterJohnson

I'm fine with delaying this change until after the compiler and C++ standard upgrade. The rotation2d implementation change to work with C++17 is definitely suboptimal.

dkt01 avatar May 20 '22 23:05 dkt01

Now that #4239 was merged, is this still blocked?

Starlight220 avatar Oct 20 '22 12:10 Starlight220

Not blocked, I just haven't gotten to updating this PR with the C++20 content yet

dkt01 avatar Oct 20 '22 12:10 dkt01