RNDF Spline lane implementation

[agalbachicar]

This PR removes the stub implementation of drake::maliput::rndf::SplineLane and provides a meaningful one. It includes also a set of unit tests to validate the API.

This PR concludes the implementation of the Maliput API for RNDF.

---

This change is 

[agalbachicar]

CC: @basicNew @hidmic to track progress of this PR.

[agalbachicar]

+@liangfok for feature review please.

[liangfok]

Checkpoint.

---

Reviewed 5 of 10 files at r1.
Review status: 5 of 10 files reviewed at latest revision, 26 unresolved discussions, some commit checks failed.

---

drake/automotive/maliput/rndf/segment.h, line 36 at r1 (raw file):

  ///
  /// @param id is the id of the lane.
  /// @param control_points is a vector of tuples that hold the point (first

"is a" should be "A".

---

drake/automotive/maliput/rndf/spline_lane.h, line 48 at r1 (raw file):

/// Specialization of drake::maliput::rndf::Lane with a spline curve as its
/// reference path. RNDF specification lacks of the elevation and

Delete "of the".

---

drake/automotive/maliput/rndf/spline_lane.h, line 59 at r1 (raw file):

  ///
  /// For @p id, @p segment, @p width and @p index please see the documentation
  /// of the Lane base class.

Doxygen requires that either all or none of the parameters are documented. Partial documentation results in a Doxygen warning like the following:

/home/liang/dev/drake-distro-1/drake/automotive/maliput/rndf/spline_lane.h:70: warning: The following parameters of drake::maliput::rndf::SplineLane::SplineLane(const api::LaneId &id, const api::Segment *segment, const std::vector< std::tuple< ignition::math::Vector3d, ignition::math::Vector3d >> &control_points, double width, int index) are not documented:
  parameter 'id'
  parameter 'segment'
  parameter 'width'
  parameter 'index'

After generating the Doxygen website, the warnings are stored in drake-distro-1/build/drake/doc/doxygen_cxx.log.

---

drake/automotive/maliput/rndf/spline_lane.h, line 61 at r1 (raw file):

  /// of the Lane base class.
  ///
  /// @param control_points are a collection of knots and the knot tangent value

"the knot tangent value" should be "their tangents"

---

drake/automotive/maliput/rndf/spline_lane.h, line 61 at r1 (raw file):

  /// of the Lane base class.
  ///
  /// @param control_points are a collection of knots and the knot tangent value

"are a" should be "A" since this section is rendered as a table.

---

drake/automotive/maliput/rndf/spline_lane.h, line 78 at r1 (raw file):

  ~SplineLane() override = default;

  /// It computes the length of a SplineLane based on the @p points set as

"It computes" should be "Computes".

---

drake/automotive/maliput/rndf/spline_lane.h, line 81 at r1 (raw file):

  /// control points. The first value are the points and the second is the
  /// value at that point.
  /// @param control_points are a collection of points and the tangent value

"are a" should be "A".

---

drake/automotive/maliput/rndf/spline_lane.h, line 82 at r1 (raw file):

  /// value at that point.
  /// @param control_points are a collection of points and the tangent value
  /// where the interpolated curve will pass. They must be at least two.

"They must be at least two." should be "There must be at least two control points."

---

drake/automotive/maliput/rndf/spline_lane.h, line 82 at r1 (raw file):

  /// value at that point.
  /// @param control_points are a collection of points and the tangent value
  /// where the interpolated curve will pass. They must be at least two.

BTW, consider documenting what happens if less than two control points are provided. Is an exception thrown? Does the program abort?

---

drake/automotive/maliput/rndf/spline_lane.h, line 86 at r1 (raw file):

  static double ComputeLength(
      const std::vector<std::tuple<ignition::math::Vector3d,
                                   ignition::math::Vector3d>>& points);

"points" should be "control_points" to match the Doxygen comment.

---

drake/automotive/maliput/rndf/spline_lane.h, line 89 at r1 (raw file):

  /// @return the tension of the curve, a bounded constant value between 0 to
  /// 1 which is a measure of the curvature of the interpolated spline. Given a

The phrase "measure of the curvature" indicates to me that this is a value that is determined based on the properties of a particular spline instance. However, the returned value is a constant. Should "desired" be inserted before "curvature"?

---

drake/automotive/maliput/rndf/spline_lane.h, line 94 at r1 (raw file):

  static double Tension() { return kTension; }

  /// @return the error bound that the arc length interpolator will

"arc" should be "path".

---

drake/automotive/maliput/rndf/spline_lane.h, line 96 at r1 (raw file):

  /// @return the error bound that the arc length interpolator will
  /// attempt to attain when approximating the inverse function that maps
  /// the s coordinate of s,r,h frame into the t parameter that

BTW, consider replacing "s,r,h frame" with "api::LanePosition" since that'll be more specific.

---

drake/automotive/maliput/rndf/spline_lane.h, line 101 at r1 (raw file):

 private:
  // This function is not implemented and will abort if called.

BTW, consider adding a TODO comment indicating that this method needs to be implemented.

---

drake/automotive/maliput/rndf/spline_lane.h, line 106 at r1 (raw file):

                                     double* distance) const override;

  // Provides the api::GeoPosition given the @p lane_pos over the lane.

BTW, consider replacing "given" with "at".

---

drake/automotive/maliput/rndf/spline_lane.h, line 113 at r1 (raw file):

      const api::LanePosition& lane_pos) const override;

  // Provides the api::Rotation matrix against the origin of th world frame at

BTW, typo: "th".

---

drake/automotive/maliput/rndf/spline_lane.h, line 114 at r1 (raw file):

  // Provides the api::Rotation matrix against the origin of th world frame at
  // @p lane_pos coordinate over the lane. s coordinate of @p lane_pose will be

Add "The" before "s coordinate".

---

drake/automotive/maliput/rndf/spline_lane.h, line 115 at r1 (raw file):

  // Provides the api::Rotation matrix against the origin of th world frame at
  // @p lane_pos coordinate over the lane. s coordinate of @p lane_pose will be
  // saturated to be processed in the range of 0.0 to the lane length.

Delete "processed".

---

drake/automotive/maliput/rndf/spline_lane.h, line 120 at r1 (raw file):

  // As we are moving along the lane with path length coordinates we will
  // return the velocity as is and will not scale through the chain rule.

I'm not sure what "scale through the chain rule" means in this context. How does this impact the returned value's accuracy?

