Route part two

[agalbachicar]

🎉 New feature

Part of #543
Goes on top of #554

Implements Route::LaneSRelationFor()

Summary

Implements and tests Route::LaneSRelationFor(). Same route as in the maliput::routing::LaneSRelation definition has been used. Also, added a new definition for coincident maliput::api::LaneSRanges which was missing to complete the space of maliput::routing::LaneSRelations.

Test it

Via unit tests.

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.

---

This change is 

[codecov]

Codecov Report

Merging #555 (a80c2ff) into main (4174aee) will increase coverage by 0.28%.
The diff coverage is 88.46%.

❗ Current head a80c2ff differs from pull request most recent head 762c26c. Consider uploading reports for the commit 762c26c to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
+ Coverage   44.08%   44.37%   +0.28%     
==========================================
  Files         164      165       +1     
  Lines        5423     5480      +57     
  Branches     2890     2912      +22     
==========================================
+ Hits         2391     2432      +41     
- Misses       1530     1537       +7     
- Partials     1502     1511       +9     

Files Changed	Coverage Δ
include/maliput/routing/lane_s_range_relation.h	0.00% <ø> (ø)
include/maliput/routing/phase.h	0.00% <ø> (ø)
include/maliput/routing/route.h	50.00% <ø> (ø)
src/routing/route.cc	85.89% <88.46%> (+5.12%)	⬆️

... and 1 file with indirect coverage changes

[agalbachicar]

Ready for reviewing @liangfok and @francocipollone

[liangfok]

Checkpoint (time boxed).

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 9 unresolved discussions (waiting on @agalbachicar and @francocipollone)

---

-- commits line 3 at r1:
BTW, consider mentioning the addition of the "coincident" relation.

---

include/maliput/routing/lane_s_range_relation.h line 36 at r1 (raw file):

/// Defines the possible relationships the routing API can interpret between
/// two api::LaneSRanges within a Route.
/// Relations described in this enum must be mutually exclusive. The following

Question that occurred to me while reading this again: Since relations must be mutually exclusive, does this mean that 'A' is not kLeft of 'B' since it is kAdjacentLeft of 'B'? In other words, that the mutually-exclusive invariant mean that at most one relation will apply to any given pair of LaneSRange objects?

---

include/maliput/routing/route.h line 195 at r1 (raw file):

  /// @return The LaneSRangeRelation between @p lane_s_range_b with respect to
  /// @p lane_s_range_a.
  LaneSRangeRelation LaneSRangeRelationFor(const api::LaneSRange& lane_s_range_a,

I just noticed that this method's name is inconsistent with the other methods in this class. In particular, this method does not start with a verb and ends with "For". For consistency, this method should be named FindLaneSRangeRelation, ComputeLaneSRangeRelation, or GetLaneSRangeRelation.

---

include/maliput/routing/route.h line 220 at r1 (raw file):

  // std::pair::first indexes the RoutePhase.
  // std::pair::second indexes the api::LaneSRange in the RoutePhase.
  using LaneSRangeIndex = std::pair<size_t, size_t>;

Should #include <utility> for std::pair.
https://google.github.io/styleguide/cppguide.html#Include_What_You_Use

---

include/maliput/routing/route.h line 230 at r1 (raw file):

  // @return An optional containing the index of the api::LaneSRange within this
  // Route.
  std::optional<LaneSRangeIndex> FindLaneSRangeIndexFor(const api::LaneSRange& lane_s_range) const;

Should #include "maliput/api/regions.h" for api::LaneSRange.

(I see this is a pre-existing condition, but might as well fix it now.)

---

include/maliput/routing/route.h line 230 at r1 (raw file):

  // @return An optional containing the index of the api::LaneSRange within this
  // Route.
  std::optional<LaneSRangeIndex> FindLaneSRangeIndexFor(const api::LaneSRange& lane_s_range) const;

To be consistent with the other methods in this class, should delete "For" in the method name. Ditto regarding methods in the .cc file.

---

src/routing/route.cc line 133 at r1 (raw file):

  }

  // lane_s_range_b is unrelated to lane_s_range_a

BTW, consider explaining why:

// lane_s_range_b is unrelated to lane_s_range_a since they are in non-adjacent phases.

---

src/routing/route.cc line 142 at r1 (raw file):

  const size_t lane_s_range_b_previous_index = lane_s_range_b_index->second - 1;

  // When both indices are the same, we should look into the indices for the lane_s_ranges.

BTW, consider adding "phase" after "both".

---

