Hi People, Here is my own review of the library (NOT as a review manager but as a reviewer) I won't state now whether I think it should or should not be accepted to avoid the appearance of a conflict of interest between the two roles. Naturally, what I think in that regard will weight into the final decision, so I'll state so when I post the veredict. Below is a list of points that have been mostly not mentioned before, so have in mind that I am implicitely agreeing with many of the issues raised. [it's 2:30 AM so this goes unedited, hope it makes sense]
- What is your evaluation of the design?
Overall, it is good, whith a more or less reasonable concept-oriented API. Here are some specific points I dislike: (*) I really don't like the use of overloading-on-concept as the main API element. Specially implemented via SFINAE as it is. IMO, a much better design would be to a have basic traits that provides the fundamental building blocks, as the library is doing, for example via "point_traits<>", but then the derived functions be also provided as static members of a template class, say, point_concept<> This is much better for a number of reasons: A user can specialize the functions for their own data types in a snap. Currently is next to impossible. For instance, having the compiler call my own assign() for my own point type, when it happens to be a model of a mutable point, can be very very tricky. A design based on many layers of specialzable classes is WAY more compiler friendly and hence much more easy for the programmer to spot mistakes such as constrains violations (wrong type) without concept checking. Then, at the front end of the API, I would provide the overloaded versions as there are now as a top layer. This is in fact what the library is doing for those fundamental functions that are given directly by the traits class, but IMO it needs to do the same with all free functions, dispatching the actual implementation to the right specialization of the concept (i.e. free fucntions would be merely forwarding) Finally, the library itself wouldn't use the overloaded free functions but the "real" member functions. This would make the code much more robust in the presence of ADL. Right now, there are countless places where unqualified calls to, say, assign(), set(), even "abs()", WILL cause ambiguities in many real world contexts. (*) The library needs to distinguish lvalue-ness from write-ness: it is one thing to "reset" the X ccordinate of a point and one totally diferent is to get a non-const reference (lvalue) to it. This general lack of "mutation" as opposed to "assignment" causes for example gross inefficiency in the operations that modify the vertices of a polygon (such as convolve, scale, transform, etc): The functions that implement that, generate a new container with the modified points and then resets the point sequence within the input/output polygon. IMO this is nonesense (and a showstopper from a review POV). yet is it really just the conequence of lacking "lvalue" access to the vertices WITHIN a polygon. Generating a new container would only make sense if the operation would return a new polygon instead of modifying the one in place. (*) In fact, that most opertations modify their arguments instead of producing new ones is odd, and in some cases just plain wrong (when the reference parameter is not even an input value) (*) The library is specifically 2D yet that fact isn't encoded in the namespace nor the identifies. This should be fixed as otherwise will conflict with future 3D libraries. (*) The library names something as "orientation". IMNSHO, in the context of polygos, orientation means something else (that which the library call direction instead). So, I would strongly suggest the rename it, to something like: axis_alignment (which is what it is). (*) some enums are named after "BLABLA_enum" and some after "BLABLA". that should be consistent... it would remove _enum (*) There is a lowercase "winding_direction" enum whith values like "clockwise_winding" and "counterclock_winding" (plus unknown), but then there is the "direction_1d_enum" with values of "CLOCKWISE" and "COUNTERCLOCKWISE" Not only that is confusing but it also boils down to inefficient code because the "winding" function returns CLOCKWISE|COUNTERCLOCKWISE yet the "polygon_traits<>::winding" code that is the actual implementation returns winding_direction. Since the actual numeric values mismatch there is an explicit conditional convertion: return w == clockwise_winding = CLOCKWISE : COUNTERCLOCKWISE (*) In fact, the problem above comes from the design of the fucntion winding(): if the actualt winding cannot be computed, it iself computes the area instead and uses its sign to second guess the winding. IMO, that alternative calculation should be done by the user if and when it makes sense. (*) In fact again, the problem above comes from the design of the function area(): there is a "point_sequence_area" that does the actual calculation and returns a signed value. But then area() returns the ABS of that, so a user would not be able to call area() as a robust way to compute the winding of a polygon as, well, winding() internally does. IMO this is a mistake: the area() method should return the signed value and let the user do the abs() WHEN and IF makes sense. (*) I don't think it makes sense to name "point_data<>" with the "_data" postfix. I understand "_traits" etc... but point should be just point IMO. (*) I dislike that there is scale and move not being merely convenience warppers of transform() (also there). And I *really* dislike the existence of a scale_up() + scale_down() which are BOTH there simply because they take "integer" factors instead of a fraction (whether a rational number or a floating point). Also, scale down *secretly* uses a "rouding_policy", instead of taking it as argument. IMO, there should be just scale(), which to add confusion already is but is really an "applicator" that takes a an scaling function object doing whatever and applying thee point's coordinate into it. Which is the same transform() does. But, anyway, IMO there should be just one "scale" taking a floating point factor, or if you prefer, a rational number (expressed as separate numerator and denominator integer arguments for instance). The rounding policy would be given as optional aregument. There is also the transformation<> (along with the transform() function), and the isotropic_scale_factor<>, which are naturally related to these scaling operators. IMO, these is overegineered and leaves the user with too many choices.
- What is your evaluation of the implementation?
Most implementation problems, such as unqualified calls to ubiqutuous functions, are a consequence of the design. Some others are jut details, like the implementation of polygon_data<>::operator == NOT using the point concepts for comparison (it uses operator== on the point tpe) There are nitpicking details all over that I won't enumerate, but some are really important: (*) isotropy.hpp sould be renamed like "common.hpp" or such since it is actually a super header defining lots of stuff totally unrelated to isotropy. (*) Likewise, polygon.hpp should be renamed since it is really the mother header of the library, including all others. I can imagine that it is called "polygon.hpp" since that's the name of the library which I expected to find on it stuff specifically related to a polygon data type or concept or such. (*) Many classes have "inline" in the method definitions. This is completely unnecesary since in-class definitions are inline already. IMO this needs to be removed. (*) euclidean_distance() casts to double just because it needs to call a sqrt(). This is just wrong and IMO is a showstopper that totally gets in the way of reparameterizing the library with a user defined number type. At the very least, the cast should be removed and let ADL + implicit conversion do the right thing (there is no sqrt() for integer types, so). But even this would be wrong, though somewhat acceptable. The only real solution IMO is to ask the user to provide the right kind of "numeric field type" to correlate with the "numeric ring type" (which the integer coordinate is) AND ask the user to provide the right Sqrt for that number type. All that via coordinate_traits. But I wouldn't do any of that. Instead I would have the library provide ONLY squared_euclidean_distance, which would return an integer value, and let the user deal with the sqrt(). This would mean that "perimeter" would need to go of course, which I'm totally OK with. Of course, this pushes the "right way" (IMNSHO) to the user, which is a differnt game. At least remove the (double) cast. (*) There are some structs wrapping enums to make them type safe. That's useful, but then the structs store the enum value as int (or unsigned int depending on where you look), while a char would be enough. (*) In polygon_traits.hpp there are #includes "in the middle" of the header, after some initial code. IMO this is wrong and should be fixed. If that preceding code was needed before the includes then it should be factored out in a separate header. (*)
- What is your evaluation of the documentation?
As many others said it really needs a good introduction that a newcomer can read to see what the library is, and perhaps more importantly, is NOT, about. Also some tutorials, both on using the library out of the box with its own models and on using it on user-defined types. And as some reviwers said, the documentation is way too focus on concepts and its needs to document models as well. In fact, I think most "junior" users won't be interested at all in adapating their own types but on just using the library with the types provided, so the documentation for the models should come first, and the concepts should go after in the chapter discussing adaptation.
- What is your evaluation of the potential usefulness of the library?
I think it is decently useful even if restricted to polygon set operations in 2D and with integral coordinates. I personally don't see the lack of floating-point support particularly disturbing, but of course, I really think the name should be adjusted. The same goes for being 2D only. In the big picture this is important because there are points and polygons in 3D and this library is specifically 2D (it CANNOT just be extended to cover 3D)
- Did you try to use the library? With what compiler? Did you have any problems?
No, I haven't attempted to use it.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I made a very carefull in-deep study of the code.
- Are you knowledgeable about the problem domain?
Yes. -- Fernando Cacciola SciSoft Consulting, Founder http://www.scisoft-consulting.com