sempr-tk / sempr

SEMPR - Semantic Environment Mapping, Processing and Reasoning
BSD 3-Clause "New" or "Revised" License
7 stars 1 forks source link

Extended PointCloud Interface #44

Closed ctieben closed 5 years ago

ctieben commented 6 years ago

A extended PointCloud shall contain following information's:

Minimal:

Optional (annotated for every point):

In discussion:

How the input format to a point cloud looks like is currently in discussion with our partner.

I would perform an interface that is able to store these information.

The different question for an different issue is if we like to use sempr to store a huge pointcloud with these information.

niniemann commented 6 years ago

How should the "list of points" look like? Or should the interface be a visitor-pattern, just like in geos?

y0urself commented 6 years ago

In geos the points are accessable as a std::vector<geos::geom::Coordinate>. As I said the problem is to connect the optional information with the Coordinates ...

Or do you think about adding an additional List of Points?

ctieben commented 6 years ago

No, not a additional list. Like its currently done, a vector of coordinate fits my idea of a "List of Points". (I have edited the starting comment)

y0urself commented 6 years ago

So this is, what we got from the inheritance of MultiPoint. It can return a CoordinateSequence which can easily be accessed as an std::vector with CoordinateSequence::toVector().

As I said (again :D ): When we got these seperated Coordinates in geos - how can we correctly and safety connect them to the rest of information?

An idea, I already talked about to Nils (and he didn't like), was:

niniemann commented 6 years ago

I think my previous argument was about having one base class as an interface instead of using the MultiPoint internally, so that all the processing modules don't have to differ between the implementations. That's why I wanted you to inherit MultiPoint, at least. ^^

Side node: We are currently thinking about a "little" restructure (again) which would allow us to add arbitrary interface-classes within the hierarchy. If/when we manage to implement that this would get rid of the need to inherit from MultiPoint as "the" interface.

But all in all we need a common, sensible interface for pointclouds. And somehow we should/must allow optional per-piont-properties (color, hyperspectral, soil-quality, ...). I'm not saying I know how to implement this propertly, yet. But this discussion should mostly revolve around the interface, right?

ctieben commented 6 years ago

Yes. I have opened this issue to find a 'final' pointlcloud interface.

One example is the pointcloud of ROS (or PCL) see http://wiki.ros.org/pcl/Overview They had the same connection issue. There is a channel list in the older PointCloud interface that contain optional information for each point. So the user have to ensure at runtime that channel and the points have the same length. The newer (PCL) PointCloud2 supports many different types of points. So its difficult to append addition information later on. (see here)

Overall I also didn't know the best solution for our case and couldn't assume how flexible our Point Cloud have to be.

y0urself commented 6 years ago

So this looks really complicated and this is nearly the same as I wanted to do? They've implemented a whole lot of different types of PointClouds.

I still think, we should do something like:

enum ColorType
{
  RGB,
  CMYK,
  HSV,
  YUV,
  //...
};

struct Color
{
  ColorType type;
  // maybe a vector with doubles or something ...
  // some Colortypes need more than 3 variables
  unsigned char r, g, b; // just pro forma
};

// what ever is needed here for Hyperspectral Data ...
struct HyperSpec
{
  std::vector<double> channels;
};

struct Point
{
  // this can be used to make a geos::geom::Geometry from the points, if we need them
  geos::geom::Coordinate coordinate;                                    
  struct Color color;
  struct HyperSpec hyperspec;
  struct ...
};

std::vector<Point> m_points;

So then we need safety access and functions that give feedback, about what informations are available in the instance. We need a function to create the Geometry, if it is needed for spatial reasoning. We need to provide accessors for the information (that only return information, if available) - so the vector shouldn't be accessible directly.

niniemann commented 6 years ago

This is already implementation detail, not the interface. ;) And don't make things too complicated: Do we really need to support all the different color types? IMHO RGB totally fine. If your sensor output is HSV or whatever, you'll have to convert it. Also, having all those structs inside the Point really bloats it if we only need xyz (e.g.). I'd rather think of separate data-channels which can be added optionally?

y0urself commented 6 years ago

I don't think, that attributes are implementation details, but interface.

So if we do not need to be really flexible with out PointCloud and force the user to convert everything into our interface, we could be fine with using only RGB. Otherwise we could provide a converter, too.

When we use different channels, the points can be stored in the geos::geom::Geometry, as they are now (and every other Geometry-Entity is just wrapped around a geos::geom::Geometry). And we only need to make sure, that the other channels have the same amount of data, as @ctieben and me already mentioned.

Do you want to create an abstract interface. I am not sure if there are that much different types of PointClouds that warrant an abstract interface.

ctieben commented 6 years ago

I have implement a first draft within 621af85dfacb1fda8cc30f7dfd6f1f6534c99206 and 04c01c483123a2ce9f3a942d01fda642742885b0 .

ctieben commented 6 years ago

There is an update of the PointCloud interface. See f5e2699d4948964e0485356afd12f597cc985b5a Now it could be abstract and its safe.

ctieben commented 6 years ago

In d1ae2af52e997268a315fcc1e41842098f18240b I templated the pointcloud and channel class to be able to store channels of different type.

So now its possible to store a list of uint8_t and float next to a pointcloud. (see the mixed_channel test)

But followed by this you have to know the type of a channel by runtime. e.g. you have to know that the R channel stores uint8_t you will get an exception of you try to interpret this as something else.

There is now other way if we like to store different types of channels dynamically at runtime! The first option is to template all type strict for a pointcloud. But than its not possible to add a new channel at runtime without a type conversion of the hole pointcloud. The second option is to store double for the channels because it can store most scalar values. This option is used before but it wast a lot of memory if you e.g. only like to store a uint8_t instead.

So I like to know if this way between is fine for you before I spend some more time on it.

ctieben commented 5 years ago

The PR #57 contains a PointCloud with Channels of different kinds. Its possible to set and modify the entries of the channels and points.

But for now its not so easy to add and remove points dynamically. To do so with geos the PointCloud have to emulate some parts of the GeometryCollection and GeometryFactory. By this its possible to use the MultiPoint only as a kind wrapper for the basic structures inside of the pointcloud.