Negative cylinder mesh shapes eat up all (at least, most) of memory

[clalancette]

I'm currently looking at ros/robot_model#175 , and ran into what may be a bug. According to the URDF spec, a link with a cylinder can have a negative length and still be well-formed. For most software, this means the cylinder grows in the negative direction from the origin. However, when urdf_to_collada runs across it, it ends up down in geometric_shapes:createMeshFromShape(cylinder), where we have this code:

  double h = cylinder.length;
  ...
  unsigned int h_num = ceil(h / circle_edge);
  ...
  for (unsigned int i = 0; i < h_num - 1 ; ++i)
    for(unsigned int j = 0; j < tot; ++j)
      vertices.push_back(Eigen::Vector3d(r * cos(phi + phid * j), r * sin(phi + phid * j), h / 2 - (i + 1) * hd));

Essentially, we divide a negative length by a positive number, assign to an unsigned (now a huge positive number), and then push_back millions of items into the vector, exhausting memory. One way to fix this might be to take fabs(h) when calculating h_num, but I'm not sure if that is correct. Any advice here on how we might fix this?

[de-vri-es]

It seems odd to me that the length of the cylinder affects the number of vertices. That doesn't seem necessary. Easiest fix without changing that would be to simply take the std::abs(h) when computing h_num.

I wonder if we can just remove it though, and connect the top and bottom circles directly.

[clalancette]

If you think we should go with the change to std::abs, I can open a pull request for that. I'm not that familiar with this code, so I wouldn't be comfortable doing a more in-depth change to connect the top and bottom circles.

[de-vri-es]

Understandable. A PR with std::abs would be welcome. You may also want to look at the code for converting cones to meshes, it probably has the same problem.

I suspect the extra circles may be a left-over artefact of common code (apparently adapted from FCL) for rendering cylinders and cones. But std::abs will fix the bug for now.

[BryceStevenWilley]

#81 has been merged and released into kinetic and melodic, this issue should be fixed. Feel free to reopen if it's not.