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 Rust API. #15

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

francocipollone
Copy link
Contributor

@francocipollone francocipollone commented Feb 5, 2024

🎉 New feature

Related to #14
Goes on top of #12

Summary

  • Creates a maliput rust package
  • Adds math module
  • Adds Vector3 struct and implementation based on maliput-sys cpp bindings.

Test it

An example using vector has been added.

cargo run --example math

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)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

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.

Leaving some comments here also not trying to block merging but to learn more about rust.

assert_eq!(v.z(), 3.0);
assert_eq!(v.norm(), (1.0_f64.powi(2) + 2.0_f64.powi(2) + 3.0_f64.powi(2)).sqrt());
assert_eq!(v.dot(&v), 1.0_f64.powi(2) + 2.0_f64.powi(2) + 3.0_f64.powi(2));
assert_eq!(v.cross(&v), Vector3::new(0.0, 0.0, 0.0));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should allow also default zero initialization in the parent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debatable based on Rust programming ways:

Note we don't have constructors, the new method is just a method, we could add another new_default method if necessary and makes sense.

/// }
/// ```
pub struct Vector3 {
v: cxx::UniquePtr<maliput_sys::math::ffi::Vector3>,
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding, is this the right way of handing intra domain memory compatibility? Using the heap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When creating bindings to Cpp for classes like Vector3 or such (Opaque types) we can only access to them via an indirection. --> Check https://cxx.rs/concepts.html

}
}

mod tests {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it common to keep tests within the same file than the type implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing debug and non debug format checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it common to keep tests within the same file than the type implementation?

Yes. Unit tests should be located at the same file or evean you can create a submodule for tests.

Integration Tests are the ones that are expected to be put under /root/tests folder

See:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we missing debug and non debug format checks?

Not sure if I follow you, can you explain?


}

impl Eq for Vector3 {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting thing about partial equality and equality traits!

Base automatically changed from francocipollone/add_vector3_bindings to main February 9, 2024 20:58
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
@francocipollone
Copy link
Contributor Author

I will proceed and merge this PR @agalbachicar . Feel free to make another pass for a follow-up PR.

@francocipollone francocipollone merged commit a62e8a4 into main Feb 9, 2024
2 checks passed
@francocipollone francocipollone deleted the francocipollone/vector3_rust_api branch February 9, 2024 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants