Subject: Re: [boost] [Review] GGL review starts today, November 5th
From: Brandon Kohn (blkohn_at_[hidden])
Date: 2009-11-19 15:14:49
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
Other parts of the design seem a bit more ad-hoc.
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
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>
static inline ReturnType apply(Geometry const& , Strategy const&)
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
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?
- Did you try to use the library? With what compiler? Did you have any
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?
- Are you knowledgeable about the problem domain?
Yes. Over the course of my professional career I have worked on 2
I think there is a lot of potential here and hope that work continues.
Best of luck,
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk