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

On Thu, Nov 5, 2009 at 9:19 AM, Hartmut Kaiser 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 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 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