On Mon, Aug 24, 2009 at 4:36 PM, Fernando Cacciola<fernando.cacciola@gmail.com> 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