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

Provides support for RoadGeometry::IdIndex methods. #46

Merged
merged 2 commits into from
Mar 22, 2024

Conversation

francocipollone
Copy link
Contributor

🎉 New feature

Related to #31 #36

Summary

  • Methods provided by the RoadGeometry::IdIndex subclass are directly added to the RoadGeometry API class.
    This is convenient as subclasses declaration (IdIndex within RoadGeometry) isn't compatible with the binding process.

    • IdIndex::GetLane --> RoadGeometry::get_lane
    • IdIndex::GetLanes --> RoadGeometry::get_lanes
  • Documentation was added to the class

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.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
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.

A few nits here and there. PTAL

@@ -50,6 +56,8 @@ pub mod ffi {
rg: &RoadGeometry,
inertial_position: &InertialPosition,
) -> UniquePtr<RoadPositionResult>;
fn RoadGeometry_GetLane(rg: &RoadGeometry, lane_id: &String) -> *const Lane;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to return a ConstLanePtr instead of a *const Lane?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use of ConstLanePtr is due to a workaround in the GetLanes method. Not sure if it is "better" but I did it for consistency.
Done.

Comment on lines 133 to 141
let mut lanes_vec = Vec::new();
for lane in lanes {
unsafe {
lanes_vec.push(Lane {
lane: lane.lane.as_ref().expect(""),
});
}
}
lanes_vec
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can do the following:

lanes.into_iter().map(| l | Lane { lane: l.lane.as_ref().expect("") }).collect::<Vec<Lane>>()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! Keep dropping more rust good practices 😄

Done.

if (lanes.size() == lanes_cpp.size()) {
return lanes;
}
for (const auto& lane : road_geometry.ById().GetLanes()) {
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
for (const auto& lane : road_geometry.ById().GetLanes()) {
lanes.reserve(lanes_cpp.size());
for (const auto& lane : road_geometry.ById().GetLanes()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice.
Done.

Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
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

@francocipollone francocipollone merged commit bb9b35c into main Mar 22, 2024
3 checks passed
@francocipollone francocipollone deleted the francocipollone/id_index branch March 22, 2024 16:22
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