Builder for BranchPoints #24

agalbachicar · 2022-09-26T11:12:27Z

🎉 New feature

Part of #10

Summary

Implements the BranchPoint builder.

Test it

Via unit tests (not yet).

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.

Note to maintainers: this PR is in draft state. There are missing tests and it doesn't compile yet but it just aims to show how it would be used for preliminary discussions.

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

agalbachicar · 2022-09-26T11:13:10Z

@francocipollone I would appreciate your review on 90ccc36. I am mostly interested in knowing your thoughts on its use.

francocipollone

lgtm overall.
I agree with the proposed api for the branchpoint building. I left some minor comments.

Populating the branchpoints is something that in general has to be implemented in each backend. I think that having this helper as agnostic to the backend as it is at maliput::base could be beneficial to avoid reimplementing code? Wdyt?

francocipollone · 2022-09-26T11:49:12Z

include/maliput_sparse/builder/builder.h

@@ -337,6 +376,12 @@ class RoadGeometryBuilder final {
  void SetJunction(maliput::common::Passkey<JunctionBuilder>,
                   std::unique_ptr<maliput::geometry_base::Junction> junction);

+  void SetBranchPoints(maliput::common::Passkey<BranchPointBuilder>,


Missing docstring

francocipollone · 2022-09-26T11:50:08Z

src/builder/builder.cc

+  std::set<BranchPointBuilder::LaneEnd> b_side{};
+};
+
+// Proivdes maliput::api::BranchPointId with an increasing counter to make each ID


typo: Proivdes to Provides

francocipollone · 2022-09-26T11:51:05Z

src/builder/builder.cc

+  }
+
+ private:
+  int branch_point_count_{0};


could be unsigned as negatives aren't being used?

Yes but it actually won't go negative until we go over int range. Do you think this is a problem?

francocipollone · 2022-09-26T11:57:13Z

src/builder/builder.cc