---

drake/automotive/maliput/rndf/spline_lane.h, line 125 at r1 (raw file):

      const api::IsoLaneVelocity& velocity) const override;

  // It will provide the x,y coordinates based on the

"It will provide" should be "Provides".

(Same comment below.)

---

drake/automotive/maliput/rndf/spline_lane.h, line 126 at r1 (raw file):

  // It will provide the x,y coordinates based on the
  // ArcLengthParameterizedSpline that will provide the interpolation

"will provide" should be "provides"

(Same comment below.)

---

drake/automotive/maliput/rndf/spline_lane.h, line 127 at r1 (raw file):

  // It will provide the x,y coordinates based on the
  // ArcLengthParameterizedSpline that will provide the interpolation
  // image at a arc length s from the beginning of the SplineLane.

"a" should be "an".

(Same comment below.)

---

drake/automotive/maliput/rndf/spline_lane.h, line 127 at r1 (raw file):

  // It will provide the x,y coordinates based on the
  // ArcLengthParameterizedSpline that will provide the interpolation
  // image at a arc length s from the beginning of the SplineLane.

"arc" should be "path".

(Same comment below.)

---

drake/automotive/maliput/rndf/spline_lane.h, line 139 at r1 (raw file):

  double heading_of_s(double s) const;

  // It will provide the derivative of the angle evaluates at at an arc length

Delete "evaluates at".

---

drake/automotive/maliput/rndf/spline_lane.h, line 174 at r1 (raw file):

  // line approximation. Based on the intersection points with the lanes set as
  // extents of the segment (the first and last ones) we can determine the
  // distance to those limits and create the api::RBounds set as results.

"results" should be "result".

---

drake/automotive/maliput/rndf/spline_lane.h, line 186 at r1 (raw file):

  ignition::math::Vector3d GetPositionToLane(double s, int lane_id) const;

  // Pointer to the spline reference curve.

This comment can be deleted since the code appears to be self documenting.

---

Comments from Reviewable

[agalbachicar]

Review status: 3 of 10 files reviewed at latest revision, 26 unresolved discussions.

---

drake/automotive/maliput/rndf/segment.h, line 36 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"is a" should be "A".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 48 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Delete "of the".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 59 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Doxygen requires that either all or none of the parameters are documented. Partial documentation results in a Doxygen warning like the following:

/home/liang/dev/drake-distro-1/drake/automotive/maliput/rndf/spline_lane.h:70: warning: The following parameters of drake::maliput::rndf::SplineLane::SplineLane(const api::LaneId &id, const api::Segment *segment, const std::vector< std::tuple< ignition::math::Vector3d, ignition::math::Vector3d >> &control_points, double width, int index) are not documented:
  parameter 'id'
  parameter 'segment'
  parameter 'width'
  parameter 'index'

After generating the Doxygen website, the warnings are stored in drake-distro-1/build/drake/doc/doxygen_cxx.log.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 61 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"the knot tangent value" should be "their tangents"

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 61 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"are a" should be "A" since this section is rendered as a table.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 78 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"It computes" should be "Computes".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 81 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"are a" should be "A".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 82 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"They must be at least two." should be "There must be at least two control points."

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 82 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider documenting what happens if less than two control points are provided. Is an exception thrown? Does the program abort?

Done. Added also the throw to the class constructor.

---

drake/automotive/maliput/rndf/spline_lane.h, line 86 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"points" should be "control_points" to match the Doxygen comment.

Done. Copied the comment from the class constructor as it's the same.

---

drake/automotive/maliput/rndf/spline_lane.h, line 89 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

The phrase "measure of the curvature" indicates to me that this is a value that is determined based on the properties of a particular spline instance. However, the returned value is a constant. Should "desired" be inserted before "curvature"?

Done. Thanks for the suggestion.

---

drake/automotive/maliput/rndf/spline_lane.h, line 94 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"arc" should be "path".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 96 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "s,r,h frame" with "api::LanePosition" since that'll be more specific.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 101 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding a TODO comment indicating that this method needs to be implemented.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 106 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "given" with "at".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 113 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, typo: "th".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 114 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Add "The" before "s coordinate".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 115 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Delete "processed".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 120 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I'm not sure what "scale through the chain rule" means in this context. How does this impact the returned value's accuracy?

I wanted to emphasize that api::LanePosition is depicted as non-isotropic in the Maliput design document, however on the current implementation it is and no scale change should be done to the velocity.

---

drake/automotive/maliput/rndf/spline_lane.h, line 125 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"It will provide" should be "Provides".

(Same comment below.)

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 126 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"will provide" should be "provides"

(Same comment below.)

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 127 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"a" should be "an".

(Same comment below.)

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 127 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"arc" should be "path".

(Same comment below.)

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 139 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Delete "evaluates at".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 174 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"results" should be "result".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 186 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

This comment can be deleted since the code appears to be self documenting.

Done.

---

Comments from Reviewable

[liangfok]

Checkpoint.

---

Reviewed 1 of 10 files at r1, 1 of 6 files at r2, 1 of 1 files at r3.
Review status: 6 of 10 files reviewed at latest revision, 14 unresolved discussions.

---

drake/automotive/maliput/rndf/segment.h, line 28 at r3 (raw file):

  /// Constructs a new Segment.
  /// @param id to name this segment

"to name this segment" should be "This segment's ID."

---

drake/automotive/maliput/rndf/segment.h, line 29 at r3 (raw file):

must remain valid for the lifetime of this class.

should be:

The api::Junction that contains this Segment. It must remain valid for the lifetime of this class.

---

drake/automotive/maliput/rndf/segment.h, line 35 at r3 (raw file):

is the id of the lane.

should be:

The lane's ID.

---

drake/automotive/maliput/rndf/segment.h, line 39 at r3 (raw file):

  /// element) and the tangent (second element) at that point to construct the
  /// spline based lane. The size should be at least two pairs.
  /// @param width is the width specified by the RNDF lane_width

"is the" should be "The".

---

drake/automotive/maliput/rndf/segment.h, line 40 at r3 (raw file):

  /// spline based lane. The size should be at least two pairs.
  /// @param width is the width specified by the RNDF lane_width
  /// parameter, or the default assigned value by this code. Later, this value

What is "the default assigned value by this code"?

---

