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

Adds Vector3, Matrix3, Quaternion, RollPitchYaw and api::Rotation bindings #43

Merged
merged 8 commits into from
Mar 25, 2024

Conversation

francocipollone
Copy link
Contributor

@francocipollone francocipollone commented Mar 18, 2024

🎉 New feature

Related to #11 #14

Summary

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if it affects the public API)

Signed-off-by: Franco Cipollone <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]>
Signed-off-by: Franco Cipollone <[email protected]>
Copy link
Contributor

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

Checkpoint, but it's looking good!

std::unique_ptr<Matrix3> Matrix3_new(double m00, double m01, double m02,
double m10, double m11, double m12,
double m20, double m21, double m22) {
return std::make_unique<Matrix3>(std::initializer_list<double>{m00, m01, m02, m10, m11, m12, m20, m21, m22});
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add

#include <initializer_list>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

rust::String Quaternion_to_str(const Quaternion& self) {
std::stringstream ss;
ss << self;
return {ss.str()};
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to add to_str() method to the Quaternion class.

Comment on lines 230 to 232
let m = Matrix3_new(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
let t = Matrix3_transpose(&m);
assert_eq!(Matrix3_row(&m, 1).z(), Matrix3_row(&t, 2).y());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let m = Matrix3_new(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
let t = Matrix3_transpose(&m);
assert_eq!(Matrix3_row(&m, 1).z(), Matrix3_row(&t, 2).y());
let m = Matrix3_new(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
let t = Matrix3_transpose(&m);
let n = Matrix3_new(1.0, 4.0, 7.0, 2.0, 5.0, 8.0, 3.0, 6.0, 9.0);
assert_eq!(Matrix3_equals(&t, &n));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done

@francocipollone
Copy link
Contributor Author

I accidentaly merged #44 which uses this PR's branch as target. So I will continue as this is.

@francocipollone francocipollone changed the title Adds Vector3, Matrix3 and Quaternion bindings Adds Vector3, Matrix3, Quaternion, RollPitchYaw and api::Rotation bindings Mar 25, 2024
@francocipollone
Copy link
Contributor Author

I will go ahead and merge this PR to unblock future PRs. Feel free to drop a review and we can work on a follow-up PR.

@francocipollone francocipollone merged commit 4221740 into main Mar 25, 2024
3 checks passed
@francocipollone francocipollone deleted the francocipollone/more_math_covering branch March 25, 2024 22:29
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.

2 participants