Re: [Boost-users] [Review] GGL review starts today, November 5th

Subject: Re: [Boost-users] [Review] GGL review starts today, November 5th
From: Michael Fawcett (michael.fawcett_at_[hidden])
Date: 2009-11-20 13:07:34

On Thu, Nov 5, 2009 at 9:19 AM, Hartmut Kaiser <hartmut.kaiser_at_[hidden]> wrote:
>
> Please always state in your review, whether you think the library should be
> accepted as a Boost library.

I must admit, I have been on the fence about this for a few days,
however, I am giving a Yes vote.

Some general thoughts:

Integration with Boost.Units should be encouraged.  I don't think
is_angle, etc should be in GGL.  If I have a vector of quantities,
things should Just Work.

I'm unsure of the focus of GGL.  Given the current offering, I don't
think GGL is an appropriate name.  I don't think Boost.Polygon is
appropriate either.  IMVHO Boost.Polygon should be Boost.VLSI and Luke
can drop floating point support if he believes it offers no benefits
to that domain (I'm sure he would love less work  :) ).

If the GGL authors decide their focus area is the core geometry
concepts, then the name GGL is fine.  As it is, Barend favors GIS,
Bruno favors gaming, and the resulting library is a mashup of GIS
terms and other arbitrary terminology and it excels in no one
particular area.  There even appears to be plans to support 3D CSG
operations in the future.  Just these three domains are HUGE,
warranting libraries all on their own.

I suppose the vision would be that each of those domain specific
libraries be built on GGL.  Ambitious, and I support the vision, but I
think the focus needs to be narrowed for the time being.

> - What is your evaluation of the design?

IMO, this is a weak spot.  The concepts need work.  They are poorly
documented, and I don't think they represent the minimal set of
operations required.

> - What is your evaluation of the implementation?

I didn't look at the actual code, but the benchmarks and the ongoing
discussions seem to indicate they are of fairly high quality.  It
sounds like there's plenty of room for improvement, both in robustness
and speed.

> - What is your evaluation of the documentation?

I agree with all of Thomas' points on the documentation, so I won't
make my post longer by restating them all here.

I think it would benefit from step-by-step tutorials, much like the
Boost.Iterator docs for iterator_facade and adaptor.

The concepts should be documented like Boost.Graph.

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

I think it will be very useful eventually.

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

Yes.  I spent a few hours trying to get it to with ESRI, a leader in
the GIS domain.  Their API is completely COM based however, and it
proved extremely difficult to get working with GGL.  I wasn't able to
successfully do this with Boost.Polygon either.

I then tried using my own custom math types and had more success,
although not without problems.

The Examples link at the bottom of the main page of documentation is
broken (last line), and custom_point_example.cpp doesn't compile -
cs::cartesian needs to be ggl::cs::cartesian.

It was not clear what includes I had to include to register my custom
point type.  GEOMETRY_REGISTER_POINT_2D is defined in
ggl/geometries/register/point.hpp, but the following won't compile:

#include <ggl/geometries/register/point.hpp>

struct test {
        double x, y;
};
GEOMETRY_REGISTER_POINT_2D(test, double, ggl::cs::cartesian, x, y)

That gives 33 errors with MSVC 8.0.  Instead of tracking it down, I
just included everything from the example program.  That worked, but I
still don't know what is *actually* required to register a custom
point.

It also doesn't appear that the custom point type can be in its own
namespace when using these registration macros.  For instance, the
following doesn't compile:

namespace foo {
struct test {
        double x, y;
};
}
GEOMETRY_REGISTER_POINT_2D(foo::test, double, ggl::cs::cartesian, x, y)

That gives 26 errors, same compiler as before.  Since my own custom
point was in a math namespace, I had to have "using namespace math"
just to register my point correctly.

Once I finally got my math types registered, I went on to using some
of the functions.  distance() worked out of the box and returned the
correct answer, but I then wanted to customize the strategy for
distance().  Using the haversine strategy caused compilation errors
because there was no overload of get_as_radian<>() for my point type.

1. It's not documented that the haversine strategy requires
get_as_radian, or at least I didn't see it.
2. My custom point type already has get<> defined, so why not relax
the requirements and have haversine do something like:

template <int N, typename T>
double get_as_radian(const T &p) {
        return get<N>(p) * constants::pi_over_180();
};

by default?

I finally got haversine working with my point type by defining
get_as_radius as above, and the result was correct.  I then went on to
define my own distance strategy based on the Vincenty distance
formula.

This almost went smoothly, except that I had to specialize
strategy_tag, but didn't see any mention of this requirement in the
docs.  Once I did that, my distance strategy worked and returned the
same value as haversine for values along the equator.

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

5-6 hours reading the documentation and making toy programs using and
extending the library.

> - Are you knowledgeable about the problem domain?

Yes, I am knowledgeable about geometry as it relates to video games,
as well as GIS.

--Michael Fawcett