drake/automotive/maliput/rndf/segment.h, line 43 at r3 (raw file):

  /// will be used to construct the api::Lane::lane_bounds() and the
  /// api::Lane::driveable_bounds() result.
  /// @return a pointer to a valid SplineLane.

BTW, consider adding "that was added to this Segment" after "SplineLane".

---

drake/automotive/maliput/rndf/segment.h, line 44 at r3 (raw file):

  /// api::Lane::driveable_bounds() result.
  /// @return a pointer to a valid SplineLane.
  /// @throws when @p control_points' size is less than 2.

Missing the exception type after "@throws".

https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdthrows

---

drake/automotive/maliput/rndf/spline_lane.h, line 120 at r1 (raw file):
Would adding the following sentence make sense?

This is because the returned api::LanePosition is isotropic, i.e., it contains real physical values.

---

drake/automotive/maliput/rndf/spline_lane.h, line 59 at r3 (raw file):
BTW, consider rewording:

A pointer to the segment that contains this lane, which must remain valid for the lifetime of this class.

---

drake/automotive/maliput/rndf/spline_lane.h, line 60 at r3 (raw file):

  /// @param segment A pointer that refers to its parent, which must remain
  /// valid for the lifetime of this class.
  /// @param width The value of the width of the lane based on the RNDF

BTW, consider deleting "value of the".

---

drake/automotive/maliput/rndf/spline_lane.h, line 65 at r3 (raw file):

  /// @param index The index that can be used to reference this Lane from
  /// api::Segment::lane() call.
  /// @param control_points A collection of knots and their tangents where

Can the order of the parameters in this section be made to match the order of the parameters in the method's signature?

---

drake/automotive/maliput/rndf/spline_lane.h, line 84 at r3 (raw file):
"points" should be "control_points". Consider rewording this sentence as follows:

Computes the length of a SplineLane created using @p control_points.

---

drake/automotive/maliput/rndf/spline_lane.h, line 90 at r3 (raw file):
Consider rewording for clarity:

The tuple's first value is the knot point while its second value is the tangent at the knot point.

---

drake/automotive/maliput/rndf/spline_lane.h, line 149 at r3 (raw file):

  double heading_of_s(double s) const;

  // Provides the derivative of the angle at a path length s distance from the

BTW, consider deleting "distance" since it's implied by "path length".

---

Comments from Reviewable

[liangfok]

Checkpoint.

---

Reviewed 2 of 6 files at r2.
Review status: 8 of 10 files reviewed at latest revision, 28 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 13 at r3 (raw file):

namespace rndf {

// This constant is set to provide the maximum tolerable error to the

BTW, consider replacing "is set to provide" with "specifies".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 13 at r3 (raw file):

namespace rndf {

// This constant is set to provide the maximum tolerable error to the

BTW, consider deleting "the" before "ArcLengthParamterizedSpline".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 14 at r3 (raw file):

use it,

should be:

be used

---

drake/automotive/maliput/rndf/spline_lane.cc, line 15 at r3 (raw file):

// This constant is set to provide the maximum tolerable error to the
// ArcLengthParameterizedSpline. It will use it, to provide a close
// approximation of the path length s coordinate to the spline's parameter.

BTW, "the spline's parameter" may be confusing. Consider explicitly stating which parameters.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 17 at r3 (raw file):

// approximation of the path length s coordinate to the spline's parameter.
const double SplineLane::kSplineErrorBound = 1e-6;
// Used to provide a nice visualization as for the general dimensions involved.

What are "the general dimensions involved"?

---

drake/automotive/maliput/rndf/spline_lane.cc, line 17 at r3 (raw file):

// approximation of the path length s coordinate to the spline's parameter.
const double SplineLane::kSplineErrorBound = 1e-6;
// Used to provide a nice visualization as for the general dimensions involved.

delete "as".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 27 at r3 (raw file):

