Re: [boost] [Review] GGL review starts today, November 5th

Subject: Re: [boost] [Review] GGL review starts today, November 5th
From: Brandon Kohn (blkohn_at_[hidden])
Date: 2009-11-19 15:14:49

Hi all,

First I would like to say that I appreciate all the hard work that has 
gone into the library. Implementing a fully generic geometry library is 
a very difficult undertaking. Having said that I don't think that GGL is 
at the level of maturity that I would expect from a Boost.Geometry 
library. During the course of my review I would constantly ask myself 
when using a given feature whether this is something that I would use in 
my professional work without hesitation, and my answer was often that it 
didn't seem quite ready for production use. I think with a few more 
iterations and more collaboration with the Boost community it could be.
So my formal vote is not to accept at this time.

- What is your evaluation of the design?

The quality of the interface varies, but is in general OK. The use of 
tag-dispatching and trait specialization seems reasonable as does the 
strategy design.

Other parts of the design seem a bit more ad-hoc.

For example:

template<typename DegreeOrRadian>
struct geographic
{
     typedef DegreeOrRadian units;
};

Which seems to be a way to specify a unit type for angular coordinates. 
I'm not sure how such a construct extends into other unit types such as 
feet/meters on distance coordinates. It is perhaps not so relevant in 
the context of the operations required to be performed on a given unit 
type. I think a better design would have incorporated these nuances into 
a trait for the given coordinate. Given the availability of Boost.Units 
I'm sure something neat could be done on this idea. Further to this I 
found a type trait struct 'is_radian<T>' but none for 'is_degree' (or 
is_meter, is_feet). I think these artifacts will present issues if the 
geometry library were to be later abstracted to support units on the 
coordinate types.

Dimensionally qualified quantities can be calculated for geometries for 
whose dimensions the quantity makes no sense (e.g. area( point ), 
length( point ).) These functions return default constructed instances 
of their corresponding return type as shown here:

template<typename ReturnType, typename Geometry, typename Strategy>
struct calculate_null
{
     static inline ReturnType apply(Geometry const& , Strategy const&)
     {
         return ReturnType();
     }
};

In my test ReturnType was double. This would seem to silently compile 
and return garbage on these inputs. In practice on vs2008( VC9 ) these 
return values were 0 (with optimizations on) even though the return 
resolves to double();. I'm guessing other platforms won't be so lucky.

The geometry concepts seem sound at a glance, but I wonder how they 
would fare in the light of extension. For example, the ggl::point_type< 
Geometry > meta-function would seem to presume that all geometries have 
an underlying point representation. This doesn't necessarily extend to 
lines or planes, and so may present issues later on when those types are 
added.

Some of the default geometry types hold data by reference which makes 
their usage in containers a bit difficult (specifically the segment type.)

Much of the nomenclature of functions and types in the library seems to 
come from the OGC (Open Geospatial Consortium) or so I assume. I don't 
think this consortium is widely recognized as a standard for geometry 
names in general. I'm not suggesting that there is a standard, but I 
think better names may have been chosen in light of more common usages 
in academic computational geometry literature. For example, we have 
polygon, but then linestring (rather than polyline). The term 'envelope' 
is used to refer to what I believe is known in the literature as an 
axis-aligned bounding box. This may come down to stylistic preference, 
but I would expect a Boost.Geometry to have a higher level of fidelity 
in the nomenclature.

- What is your evaluation of the implementation?

The implementation seems technically competent. The segment intersection 
algorithm matches the speed of what we use in our production code. The 
boolean operations seem fast. My tests of the area/length/centroid 
operations gave correct results.

- What is your evaluation of the documentation?

The documentation is not sufficient. I would suggest better tutorials 
for all common use cases. A simple query on the user newsgroup would 
probably give a good list of those most common cases. Overall, the 
documentation is probably the least polished part of the submission.

- What is your evaluation of the potential usefulness of the library?

Tremendously useful.

- Did you try to use the library?  With what compiler?  Did you have any
problems?

Yes. VS 2008 (vc9). Yes, a few. The most troubling was a seg-fault while 
trying to synthesize a boolean difference operation. Some of the default 
geometry concept implementations were clunky to use in what I would 
consider a common use case. (As I said above the segment type isn't 
default constructible due to holding points by reference..). This latter 
issue is a bit trivial as I can write my own, but I figured it might 
speak to the library's maturity as rough edges like this tend to be 
fixed with wide use.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

In-depth study.

- Are you knowledgeable about the problem domain?

Yes. Over the course of my professional career I have worked on 2 
geometry libraries.

I think there is a lot of potential here and hope that work continues.

Best of luck,

Brandon