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

Subject: Re: [boost] Formal Review: Boost.Polygon starts today August 24, 2009
From: Simonson, Lucanus J (lucanus.j.simonson_at_[hidden])
Date: 2009-09-09 13:20:57

Fernando Cacciola wrote:
> (*)
> 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. 

This is a trade-off that I made intentionally.  The rationale for not allowing references to member data of geometry objects is that a geometry object may not have such a data member.  For example, a polygon may store its first point as a point and then subsequent points as offset pairs from the previous point, a rectangle may be represented as its center plus width and height and a point may be in polar coordinates convertible to cartesian in the traits specialization.  My own manhattan polygon data type stores only one coordinate per vertex and has no point to get a non-const reference to.  In this case I sacrificed performance for generality.

> (*)
> 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 was trying to make providing the traits easier.  Having a trait for winding is an optimization.  I was trying to make it easy for users to specify that trait by returning that the winding of their polygon is not known at compile time.  I expect the return of negative values from area() would be more surprising to the user than the fact that I created a special enum for use by the winding trait.

> (*)
> 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. 

I should probably break it into two header files.
 
> 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.

Yes, I struggled with the decision to cast to double to use the sqrt() function in cmath.  I think adding sqrt(coordinate_type) to the coordinate traits is a good suggestion. 
 
> (*)
> 
> 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. 

Actually my intention was to split it into two header files eventually.  I just hadn't done so yet.

Thanks,
Luke