Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009

Subject: Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009
From: Michael Fawcett (michael.fawcett_at_[hidden])
Date: 2009-09-02 13:46:38

On Mon, Aug 24, 2009 at 4:36 PM, Fernando
Cacciola<fernando.cacciola_at_[hidden]> wrote:
> Dear Developers,
>
> The formal review of the Boost.Polygon library by Lucanus Simonson starts
> today, August 24, 2009 and will finish September 2, 2009.
>
> - What is your evaluation of the design?

Decent, but not sufficiently generic.  All toy examples worked and
simple data types worked, but it quickly broke down as soon as I
started trying to integrate this into a real world program with
complex custom data types.

> - What is your evaluation of the implementation?

Didn't look.

> - What is your evaluation of the documentation?

It is in sore need of good Intro and Tutorial sections.  The concepts
and interfaces are fairly well documented.

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

This is hard to say.  Being restricted to integer coordinates may be
fine for VLSI and PCB layout, but I work in GIS and games, and the
lack of floating point support is a showstopper.  It seems very niche,
which is not to say it should be rejected solely for that, just that I
won't be using it.

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

Yes, I used the library with MSVC9.  Compiling the examples gave
floods of warnings, some seem very real.

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

I spent a day integrating this with a well known third-party API -
http://www.esri.com/software/arcgis/arcgisengine/index.html

> - Are you knowledgeable about the problem domain?

Well, I'm knowledgeable about polygons and boolean operations on them.
 I am not knowledgeable about VLSI and PCB, so I cannot say whether or
not this library is useful.  I firmly believe that if this library is
to be accepted, it needs to further restrict itself, both in code and
documentation, to the stated domain (VLSI/PCB).

This library is unusable for CSG and GIS.

Now, on to specific problems -

Example programs are not fully generic.  They have int hard-coded in a
few places that actually causes compilation errors when a custom point
type is used with a coordinate type other than int.

- The following lines in custom_point.cpp should not be hardcoded to int

        boost::polygon::transformation<int>
tr(boost::polygon::axis_transformation::SWAP_XY);
        boost::polygon::transformation<int> tr2 = tr.inverse();

should be:

        typedef typename boost::polygon::point_traits<Point>::coordinate_type
coordinate_type;
        boost::polygon::transformation<coordinate_type>
tr(boost::polygon::axis_transformation::SWAP_XY);
        boost::polygon::transformation<coordinate_type> tr2 = tr.inverse();

- Making the above changes and getting custom_point.cpp to compile
results in 66 warnings

- All occurrences of <int> in custom_polygon.cpp need to be replaced with
        typedef typename boost::polygon::point_traits<Point>::coordinate_type
coordinate_type;
        <coordinate_type>

-Just including boost/polygon.hpp and specializing the point_traits
struct generates 53 warnings.

- polygon_set_data::get and others dispatch on container type, but is
this really preferred over an output iterator?  This makes it very
difficult to use types that are not allowed to be stored in STD
library containers without an adaptor, which also breaks all the
template specializations.

- I couldn't use STD Library containers to fulfill PolygonSet Concept
because _com_ptr_t can't be held in them since it overloads the
address of operator.  Wrapping in CAdapt is how one gets around that,
but that breaks all the template specializations for point_traits and
polygon_traits.  Maybe not a library deficiency, but it's a high
barrier to entry just to get started with this library.

- polygon_45_set_view is declared as both struct and class

- I'm not sure what the value_types of the input iterators are in
polygon_set_mutable_traits::set and polygon_mutable_traits::set are.
I thought they should have been my custom polygon type and my custom
point type respectively, but it appears they are
std::pair<std::pair<boost::polygon::point_data<double>,boost::polygon::point_data<double>>,int>
and boost::polygon::point_data<double> respectively.  If you're not
going to provide implicit conversions or even explicit conversions,
aren't you exposing quite a few implementation details here when the
goal is to inter-operate with custom types?  In fact, why does
point_data exist?  Shouldn't you just be using the Point concept to
access my custom type instead of copying out of my custom type into
point_data, then forcing me to copy it back into my custom type?

- Copying/converting data to integer coordinates is a showstopper.
Can we use point_traits to get around this?  Multiplying in get, and
dividing in set?  What about construct?  I'm not sure if this will
work...regardless it's very unfortunate that this library is so niche
that integer coordinates are considered sufficient.  I won't be using
this for GIS.

My vote is No, unfortunately, but that is coming from someone who
needs a polygon library suitable for GIS work.  I have no interest in
VLSI/PCB, so perhaps that means my vote doesn't count for much.  If
the library is clearly restricted to the integer domain and
VLCI/PCB/CAD with the documentation updated to reflect that, I still
wouldn't vote Yes, but I would be happier about it being included in
Boost since it looks like the Yes votes outweigh the No votes
currently.

Despite all the criticisms, I really appreciate all the work that Luke
has done, and Intel for sponsoring the work.  I'm sure that others
will find it useful.  I hope I have not discouraged you, Luke.

--Michael Fawcett