src/routing/route.cc line 147 at r1 (raw file):

      return LaneSRangeRelation::kCoincident;
    } else if (lane_s_range_a_index->second == lane_s_range_b_next_index) {
      return LaneSRangeRelation::kAdjacentRight;

Does this assume the LaneSRange values in a Phase are sorted left-to-right? The docstring for Phase says it contains a "set" of LaneSRanges, which is typically not sorted.

I see Phase::lane_s_ranges() returns a vector, which is ordered, but I'm not finding documentation indicating whether the ordered values are sorted.

[agalbachicar]

Answered to all the comments. Looking forward to the termination of the review, take your time!

Reviewable status: 1 of 5 files reviewed, 7 unresolved discussions (waiting on @francocipollone and @liangfok)

---

-- commits line 3 at r1:

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider mentioning the addition of the "coincident" relation.

Leaving unresolved so I don't forget when squashing commits.

---

include/maliput/routing/lane_s_range_relation.h line 36 at r1 (raw file):
Because the lateral space partition has a notion of adjacency as it is described here, the answer is no. That does not mean the general sense of what it is to the left side includes from the contact interface to the opposing extent.

So, let me try to add some ascii art here to make an example:

|  ^   |  ^  |  ^  |  ^  |
|  ^   |  ^  |  ^  |  ^  |
|  ^   |  ^  |  ^  |  ^  |
   A      B     C     D

Then,

• In general terms, {A,B,C} are to the left of D.
• The relation each one have with D is:
  • A is kLeft D
  • B is kLeft D
  • C is kAdjacentLeft D

does this mean that 'A' is not kLeft of 'B' since it is kAdjacentLeft of 'B'?

Correct. In the ascii art in the code, 'A' is not kLeft of 'B' since it is kAdjacentLeft of 'B'

In other words, that the mutually-exclusive invariant mean that at most one relation will apply to any given pair of LaneSRange objects?

I don't think that is quite right. I think the mutually-exclusive invariant mean that only one relation applies for a given pair of api::LaneSRanges.

As discussed in previous PRs after the termination of the implementation together with some integration tests we can revisit the API to polish it. I've already seen this in code that ends up being the same for kLeft and kAdjacentLeft, and the same for the right counterparts. But I still hold the case for the consumers which may prefer to act differently if they need to incur into a 1 lane change or 2+ lane change.

Let me know if you think we need to make something different.

---

include/maliput/routing/route.h line 195 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I just noticed that this method's name is inconsistent with the other methods in this class. In particular, this method does not start with a verb and ends with "For". For consistency, this method should be named FindLaneSRangeRelation, ComputeLaneSRangeRelation, or GetLaneSRangeRelation.

Done.

---

include/maliput/routing/route.h line 220 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should #include <utility> for std::pair.
https://google.github.io/styleguide/cppguide.html#Include_What_You_Use

Sorry for that. Done.

---

include/maliput/routing/route.h line 230 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should #include "maliput/api/regions.h" for api::LaneSRange.

(I see this is a pre-existing condition, but might as well fix it now.)

Done.

---

include/maliput/routing/route.h line 230 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

To be consistent with the other methods in this class, should delete "For" in the method name. Ditto regarding methods in the .cc file.

Done.

---

src/routing/route.cc line 142 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding "phase" after "both".

Done.

---

src/routing/route.cc line 147 at r1 (raw file):

Does this assume the LaneSRange values in a Phase are sorted left-to-right?

I'm using the same order as the const Lane* api::Segment::lane(int index) const. This order is right-to-left (increasing index as you move to the left, i.e. +r direction).

I see Phase::lane_s_ranges() returns a vector, which is ordered, but I'm not finding documentation indicating whether the ordered values are sorted.

I changed the docstring, validations are already in place for that condition ( https://github.com/maliput/maliput/blob/main/src/routing/phase.cc#L60-L67 ).

Let me know if you think we need to change something else.

[liangfok]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @agalbachicar and @francocipollone)

---

include/maliput/routing/lane_s_range_relation.h line 36 at r1 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

Because the lateral space partition has a notion of adjacency as it is described here, the answer is no. That does not mean the general sense of what it is to the left side includes from the contact interface to the opposing extent.

So, let me try to add some ascii art here to make an example:

|  ^   |  ^  |  ^  |  ^  |
|  ^   |  ^  |  ^  |  ^  |
|  ^   |  ^  |  ^  |  ^  |
   A      B     C     D

Then,

• In general terms, {A,B,C} are to the left of D.
• The relation each one have with D is:
  • A is kLeft D
  • B is kLeft D
  • C is kAdjacentLeft D

does this mean that 'A' is not kLeft of 'B' since it is kAdjacentLeft of 'B'?

Correct. In the ascii art in the code, 'A' is not kLeft of 'B' since it is kAdjacentLeft of 'B'

In other words, that the mutually-exclusive invariant mean that at most one relation will apply to any given pair of LaneSRange objects?

I don't think that is quite right. I think the mutually-exclusive invariant mean that only one relation applies for a given pair of api::LaneSRanges.

As discussed in previous PRs after the termination of the implementation together with some integration tests we can revisit the API to polish it. I've already seen this in code that ends up being the same for kLeft and kAdjacentLeft, and the same for the right counterparts. But I still hold the case for the consumers which may prefer to act differently if they need to incur into a 1 lane change or 2+ lane change.

Let me know if you think we need to make something different.

Got it, thanks. Consider adding a clarification as follows:

/// Relations described in this enum must be mutually exclusive and comprehensive, i.e.,
/// exactly one relation will apply for any given pair of `LaneSRange` objects. 

I currently think we can simplify the relationships by removing the "adjacent" variations of the left / right relationships. I believe what's important is whether one LaneSRange is to the left or to the right of another LaneSRange. The degree to which it is left-of or right-of can be obtained via a separate query if the router is interested, and be even more expressive. For example, consumers may also want to distinguish between a 2-lane-change versus 3-lane-change, etc.

With adjacent-left and adjacent-right being present, one may wonder why there isn't a similar relation for the longitudinal direction, like kAdjacentSucceedingLeft, etc.

[liangfok]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @francocipollone)