+
+void RoadGeometryBuilder::SetBranchPoints(
+    maliput::common::Passkey<BranchPointBuilder>,
+    std::vector<std::unique_ptr<maliput::geometry_base::BranchPoint>>&& branch_points) {


We probably have to assert that the vector isn't size zero also.

francocipollone · 2022-09-26T11:57:51Z

src/builder/builder.cc

+        for (int i = 0; i < junction->num_segments(); ++i) {
+          const maliput::api::Segment* segment = junction->segment(j);
+          for (int j = 0; j < segment->num_lanes(); ++j) {
+            maliput::geometry_base::Lane* lane = segment->lane(j);


nit: add const

francocipollone · 2022-09-26T12:35:14Z

src/builder/builder.cc

+    const maliput::api::LaneId& lane_id_a, const maliput::api::LaneEnd::Which which_a,
+    const maliput::api::LaneId& lane_id_b, const maliput::api::LaneEnd::Which which_b) {
+  const LaneEnd lane_end_a = std::make_pair(lane_id_a, which_a);
+  const LaneEnd lane_end_b = std::make_pair(lane_id_a, which_b);


Suggested change

const LaneEnd lane_end_b = std::make_pair(lane_id_a, which_b);

const LaneEnd lane_end_b = std::make_pair(lane_id_b, which_b);

Changed the implementation.

francocipollone · 2022-09-26T12:49:46Z

src/builder/builder.cc

+    it_a_side->b_side.insert(b_side_set.begin(), b_side_set.end());
+  } else if (it_b_side != branch_point_sets->end()) {
+    auto a_side_set = GetLaneEndSet(lane_end, lane_ends);
+    it_b_side->b_side.insert(a_side_set.begin(), a_side_set.end());


Suggested change

it_b_side->b_side.insert(a_side_set.begin(), a_side_set.end());

it_b_side->b_side.insert(b_side_set.begin(), a_side_set.end());

Changed the implementation.

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

agalbachicar · 2022-10-10T14:55:12Z

This PR now depends on maliput/maliput#524

agalbachicar · 2022-10-11T16:26:17Z

I've just triggered CI again @francocipollone

francocipollone

LGTM! Nice job! This is a great addition!

francocipollone · 2022-10-11T16:13:14Z

src/builder/builder.cc

+  auto range_a_b = lane_ends_.equal_range(lane_end_a);
+  auto range_b_a = lane_ends_.equal_range(lane_end_b);


can't these be const?

Yes, good catch!

francocipollone · 2022-10-11T16:38:26Z

src/builder/builder.cc

+// When @p lane_ends appears on the A side of a BranchPointSets, the B side of such entity is updated.
+// When @p lane_ends appears on the B side of a BranchPointSets, the A side of such entity is updated.


Suggested change

// When @p lane_ends appears on the A side of a BranchPointSets, the B side of such entity is updated.

// When @p lane_ends appears on the B side of a BranchPointSets, the A side of such entity is updated.

// When @p lane_end appears on the A side of a BranchPointSets, the B side of such entity is updated.

// When @p lane_end appears on the B side of a BranchPointSets, the A side of such entity is updated.

Done. Changed a bit the entire docstring.

francocipollone · 2022-10-11T16:43:50Z

src/builder/builder.cc

+    return branch_point;
+  }
+
+  const std::unordered_map<maliput::api::LaneId, const maliput::geometry_base::Lane*> lanes_;


can't you receive a reference here instead of copying the unordered map?

Done! Thanks for catching it.

francocipollone · 2022-10-11T16:59:14Z

test/builder/builder_test.cc

+  ASSERT_TRUE(lane_end_is_in(lane_a->GetOngoingBranches(finish)->get(0), expected_lane_ends));
+  ASSERT_TRUE(lane_end_is_in(lane_a->GetOngoingBranches(finish)->get(1), expected_lane_ends));
+  ASSERT_TRUE(lane_end_is_in(lane_a->GetOngoingBranches(finish)->get(2), expected_lane_ends));


nit: This might hide one case:

when get(0), get(1) and get(2) returns the same value for example.

expected_lane_ends should be iterated and for each value, there should be one value in the lane_a->GetOngoingBranches(finish) set that matches.

You are right. I did not think of it. Let me address that in a follow up PR.

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

francocipollone

🥇

agalbachicar added 8 commits September 19, 2022 12:29

Creates base classes for the RoadGeometry

1b230d6

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Implements a basic Builder API.

d887fb7

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Updates documentation and tests.

adffb7b

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Updates the Builders to include LaneGeometry.

d312fff

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Solve review comments.

0887dff

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Fixes undesired changes.

acf4b23

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Solves review comments and improves tests.

bf7e07d

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Adds BranchPoint builder.

90ccc36

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

francocipollone requested changes Sep 26, 2022

View reviewed changes

agalbachicar added 2 commits September 29, 2022 15:57

Merge branch 'main' into agalbachicar/#10_builder_branch_points

c1bb630

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

Wraps up the implementation of the BranchPoint builder.

e6bca61

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

agalbachicar force-pushed the agalbachicar/#10_builder_branch_points branch from 8ee9c24 to e6bca61 Compare October 10, 2022 14:49

agalbachicar mentioned this pull request Oct 10, 2022

Adds a test function for LaneEnds. maliput/maliput#524

Merged

6 tasks

agalbachicar marked this pull request as ready for review October 10, 2022 14:55

agalbachicar requested a review from francocipollone October 10, 2022 14:58

Merge branch 'main' into agalbachicar/#10_builder_branch_points

3bc88f3

francocipollone previously approved these changes Oct 11, 2022

View reviewed changes

Addresses review comments.

f9abf7d

Signed-off-by: Agustin Alba Chicar <ag.albachicar@gmail.com>

agalbachicar dismissed francocipollone’s stale review via f9abf7d October 11, 2022 18:32

agalbachicar requested a review from francocipollone October 11, 2022 18:33

francocipollone approved these changes Oct 11, 2022

View reviewed changes

agalbachicar merged commit f6016d2 into main Oct 12, 2022

agalbachicar deleted the agalbachicar/#10_builder_branch_points branch October 12, 2022 07:17

francocipollone mentioned this pull request Oct 12, 2022

Matches with branchpoint builder implementation. maliput/maliput_osm#24

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builder for BranchPoints #24

Builder for BranchPoints #24

agalbachicar commented Sep 26, 2022

agalbachicar commented Sep 26, 2022

francocipollone left a comment

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

francocipollone Sep 26, 2022

agalbachicar Oct 10, 2022

agalbachicar commented Oct 10, 2022

agalbachicar commented Oct 11, 2022

francocipollone left a comment

francocipollone Oct 11, 2022

agalbachicar Oct 11, 2022

francocipollone Oct 11, 2022

agalbachicar Oct 11, 2022

francocipollone Oct 11, 2022

agalbachicar Oct 11, 2022

francocipollone Oct 11, 2022

agalbachicar Oct 11, 2022

francocipollone left a comment

	const LaneEnd lane_end_b = std::make_pair(lane_id_a, which_b);
	const LaneEnd lane_end_b = std::make_pair(lane_id_b, which_b);

	it_b_side->b_side.insert(a_side_set.begin(), a_side_set.end());
	it_b_side->b_side.insert(b_side_set.begin(), a_side_set.end());

		auto range_a_b = lane_ends_.equal_range(lane_end_a);
		auto range_b_a = lane_ends_.equal_range(lane_end_b);

		// When @p lane_ends appears on the A side of a BranchPointSets, the B side of such entity is updated.
		// When @p lane_ends appears on the B side of a BranchPointSets, the A side of such entity is updated.

Builder for BranchPoints #24

Builder for BranchPoints #24

Conversation

agalbachicar commented Sep 26, 2022

🎉 New feature

Summary

Test it

Checklist

agalbachicar commented Sep 26, 2022

francocipollone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agalbachicar commented Oct 10, 2022

agalbachicar commented Oct 11, 2022

francocipollone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francocipollone left a comment

Choose a reason for hiding this comment