Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wpimath] Make geometry classes constexpr #7222

Conversation

calcmogul
Copy link
Member

No description provided.

@calcmogul calcmogul requested a review from a team as a code owner October 16, 2024 22:47
@calcmogul calcmogul force-pushed the wpimath-make-geometry-classes-constexpr branch 4 times, most recently from 1cd0310 to 52e13b7 Compare October 16, 2024 23:42
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the Rotation3d(const Eigen::Vector3d&, const Eigen::Vector3d&) constructor be marked constexpr?

@calcmogul
Copy link
Member Author

calcmogul commented Oct 16, 2024

That constructor can't be constexpr because it uses Eigen's QR decomposition, which isn't constexpr. We use that to compute an orthogonal vector when the two input vectors are antiparallel. I'd prefer a cheaper method.

Now that I think about it, we could probably pick any other non-antiparallel vector and do the cross product with that instead.

@calcmogul calcmogul force-pushed the wpimath-make-geometry-classes-constexpr branch 2 times, most recently from 65c4328 to 0be227b Compare October 17, 2024 03:00
@calcmogul
Copy link
Member Author

I'll see if I can make Pose3d::Exp() and Pose3d::Log() constexpr as well.

@calcmogul calcmogul force-pushed the wpimath-make-geometry-classes-constexpr branch 5 times, most recently from 00bab7c to c410496 Compare October 17, 2024 18:49
@calcmogul
Copy link
Member Author

calcmogul commented Oct 17, 2024

I'm not happy with the amount of code duplication in Rotation3d and Pose3d for the constexpr and non-constexpr paths. Idk what we can do about it tho other than patching Eigen to make everything constexpr. I have an Eigen branch for that, but it's annoying to keep up to date because it touches a lot of files (336 out of 746 files, 10k LOC of changes).

@calcmogul calcmogul force-pushed the wpimath-make-geometry-classes-constexpr branch 6 times, most recently from 969b112 to e353886 Compare October 17, 2024 22:20
@calcmogul calcmogul force-pushed the wpimath-make-geometry-classes-constexpr branch from e353886 to b9e2c2b Compare October 17, 2024 22:23
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that ct_matrix will use Eigen if the call is not constant evaluated- Should we just write everything to always use ct_matrix? The downside is overhead from copying the Eigen matrices in the ct_matrix constructor.

wpimath/src/main/native/include/frc/geometry/Rotation3d.h Outdated Show resolved Hide resolved
@calcmogul
Copy link
Member Author

I want to avoid runtime overhead whenever possible.

Co-authored-by: Joseph Eng <91924258+KangarooKoala@users.noreply.github.com>
@PeterJohnson PeterJohnson merged commit 95b9bd8 into wpilibsuite:main Oct 18, 2024
36 checks passed
@calcmogul calcmogul deleted the wpimath-make-geometry-classes-constexpr branch October 18, 2024 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants