search

gazebosim / gz-math

General purpose math library for robot applications.
https://gazebosim.org/libs/math
Apache License 2.0
55 stars 68 forks source link

Add sphere and cylinder shapes #90

Closed osrf-migration closed 6 years ago

osrf-migration commented 6 years ago

Original report (archived issue) by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

We already have Box, OrientedBox and Frustum classes which provide handy functions like checking whether they contain a point, intersect with a line and so on.

It would be nice to have other shapes too, like Sphere and Cylinder

osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

IssueForNewDevelopers

osrf-migration commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

I would actually urge us to think carefully before adding more shape classes to ign-math.

As we experienced with the frustum class, collision detection is a really really hard problem, and it becomes combinatorially harder as you add arbitrarily more shape types to the mix.

If anything, I would suggest that we consider ways to have better integration with third-party collision detectors like FCL, ODE, Bullet, etc. Ideally we'd provide an abstract shape + collision detection interface for third-party collision detection libraries, and then choose a few popular open source collision detectors to provide stable support for, in the form of an optional component library.

It's not totally obvious to me where that kind of abstract shape concept would belong. I'm a bit reluctant to put it in ign-math, since ign-math (as far as I'm aware) has a history of just containing basic plain data types with some convenience functions. It wasn't particularly built to deal with abstraction, and supporting multiple collision detectors would be best facilitated by a plugin framework. ign-physics is all about abstraction and plugins, so I'd feel at least somewhat inclined to put this kind of thing in there instead.

osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

I agree with all your points regarding collision detection. But we should keep in mind that there are many other use cases for a mathematical representation of a shape. Just like we use the Box and Frustum classes right now for a variety of logical tasks, Sphere and Cylinder classes could be convenient for things outside of physics, like visualizations, sensors and just general 3D geometry logic.

osrf-migration commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

To elaborate a little more on my thoughts, I definitely agree that mathematical representations of various shapes are valuable. Putting that into ign-math makes a lot of sense, since it would give various ignition projects an upstream way of communicating about geometry information.

The design concerns that I have in mind right now are the following:

Do we want to have geometry abstraction? And if so, what library does it belong in?

It can be very useful to have an abstraction for the concept of a "geometric shape". For example, it's useful if you can pass an arbitrary shape (whether it's a Circle, Cylinder, Box, Mesh, etc) to a collision detection engine or a rendering engine and expect it to handle the shape correctly. It's especially valuable if this is clean and extensible, e.g.:

class CollisionDetector
{
public:
  CollisionObjectPtr AddShape(const Shape& shape);
};

where Shape can be any of a number of types, instead of something like:

class CollisionDetector
{
public:

  CollisionObjectPtr AddShape(const Sphere& sphere);

  CollisionObjectPtr AddShape(const Box& box);

  CollisionObjectPtr AddShape(const Cylinder& cylinder);

  // ... and so on ...
};

This would require the Shape class to provide a pure virtual interface for the various shape types to inherit and implement. That means these shape types would have virtual functions and have some level of sophistication, which we should carefully design.

It sounds like your proposal is to have relatively plain data types for Sphere and Cylinder that only contain the minimal parameters needed to describe a "sphere" or "cylinder" concept. I think it makes a lot of sense to provide such classes, and I'm definitely in favor of doing so.

However, the idea of more sophisticated abstract shape classes could be useful to both physics and rendering (and maybe other projects?), so it might make sense for them to share the implementation of that inside of ign-math. In that case, we would probably want ignition::math::Sphere to be the more sophisticated abstracted shape type instead of a simple class that just holds a radius parameter. Maybe we would actually want the plain sphere parameter class to be named something like ignition::math::SphereParameters or ignition::math::Sphere::Parameters. If we consistently use the nested ::Parameters class pattern across all of the shape types, it would allow us (and users) to do some really fancy template programming. However, these things would be painful to introduce if those class names are already occupied (in fact, I think there would already be some pain caused by the existence of the ignition::math::Box class).

Do we want to template the shape classes for 2D vs 3D?

The Feature System proposal in ign-physics would allow us to very easily and cleanly support physics interfaces that can be shared between 2D and 3D simulation frameworks. However, it will require that our mathematical representations are templated for 2D and 3D information. This is very straightforward to implement using a template library like Eigen, and we could offer shape classes like:

template<typename Precision, unsigned int Dimension>
class Ball : public Shape<Precision, Dimension>
{
  public: Precision& Radius() { return radius; }
  public: const Precision& Radius() const { return radius; }

  private: Precision radius;
};

using Ball3d = Ball<double, 3>;
using Ball2d = Ball<double, 2>;
using Sphere = Ball3d;
using Circle = Ball2d;

template<typename Precision, unsigned int Dimension>
class Box : public Shape<Precision, Dimension>
{
  public: Precision& operator[](unsigned int i) { return parameters[i]; }

  public: Precision& XLength() { return parameters[0]; }
  public: Precision& YLength() { return parameters[1]; }

  // Template magic that makes this only compile when Dimension==3
  public : Precision& ZLength() { return parameters[2]; }

  private: Eigen::Matrix<Precision, Dimension, 1> parameters;
};

using Box3d = Box<double, 3>;
using Box2d = Box<double, 2>;
osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

I think geometry abstraction can be very useful. We currently have gazebo::physics::Shape and I agree it belongs more in math than in physics. Interestingly though, there is no such concept on gazebo::rendering, in fact, rendering geometries are created by gazebo::common::MeshManager.

Templating for different numbers of dimensions also sounds like a good idea. Ignition math currently has completely separate classes for things like Vector2, 3 and 4, which means there is some duplicate code and some inconsistent API (issue #71 for example).

osrf-migration commented 6 years ago

Original comment by Michael Grey (Bitbucket: mxgrey, GitHub: mxgrey).

Supporting shape information within ign-math would probably also make it easier to transmit shape-related messages over ign-msgs.

osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).

Sphere and cylinder classes have recently been added:

I'll close this issue. @mxgrey, feel free to ticket a new issue about shape templates if you think that's still relevant.

osrf-migration commented 6 years ago

Original comment by Louise Poubel (Bitbucket: chapulina, GitHub: chapulina).