    : Lane(id, segment, width, index) {
  DRAKE_THROW_UNLESS(control_points.size() >= 2);
  // It first creates a spline based on the positions and its tangents.

BTW, consider replacing "It first creates" with "Creates".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 27 at r3 (raw file):

    : Lane(id, segment, width, index) {
  DRAKE_THROW_UNLESS(control_points.size() >= 2);
  // It first creates a spline based on the positions and its tangents.

"its" should be "their".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 28 at r3 (raw file):

  DRAKE_THROW_UNLESS(control_points.size() >= 2);
  // It first creates a spline based on the positions and its tangents.
  std::unique_ptr<ignition::math::Spline> spline =

BTW, "std::unique_ptrignition::math::Spline" can be replaced by "auto" since the type is clear.

https://google.github.io/styleguide/cppguide.html#auto

---

drake/automotive/maliput/rndf/spline_lane.cc, line 37 at r3 (raw file):
BTW, consider replacing the above comment with:

Wraps the ignition::math::Spline within an ArcLengthParamterizedSpline for compatibility with Maliput's s parameter.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 128 at r3 (raw file):
I just realized that width_ is an inherited protected member variable that violates the style guide (sorry I missed this in previous reviews):

Make data members private, unless they are static const (and follow the naming convention for constants).

https://google.github.io/styleguide/cppguide.html#Access_Control

drake/automotive/maliput/rndf/lane.h contains several additional protected member variables that should be private.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 135 at r3 (raw file):

    return api::RBounds(-width_ / 2., width_ / 2.);
  }
  // Get the position to the first lane

Missing period at end of sentence.

---

drake/automotive/maliput/rndf/test/segment_test.cc, line 3 at r3 (raw file):

#include "drake/automotive/maliput/rndf/segment.h"

#include <string>

BTW, unused?

---

drake/automotive/maliput/rndf/test/spline_helpers_test.cc, line 11 at r3 (raw file):
The should remain at the top of the file per style guide since it classifies as being a "related header":

Use standard order for readability and to avoid hidden dependencies: Related header, C library, C++ library, other libraries' .h, your project's .h.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

---

Comments from Reviewable

[liangfok]

Checkpoint.

---

Reviewed 3 of 9 files at r4.
Review status: 7 of 13 files reviewed at latest revision, 47 unresolved discussions, some commit checks failed.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 150 at r3 (raw file):

ignition::math::Vector3d SplineLane::GetPositionToLane(double s,
                                                       int lane_id) const {
  // This the geo position in the current lane for a LanePosition(s, 0., 0.).

"This" should be "Computes".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 150 at r3 (raw file):

ignition::math::Vector3d SplineLane::GetPositionToLane(double s,
                                                       int lane_id) const {
  // This the geo position in the current lane for a LanePosition(s, 0., 0.).

"for a" should be "at".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 151 at r3 (raw file):
What does "g_l_1" stand for?

Names should be descriptive; avoid abbreviation.

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

(Same for the other similarly-named variables below like g_l_0_a, g_l_0_b, etc.)

---

drake/automotive/maliput/rndf/spline_lane.cc, line 154 at r3 (raw file):

      spline_->InterpolateMthDerivative(0, s);
  // After continue working out the expression, if lane_id points to myself,
  // I just need to return my position.

Consider deleting "After continue working out the expression".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 161 at r3 (raw file):

  ignition::math::Vector3d t_1 = spline_->InterpolateMthDerivative(1, s);
  t_1.Normalize();
  // And the normal.

BTW, consider documenting that (1) a right-handed coordinate frame is used and (2) this is a 90 degree rotation about the +Z axis.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 163 at r3 (raw file):

  // And the normal.
  const ignition::math::Vector3d n_1(-t_1.Y(), t_1.X(), 0.);
  // Here we get the beginning and ending of the other lane, and then compute

BTW, consider replacing "Here we get" with "Gets".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 163 at r3 (raw file):

  // And the normal.
  const ignition::math::Vector3d n_1(-t_1.Y(), t_1.X(), 0.);
  // Here we get the beginning and ending of the other lane, and then compute

BTW, consider replacing "compute" with "computes".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 165 at r3 (raw file):

  // Here we get the beginning and ending of the other lane, and then compute
  // the respective GeoPositions.
  const api::Lane* other_lane = segment_->lane(lane_id);

BTW, consider adding:

DRAKE_DEMAND(other_lane != nullptr);

---

drake/automotive/maliput/rndf/spline_lane.cc, line 170 at r3 (raw file):
Consider replacing this comment and the one below with:

Converts the beginning and ending positions of the other lane into ignition::math::Vector3d objects.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 179 at r3 (raw file):

  ignition::math::Vector3d t_0 = (g_l_0_b - g_l_0_a);
  t_0.Normalize();
  // We get here the intersection point between n_1 and the other_lane's line

BTW, consider replacing "We get here" with "Computes".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 180 at r3 (raw file):

  t_0.Normalize();
  // We get here the intersection point between n_1 and the other_lane's line
  // approximation

Missing period.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

  // approximation
  return g_l_1 +
         n_1 * (t_0.Cross(g_l_1 - g_l_0_a).Length() / t_0.Cross(n_1).Length());

BTW, for my own enlightenment, does the following post describe the math that's implemented here?

https://stackoverflow.com/a/565282

---

drake/automotive/maliput/rndf/spline_lane.cc, line 187 at r3 (raw file):

double SplineLane::ComputeLength(
    const std::vector<std::tuple<ignition::math::Vector3d,
                                 ignition::math::Vector3d>>& points) {

For consistency, "points" should be "control_points".

---

drake/automotive/maliput/rndf/spline_lane.h, line 190 at r3 (raw file):

  // Computes the intersection point of the normal line that intersects the
  // current lane at s coordinate (r = 0, h = 0) with the lane whose index is

BTW, consider replacing "s coordinate" with "@p s".

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

  // Computes the intersection point of the normal line that intersects the
  // current lane at s coordinate (r = 0, h = 0) with the lane whose index is
  // lane_id. The function of lane_id is approximated to a line defined by the

"to" should be "by".

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

  // Computes the intersection point of the normal line that intersects the
  // current lane at s coordinate (r = 0, h = 0) with the lane whose index is
  // lane_id. The function of lane_id is approximated to a line defined by the

Consider documenting the fact that this method supports @p lane_id equal to the current lane and will simply return the geo position at (s, 0, 0) of the current lane.

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

  // Computes the intersection point of the normal line that intersects the
  // current lane at s coordinate (r = 0, h = 0) with the lane whose index is
  // lane_id. The function of lane_id is approximated to a line defined by the

Consider adding "the lane with ID @p" after "function of".

---

drake/automotive/maliput/rndf/spline_lane.h, line 192 at r3 (raw file):

  // current lane at s coordinate (r = 0, h = 0) with the lane whose index is
  // lane_id. The function of lane_id is approximated to a line defined by the
  // api::GeoPosition values of both the beginning and ending of it.

BTW, this approximation may have high error if the lane with ID lane_id is U-shaped right? How is this scenario handled or avoided?

---

externals/bullet, line 1 at r4 (raw file):

Subproject commit ae2c4ca0618d55c6a29900aed75b958604149fdb

BTW, why this change? (I briefly searched and couldn't immediately find why this is necessary.)

---

Comments from Reviewable

[agalbachicar]

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

my own enlightenment, does the following post describe the math that's implemented here?
Yes, but it wasn't taken from there. Should I adapt it to match the post and add the links as a reference?

---

Comments from Reviewable

[liangfok]

Review status: 7 of 13 files reviewed at latest revision, 47 unresolved discussions, some commit checks failed.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

my own enlightenment, does the following post describe the math that's implemented here?
Yes, but it wasn't taken from there. Should I adapt it to match the post and add the links as a reference?

Yes, thanks, because when I first encountered this code, its correctness wasn't immediately obvious to me. Having a citation to the derivation helps. Alternatively, is there a standard name for this method for determining where two line segments intersect?

---

Comments from Reviewable

[agalbachicar]

Comments have been addressed. PTAL.

---

Review status: 5 of 12 files reviewed at latest revision, 47 unresolved discussions.

---

drake/automotive/maliput/rndf/segment.h, line 28 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"to name this segment" should be "This segment's ID."

Done.

---

drake/automotive/maliput/rndf/segment.h, line 29 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

must remain valid for the lifetime of this class.

should be:

The api::Junction that contains this Segment. It must remain valid for the lifetime of this class.

Done.

---

drake/automotive/maliput/rndf/segment.h, line 35 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

is the id of the lane.

should be:

The lane's ID.

Done.

---

drake/automotive/maliput/rndf/segment.h, line 39 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"is the" should be "The".

Done.

---

drake/automotive/maliput/rndf/segment.h, line 40 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

What is "the default assigned value by this code"?

Done. Yes, it is wrong. RNDF specification allows to have lanes without a specification of the width of the road, so the parser or who uses it should assign a default value to it if necessary. The builder code (not pull requested yet) does so. I wrote the docs having that in mind.

---

drake/automotive/maliput/rndf/segment.h, line 43 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding "that was added to this Segment" after "SplineLane".

Done.

---

drake/automotive/maliput/rndf/segment.h, line 44 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing the exception type after "@throws".

https://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdthrows

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 13 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "is set to provide" with "specifies".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 13 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider deleting "the" before "ArcLengthParamterizedSpline".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 14 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

use it,

should be:

be used

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 15 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, "the spline's parameter" may be confusing. Consider explicitly stating which parameters.

Done. I've added "t" parameter as it is the only path (non-isotropic) parameter that ignition spline provides.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 17 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

What are "the general dimensions involved"?

Done. Sorry for this, but tension parameter was recently removed as it is no longer necessary (future PR do not longer use it). So it's no sense keeping that constant. BTW, that comment referred to the roads in a city, as they should avoid having cusps and / or loops, having a value of tension near to 1.0 is needed.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 17 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

delete "as".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 27 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "It first creates" with "Creates".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 27 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"its" should be "their".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 28 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, "std::unique_ptrignition::math::Spline" can be replaced by "auto" since the type is clear.

https://google.github.io/styleguide/cppguide.html#auto

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 37 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing the above comment with:

Wraps the ignition::math::Spline within an ArcLengthParamterizedSpline for compatibility with Maliput's s parameter.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 128 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

I just realized that width_ is an inherited protected member variable that violates the style guide (sorry I missed this in previous reviews):

Make data members private, unless they are static const (and follow the naming convention for constants).

https://google.github.io/styleguide/cppguide.html#Access_Control

drake/automotive/maliput/rndf/lane.h contains several additional protected member variables that should be private.

Done. No problem. Good to know it.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 135 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period at end of sentence.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 150 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"This" should be "Computes".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 150 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"for a" should be "at".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 151 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

What does "g_l_1" stand for?

Names should be descriptive; avoid abbreviation.

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

(Same for the other similarly-named variables below like g_l_0_a, g_l_0_b, etc.)

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 154 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider deleting "After continue working out the expression".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 161 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider documenting that (1) a right-handed coordinate frame is used and (2) this is a 90 degree rotation about the +Z axis.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 163 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "Here we get" with "Gets".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 163 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "compute" with "computes".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 165 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding:

DRAKE_DEMAND(other_lane != nullptr);

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 170 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider replacing this comment and the one below with:

Converts the beginning and ending positions of the other lane into ignition::math::Vector3d objects.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 179 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "We get here" with "Computes".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 180 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Yes, thanks, because when I first encountered this code, its correctness wasn't immediately obvious to me. Having a citation to the derivation helps. Alternatively, is there a standard name for this method for determining where two line segments intersect?

Done. AFAIK it's just line intersection in 3-D space. However as I do not support all the cases because it has no sense to do it for collinear and coincident cases and math should serve the purpose I'm not creating a specific math method for it. If you consider this should be done please let me know.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 187 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

For consistency, "points" should be "control_points".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 120 at r1 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Would adding the following sentence make sense?

This is because the returned api::LanePosition is isotropic, i.e., it contains real physical values.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 59 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider rewording:

A pointer to the segment that contains this lane, which must remain valid for the lifetime of this class.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 60 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider deleting "value of the".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 65 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Can the order of the parameters in this section be made to match the order of the parameters in the method's signature?

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 84 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"points" should be "control_points". Consider rewording this sentence as follows:

Computes the length of a SplineLane created using @p control_points.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 90 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider rewording for clarity:

The tuple's first value is the knot point while its second value is the tangent at the knot point.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 149 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider deleting "distance" since it's implied by "path length".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 190 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "s coordinate" with "@p s".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"to" should be "by".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider documenting the fact that this method supports @p lane_id equal to the current lane and will simply return the geo position at (s, 0, 0) of the current lane.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 191 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider adding "the lane with ID @p" after "function of".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 192 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, this approximation may have high error if the lane with ID lane_id is U-shaped right? How is this scenario handled or avoided?

Yes, it may have a high error. The builder (not pull requested yet) has code to create interpolated waypoints over the segment's lanes and improve the sampling over the lanes. This will reduce the error and couple lanes by having paired waypoints. Here you have something similar to a pair of U-shaped lanes, as you can see there are several arrows at the curved parts of the lanes. This is because of waypoint interpolation from one lane to the other based on the map information. As you can see, error is decreased by doing so.

---

drake/automotive/maliput/rndf/test/segment_test.cc, line 3 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, unused?

Done.

---

drake/automotive/maliput/rndf/test/spline_helpers_test.cc, line 11 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

The should remain at the top of the file per style guide since it classifies as being a "related header":

Use standard order for readability and to avoid hidden dependencies: Related header, C library, C++ library, other libraries' .h, your project's .h.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Done.

---

externals/bullet, line 1 at r4 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, why this change? (I briefly searched and couldn't immediately find why this is necessary.)

Done. There was an issue on our side with gitsubmodules. Sorry for it.

---

Comments from Reviewable

[liangfok]

The linux-xenial-clang-bazel-experimental-debug error is surprising:

/usr/bin/ld: final link failed: No space left on device

@drake-jenkins-bot linux-xenial-clang-bazel-experimental-debug please.

[agalbachicar]

Thanks @liangfok for the trigger. Removed CMakelists files to avoid conflicts with PR #6445. PTAL.

---

Review status: 4 of 11 files reviewed at latest revision, 9 unresolved discussions.

---

Comments from Reviewable

[liangfok]

Checkpoint.

---

Reviewed 3 of 9 files at r4, 2 of 5 files at r5, 2 of 3 files at r6.
Review status: 9 of 11 files reviewed at latest revision, 14 unresolved discussions.

---

a discussion (no related file):
Since drake::maliput::rndf::Lane only one one subclass, drake::maliput::rndf::SplineLane, why separate the two?

---

drake/automotive/maliput/rndf/segment.h, line 28 at r6 (raw file):

  /// Constructs a new Segment.
  /// @param This segment's ID.

Missing "id" after "@param".

---

drake/automotive/maliput/rndf/segment.h, line 29 at r6 (raw file):

  /// Constructs a new Segment.
  /// @param This segment's ID.
  /// @param The api::Junction that contains this Segment. It must remain valid

Missing "junction" after "@param".

---

drake/automotive/maliput/rndf/segment.h, line 30 at r6 (raw file):

  /// @param This segment's ID.
  /// @param The api::Junction that contains this Segment. It must remain valid
  /// for the lifetime of this class.

Let's change "class" to "object".

---

drake/automotive/maliput/rndf/segment.h, line 36 at r6 (raw file):

  /// Gives the segment a newly constructed SplineLane.
  ///
  /// @param The lane's ID.

Missing "id" after "@param".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

Done. AFAIK it's just line intersection in 3-D space. However as I do not support all the cases because it has no sense to do it for collinear and coincident cases and math should serve the purpose I'm not creating a specific math method for it. If you consider this should be done please let me know.

Thanks. I'm satisfied with the latest revision.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 126 at r5 (raw file):
This method needs to be called DoDriveableBounds() since it is more than 5 lines long:

The Function Names rule specifies that the names of “very cheap” methods may be all lower-case with underscores between words. It defines “very cheap” as a method that you wouldn’t hesitate calling from within a loop. We clarify that this method should have a time complexity of O(1) and be less than 5 lines long.

http://drake.mit.edu/code_style_guide.html#clarifications

---

drake/automotive/maliput/rndf/spline_lane.cc, line 174 at r5 (raw file):

  ignition::math::Vector3d t_q = (q_ending - q);
  t_q.Normalize();
  // Checks if lines are collinear.

Missing "the" before "lines".

---

drake/automotive/maliput/rndf/spline_lane.h, line 192 at r3 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

Yes, it may have a high error. The builder (not pull requested yet) has code to create interpolated waypoints over the segment's lanes and improve the sampling over the lanes. This will reduce the error and couple lanes by having paired waypoints. Here you have something similar to a pair of U-shaped lanes, as you can see there are several arrows at the curved parts of the lanes. This is because of waypoint interpolation from one lane to the other based on the map information. As you can see, error is decreased by doing so.

Thanks for the explanation. I'm marking myself satisfied.

---

drake/automotive/maliput/rndf/spline_lane.h, line 63 at r5 (raw file):
Instead of:

The tuple will be interpreted as with the first value as the knot and the second as the tangent.

Use:

The tuple's first value is the knot point while its second value is the tangent at the knot point.

---

drake/automotive/maliput/rndf/spline_lane.h, line 87 at r5 (raw file):

  /// Computes the length of a SplineLane created using @p control_points.
  /// The first value are the points and the second is the
  /// value at that point.

This sentence can be deleted since the information is contained in the description of parameter control_points.

---

drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5 (raw file):

  // at (s, 0, 0).
  // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.
  // Aborts the lane referred by @p lane_id has no intersection with this lane,

Missing "if" after "Aborts".

---

drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5 (raw file):

  // at (s, 0, 0).
  // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.
  // Aborts the lane referred by @p lane_id has no intersection with this lane,

Should "this lane" be "this lane's normal at @p s"?

---

drake/automotive/maliput/rndf/spline_lane.h, line 197 at r5 (raw file):

  // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.
  // Aborts the lane referred by @p lane_id has no intersection with this lane,
  // or it is collinear and coincident.

"it is" should be "the two lines are"

---

Comments from Reviewable

[liangfok]

Completed first pass.

---

Reviewed 1 of 9 files at r4, 1 of 5 files at r5.
Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 63 at r5 (raw file):

    const api::LanePosition& lane_pos) const {
  const double s = math::saturate(lane_pos.s(), 0., this->do_length());
  // Recover linear parameter p from path length position s.

This comment mentions "p", which doesn't match the variable name below.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 64 at r5 (raw file):
"Rabg" violates the style guide:

The names of variables (including function parameters) and data members are all lowercase, with underscores between words.

https://google.github.io/styleguide/cppguide.html#Variable_Names

Consider simplifying as follows:

return api::Rotation::FromRpy(0.0, 0.0, Rabg_of_s(s).yaw());

---

drake/automotive/maliput/rndf/spline_lane.cc, line 74 at r5 (raw file):
Consider changing:

xy_of_s it's called L

to be:

Let xy_of_s be called L,

---

drake/automotive/maliput/rndf/spline_lane.cc, line 75 at r5 (raw file):

Vector2<double> SplineLane::xy_of_s(double s) const {
  // xy_of_s it's called L which is a function
  // R --> R^2. We discard z component right now. We can say

Missing "the" after "discard".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 82 at r5 (raw file):

}
Vector2<double> SplineLane::xy_dot_of_s(double s) const {
  // We get here the tangent, which is the first derivative of

Consider replacing "We get here" with "Computes".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 91 at r5 (raw file):

  // The tangent of the heading is the function of y(s) / x(s).
  // So, we can say that h(s) = arctg (y(s) / x(s)). This function
  // is a function like: h(s) = R --> R or h(f(x, y)) where f it's

"it's" should be "is".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 91 at r5 (raw file):

  // The tangent of the heading is the function of y(s) / x(s).
  // So, we can say that h(s) = arctg (y(s) / x(s)). This function
  // is a function like: h(s) = R --> R or h(f(x, y)) where f it's

Consider replacing "is a function like" with "takes the form"

---

drake/automotive/maliput/rndf/spline_lane.cc, line 92 at r5 (raw file):

  // So, we can say that h(s) = arctg (y(s) / x(s)). This function
  // is a function like: h(s) = R --> R or h(f(x, y)) where f it's
  // a function defined like y / x. y and x are the components

Should "like" be "by"?

---

drake/automotive/maliput/rndf/spline_lane.cc, line 93 at r5 (raw file):
Should:

Then, we got:

be:

Thus, we get:

?

---

drake/automotive/maliput/rndf/spline_lane.cc, line 99 at r5 (raw file):

double SplineLane::heading_dot_of_s(double s) const {
  // Based on the explanation of heading_of_s, we got it applying the chain

Consider replacing "got it" with "get its derivative by"

---

drake/automotive/maliput/rndf/spline_lane.cc, line 103 at r5 (raw file):

  // dh / ds = d/ds {arctg (f(x(s), y(s)))}
  //         = 1 / (1 + f(x(s), y(s))^2) * d/ds {f(x(s), y(s))}
  // As x(s) and y(s) and independent polynomials, we can say that:

"As" should be "Since".

---

drake/automotive/maliput/rndf/spline_lane.cc, line 103 at r5 (raw file):

  // dh / ds = d/ds {arctg (f(x(s), y(s)))}
  //         = 1 / (1 + f(x(s), y(s))^2) * d/ds {f(x(s), y(s))}
  // As x(s) and y(s) and independent polynomials, we can say that:

"and" should be "are".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 38 at r6 (raw file):

// This is a wrapper to easily create an ignition::math::Spline.
// @param points a tuple consisting of two ignition::math::Vector3d. The
// first one is the position of the point and the second one is the tangent

Should "position of the point" be "knot point" for consistency with spline_lane.h?

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 52 at r6 (raw file):

}

// Makes geometric tests over a straight line lane.

BTW, consider replacing "Makes" with "Performs".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 52 at r6 (raw file):

}

// Makes geometric tests over a straight line lane.

BTW, consider deleting "line".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 54 at r6 (raw file):

// Makes geometric tests over a straight line lane.
GTEST_TEST(RNDFSplineLanesTest, FlatLineLane) {
  const double width = 5.;

This variable should be called kWidth.

https://google.github.io/styleguide/cppguide.html#Constant_Names

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 66 at r6 (raw file):

                      ignition::math::Vector3d(10.0, 0.0, 0.0)));
  Lane* l1 = s1->NewSplineLane({"l1"}, control_points, width);
  // Check road geometry invariants

Missing period at end of sentence. (Same comment below.)

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 117 at r6 (raw file):

}

// Makes geometric tests over a curved spline.

BTW, consider replacing "Makes" with "Performs".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 117 at r6 (raw file):

}

// Makes geometric tests over a curved spline.

BTW, consider replacing "spline" with "lane".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 119 at r6 (raw file):

// Makes geometric tests over a curved spline.
GTEST_TEST(RNDFSplineLanesTest, CurvedLineLane) {
  const double width = 5.;

This variable should be called kWidth.

https://google.github.io/styleguide/cppguide.html#Constant_Names

---

Comments from Reviewable

[agalbachicar]

Addressed comments. PTAL.

---

Review status: 7 of 11 files reviewed at latest revision, 32 unresolved discussions.

---

a discussion (no related file):

Previously, liangfok (Chien-Liang Fok) wrote…

Since drake::maliput::rndf::Lane only one one subclass, drake::maliput::rndf::SplineLane, why separate the two?

I have followed Monolane's implementation as a guide. I have thought that in case new functions are used to induce a geometry from RNDF's waypoints, having a separate class to handle all non-geometric stuff will facilitate the integration.

---

drake/automotive/maliput/rndf/segment.h, line 28 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "id" after "@param".

Done.

---

drake/automotive/maliput/rndf/segment.h, line 29 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "junction" after "@param".

Done.

---

drake/automotive/maliput/rndf/segment.h, line 30 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Let's change "class" to "object".

Done.

---

drake/automotive/maliput/rndf/segment.h, line 36 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "id" after "@param".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Thanks. I'm satisfied with the latest revision.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 63 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

This comment mentions "p", which doesn't match the variable name below.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 64 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"Rabg" violates the style guide:

The names of variables (including function parameters) and data members are all lowercase, with underscores between words.

https://google.github.io/styleguide/cppguide.html#Variable_Names

Consider simplifying as follows:

return api::Rotation::FromRpy(0.0, 0.0, Rabg_of_s(s).yaw());

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 74 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider changing:

xy_of_s it's called L

to be:

Let xy_of_s be called L,

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 75 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "the" after "discard".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 82 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider replacing "We get here" with "Computes".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 91 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"it's" should be "is".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 91 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider replacing "is a function like" with "takes the form"

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 92 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should "like" be "by"?

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 93 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should:

Then, we got:

be:

Thus, we get:

?

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 99 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Consider replacing "got it" with "get its derivative by"

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 103 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"As" should be "Since".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 103 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"and" should be "are".

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 126 at r5 (raw file):

This method needs to be called DoDriveableBounds() since it is more than 5 lines long:

The Function Names rule specifies that the names of “very cheap” methods may be all lower-case with underscores between words. It defines “very cheap” as a method that you wouldn’t hesitate calling from within a loop. We clarify that this method should have a time complexity of O(1) and be less than 5 lines long.
Since I'm implementing a virtual method, shall I create a new method called "DoDriveableBounds()" or leave it as it is and call it from do_driveable_bounds()?

---

drake/automotive/maliput/rndf/spline_lane.cc, line 174 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "the" before "lines".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 192 at r3 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Thanks for the explanation. I'm marking myself satisfied.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 63 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Instead of:

The tuple will be interpreted as with the first value as the knot and the second as the tangent.

Use:

The tuple's first value is the knot point while its second value is the tangent at the knot point.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 87 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

This sentence can be deleted since the information is contained in the description of parameter control_points.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing "if" after "Aborts".

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should "this lane" be "this lane's normal at @p s"?

Done. You're right.

---

drake/automotive/maliput/rndf/spline_lane.h, line 197 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

"it is" should be "the two lines are"

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 38 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Should "position of the point" be "knot point" for consistency with spline_lane.h?

Done. Copied variable name and comments from header file.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 52 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "Makes" with "Performs".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 52 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider deleting "line".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 54 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

This variable should be called kWidth.

https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 66 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Missing period at end of sentence. (Same comment below.)

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 117 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "Makes" with "Performs".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 117 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider replacing "spline" with "lane".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 119 at r6 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

This variable should be called kWidth.

https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

---

Comments from Reviewable

[liangfok]

---

Reviewed 4 of 4 files at r7.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

a discussion (no related file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

I have followed Monolane's implementation as a guide. I have thought that in case new functions are used to induce a geometry from RNDF's waypoints, having a separate class to handle all non-geometric stuff will facilitate the integration.

I see. OK I'll mark myself satisfied.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 126 at r5 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

This method needs to be called DoDriveableBounds() since it is more than 5 lines long:

The Function Names rule specifies that the names of “very cheap” methods may be all lower-case with underscores between words. It defines “very cheap” as a method that you wouldn’t hesitate calling from within a loop. We clarify that this method should have a time complexity of O(1) and be less than 5 lines long.
Since I'm implementing a virtual method, shall I create a new method called "DoDriveableBounds()" or leave it as it is and call it from do_driveable_bounds()?

Oops! Sorry I forgot this was overriding an api::Lane method. Let's leave it as-is.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 82 at r7 (raw file):

}
Vector2<double> SplineLane::xy_dot_of_s(double s) const {
  // Computes tangent, which is the first derivative of

BTW, missing "the" after "Computes".

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 37 at r7 (raw file):

// This is a wrapper to easily create an ignition::math::Spline.
// @param control_points A tuple consisting of two ignition::math::Vector3d. The

BTW, "tuple" should be "vector of tuples"

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 37 at r7 (raw file):

// This is a wrapper to easily create an ignition::math::Spline.
// @param control_points A tuple consisting of two ignition::math::Vector3d. The

BTW, consider adding "objects" after "ignition::math::Vector3d".

---

Comments from Reviewable

[agalbachicar]

Review status: 9 of 11 files reviewed at latest revision, 3 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 126 at r5 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

Oops! Sorry I forgot this was overriding an api::Lane method. Let's leave it as-is.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 82 at r7 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, missing "the" after "Computes".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 37 at r7 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding "objects" after "ignition::math::Vector3d".

Done.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 37 at r7 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, "tuple" should be "vector of tuples"

Done.

---

Comments from Reviewable

[agalbachicar]

+@soonho-tri for platform-review please.

[jwnimmer-tri]

platform

---

Reviewed 3 of 10 files at r1, 3 of 9 files at r4, 1 of 5 files at r5, 2 of 4 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.h, line 45 at r8 (raw file):

 private:
  Eigen::Matrix<double, 3, 1, Eigen::DontAlign> rpy_;
};

This is copypasta from monolane. Factor for reuse.

(Neither of them should have this class, but should rather prefer to use pre-existing representations for this. But for now, having them share a single copy of a bad abstraction is better than having two copies of a bad abstraction.)

---

drake/automotive/maliput/rndf/test/rndf_test_utils.h, line 1 at r8 (raw file):

#pragma once

(1) Most of this file is copypasta from monolane. Those helpers should be reused, instead of duplicated.

(2) BTW This is not how googletest is supposed to work. Ideally we would use one of the googletest-idiomatic ways of expressing how to compare two GetPosition etc are to be compared for (in)equality.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 19 at r8 (raw file):

#include "drake/automotive/maliput/rndf/segment.h"
#include "drake/automotive/maliput/rndf/spline_helpers.h"
#include "drake/automotive/maliput/rndf/spline_lane.h"

I think this is the "related header" and thus should appear atop the file?

---

Comments from Reviewable

[agalbachicar]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.h, line 45 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is copypasta from monolane. Factor for reuse.

(Neither of them should have this class, but should rather prefer to use pre-existing representations for this. But for now, having them share a single copy of a bad abstraction is better than having two copies of a bad abstraction.)

Shall I leave it for a subsequent PR? I can promote common Monolane and RNDF code to Maliput interface if needed. If you agree I would keep it in different PRs.

---

drake/automotive/maliput/rndf/test/rndf_test_utils.h, line 1 at r8 (raw file):

st of this file is copypasta from monolane. Those helpers should be reused, instead of duplicated
I agree with both (1) and (2). (1) was done taken into account current code in Monolane and trying to avoid dependencies from each other. I guess it should be promoted to Maliput. I can do it in a subsequent PR, does it sound ok? The same as the previous comment.

---

Comments from Reviewable

[jwnimmer-tri]

There is no reason to hurry on this PR, right? If you want to do the factoring in a separate PR that's fine, but then that PR needs to merge first and this one rebase atop it.

[jwnimmer-tri]

drake/automotive/maliput/rndf/test/rndf_test_utils.h, line 1 at r8 (raw file):

Previously, agalbachicar (Agustin Alba Chicar) wrote…

st of this file is copypasta from monolane. Those helpers should be reused, instead of duplicated
I agree with both (1) and (2). (1) was done taken into account current code in Monolane and trying to avoid dependencies from each other. I guess it should be promoted to Maliput. I can do it in a subsequent PR, does it sound ok? The same as the previous comment.

Clarifying: for (2) its OK to fix separately (or never). Only (1) is the defect for this PR.

---

Comments from Reviewable

[agalbachicar]

I've just remembered this comment from @maddog-tri . I would like to bring him and his advice on Maliput code promotion from specific implementations just to do the right thing.

[liangfok]

Reviewed 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 80 at r8 (raw file):

      spline_->InterpolateMthDerivative(0, s);
  return {point.X(), point.Y()};
}

BTW, consider adding a blank line after this closing curly brace for consistency with the rest of this file.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 87 at r8 (raw file):

      spline_->InterpolateMthDerivative(1, s);
  return {point.X(), point.Y()};
}

BTW, consider adding a blank line after this closing curly brace for consistency with the rest of this file.

---

Comments from Reviewable

[liangfok]

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 4 files at r10.
Review status: 9 of 10 files reviewed at latest revision, 4 unresolved discussions.

---

drake/automotive/maliput/rndf/BUILD, line 72 at r10 (raw file):

drake_cc_googletest(
    name = "road_geometry_test",
    size = "small",

FYI This is the default, and not usually written out.

---

Comments from Reviewable

[jwnimmer-tri]

Reviewed 2 of 4 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[jwnimmer-tri]

I this this is +(status: ready to merge) after Jenkins passes.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[agalbachicar]

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

drake/automotive/maliput/rndf/BUILD, line 72 at r10 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI This is the default, and not usually written out.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 80 at r8 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding a blank line after this closing curly brace for consistency with the rest of this file.

Done.

---

drake/automotive/maliput/rndf/spline_lane.cc, line 87 at r8 (raw file):

Previously, liangfok (Chien-Liang Fok) wrote…

BTW, consider adding a blank line after this closing curly brace for consistency with the rest of this file.

Done.

---

drake/automotive/maliput/rndf/spline_lane.h, line 45 at r8 (raw file):

hall I leave it for a subsequent PR? I can promote common Monolane and RNDF code to Maliput interface if needed. If you agree I would keep it in different PRs.
Removed in favor of rotation matrices from drake::math.

---

drake/automotive/maliput/rndf/test/spline_lane_test.cc, line 19 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this is the "related header" and thus should appear atop the file?

Done.

---

drake/automotive/maliput/rndf/test/rndf_test_utils.h, line 1 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Clarifying: for (2) its OK to fix separately (or never). Only (1) is the defect for this PR.

Done. With PR #6506 we have Google Test idiomatic functions to compare both Maliput and Ignition types.

---

Comments from Reviewable

[agalbachicar]

Comments addressed. Thanks for the reviews.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable