tier4 / scenario_simulator_v2

A scenario-based simulation framework for Autoware
Apache License 2.0
122 stars 60 forks source link

getMaximum2DCurvature wrong curvature calculation #1119

Closed TauTheLepton closed 10 months ago

TauTheLepton commented 1 year ago

Describe the bug CatmullRomSpline member function getMaximum2DCurvature calculates the wrong value. The member function finds the greatest max 2D curvature value from all curves that are included in the spline. This is done in the line below. https://github.com/tier4/scenario_simulator_v2/blob/173a8c1d5e16b6e594de58d78cdb81a1d518ad82/common/math/geometry/src/spline/catmull_rom_spline.cpp#L570 However the max 2D curvature can be a positive as well as a negative value. The curvature magnitude is determined by the absolute value of the curvature value, which is considered in the function below. https://github.com/tier4/scenario_simulator_v2/blob/173a8c1d5e16b6e594de58d78cdb81a1d518ad82/common/math/geometry/src/spline/hermite_curve.cpp#L303-L310 This means that the CatmullRomSpline member function getMaximum2DCurvature having two values: -10 and 2 will choose 2, however the value -10 corresponds to a much greater curvature, just in the other direction. For a spline having all component curves bending in one way (when all component max 2D curvatures are negative) this member function actually does the opposite of what is should - it selects the smallest curvature. The logic present in HermiteCurve member function should be applied to a CatmullRomSpline member function as well.

Context: I am adding unit tests for the geometry package and wanted to add tests for the getMaximum2DCurvature function, but it generates the wrong result in a case similar to the example above.

To Reproduce Steps to reproduce the behavior:

  1. Edit any existing source code file or create a new one
  2. Create a CatmullRomSpline passing through points (0, 0), (0.5, 0.5), (1, 0), (2, -1) and (3, 0)
  3. Call the getMaximum2DCurvature member function
  4. Print and see the incorrect result

Expected behavior Greatest absolute value should be selected instead of the greatest one.

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

hakuturu583 commented 1 year ago

Thanks, it is a bug.

hakuturu583 commented 10 months ago

1139 was merged