Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getPolygon with 0 points #1122

Closed
TauTheLepton opened this issue Oct 31, 2023 · 5 comments
Closed

getPolygon with 0 points #1122

TauTheLepton opened this issue Oct 31, 2023 · 5 comments

Comments

@TauTheLepton
Copy link
Contributor

TauTheLepton commented Oct 31, 2023

Describe the bug
The CatmullRomSpline member function getPolygon when called with num_points = 0 raises an error with a description failed to calculate curve index.
This description does not say where the real error is in this case. It is thrown deep from the call stack by member function getCurveIndexAndS in the call order presented below:
getPolygon -> getLeftBounds -> getNormalVector -> getCurveIndexAndS
This happens because the num_points (= 0) is used in getLeftBounds to calculate step_size (which is then equal to infinity).

double step_size = getLength() / static_cast<double>(num_points);

This leads to a NaN s value which then is not matched to any component curve and a mentioned exception is being thrown by getCurveIndexAndS.
double s = step_size * static_cast<double>(i);

In my opinion there are three ways to improve the behavior in this situation:

  • To catch the exception in the getPolygon function and thrown another exception with more informative description
  • To just throw an exception in getPolygon when num_points == 0
  • To add an early return in getPolygon to return an empty vector when num_points == 0

Context:
I am adding unit tests for the geometry package and wanted to add tests for the getPolygon function.

To Reproduce
Steps to reproduce the behavior:

  1. Edit any existing source code file or create a new one
  2. Create any CatmullRomSpline
  3. Call the getPolygon member function with num_points argument set to 0
  4. Compile and execute
  5. See the error

Expected behavior
Either an exception with an appropriate description or the return of an empty vector.

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

Desktop (please complete the following information):

  • OS: [e.g. iOS]: Ubuntu
  • Browser [e.g. chrome, safari]: Firefox
  • Version [e.g. 22]: 22
  • ROS 2 version: Humble
  • DDS: CycloneDDS
@hakuturu583
Copy link
Collaborator

I think CatmullRomSpline class cannot construct with 0 control points.

case 0:
THROW_SEMANTIC_ERROR(
"Control points are empty. We cannot determine the shape of the curve.",
"This message is not originally intended to be displayed, if you see it, please contact "
"the developer of traffic_simulator.");

So, I think it would not be a problem.
How did you test this issue?

@TauTheLepton
Copy link
Contributor Author

TauTheLepton commented Nov 2, 2023

I am sorry if my explanation was not clear enough.
You are right, the CatmullRomSpline cannot be constructed with 0 control points.
However, the member function getPolygon

auto CatmullRomSpline::getPolygon(
const double width, const size_t num_points, const double z_offset)
-> std::vector<geometry_msgs::msg::Point>
{
std::vector<geometry_msgs::msg::Point> points;
std::vector<geometry_msgs::msg::Point> left_bounds = getLeftBounds(width, num_points, z_offset);
std::vector<geometry_msgs::msg::Point> right_bounds = getRightBounds(width, num_points, z_offset);
for (size_t i = 0; i < static_cast<size_t>(left_bounds.size() - 1); i++) {
geometry_msgs::msg::Point pr_0 = right_bounds[i];
geometry_msgs::msg::Point pl_0 = left_bounds[i];
geometry_msgs::msg::Point pr_1 = right_bounds[i + 1];
geometry_msgs::msg::Point pl_1 = left_bounds[i + 1];
points.emplace_back(pr_0);
points.emplace_back(pl_0);
points.emplace_back(pr_1);
points.emplace_back(pl_0);
points.emplace_back(pl_1);
points.emplace_back(pr_1);
}
return points;
}

takes num_points as an argument. This argument is used to construct a polygon (or rather pairs of triangles) approximating the surface of the spline with a given width. It is independent from the number of control points that were used to construct the CatmullRomSpline.
This num_points argument is not checked anywhere, and when it is set to 0, then the behavior I have described above is triggered (which leads to an exception).
I have created an example test case to showcase the bug here.

To see the problem:

  1. Clone this branch
  2. Build
  3. Run tests
  4. See the error in the CatmullRomSpline example test case

@TauTheLepton
Copy link
Contributor Author

@hakuturu583
Does the explanation in my previous message clearly explain where the bug is?

@hakuturu583
Copy link
Collaborator

Sorry for my late reply.
I understand...
It is a bug.

@TauTheLepton
Copy link
Contributor Author

This issue was solved under #1139, but I forgot to mention this issue in the description of the #1139.
The #1139 was merged, so I am closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants