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

Completes Lane API bindings for branch points #64

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

francocipollone
Copy link
Contributor

🎉 New feature

Related to #37 #38

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]>
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.

LGTM besides the comment.

/// Check if the `Lane` contains the given `LanePosition`.
pub fn contains(&self, lane_position: &LanePosition) -> bool {
self.lane.Contains(lane_position.lp.as_ref().expect(""))
}
}

/// Copy trait for Lane.
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some doubts about this. Is this public? meaning that others calling clone will be able to execute this code? That we don't want to the best of my knowledge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand the point and I am open to discussion here.

In Cpp when we get a lane we receive a pointer to the Lane (For example via ByIndex::GetLane). And that is fine: We are allowed to copy that pointer and put it here and there.

In Rust land, how can we actually copy that Lane if we want to pass it somewhere else (Of course we can try to always use a reference to the Lane). The Rust Lane class only contains a reference to the cpp Lane so creating a clone to Lane object will be simple to clone the reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine for me. I just want to understand the memory management under the hood:

Using references:

  • Given that Lanes in Rust hide a UniquePtr to a Lane in C++, when we clone the reference, which one owns the UniquePtr? What does it happen upon deletion?

Using pointers:

  • Given that Lanes in Rust hide a UniquePtr to a Lane in C++, is it legit in Rust to copy a pointer to a Rust object as a mechanism to not delegate ownership? Or is it better to use a constant reference? If so, then how does the lifecycle works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions:

Lane is obtained via get_lane method at RoadGeometry. There, a const Lane * is converted to a rust refence, and a lifetime is attached to that reference, the lifetime used is the same of the lifetime of the RoadGeometry object.

See https://doc.rust-lang.org/rust-by-example/scope/lifetime.html

When we copy the Lane object and therefore the underlying reference we are then copying its lifetime as well.

Or is it better to use a constant reference?

Pointers in rust are unsafe, so it is better to always use references.

@francocipollone francocipollone merged commit b2464eb into main Apr 23, 2024
3 checks passed
@francocipollone francocipollone deleted the francocipollone/lane_branch_point_api branch April 23, 2024 20:32
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