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

Subject: Re: [boost] [Boost-users] [Review] GGL review starts today, November 5th
From: Barend Gehrels (barend_at_[hidden])
Date: 2009-11-21 07:12:44

Hi Michael,

Thanks for your review!

I don't understand some of the problems you have had, so I'll explain or 
ask some things.

> 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 think you mean is_radian, which is used internally to know if 
spherical coordinates are in degree's or in radian's. We didn't thought 
it a good idea, just for this one piece, to integrate with another boost 
library. Though for some coordinate systems it certainly would make 
sense (see  also answer to Brandon's review)

> 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.  
>   
Thanks for your namespace suggestion in another thread. This will help 
to improve this.

>
> 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.
>   

Yes, it would be very cool if this would work nicely, and it is meant to 
work. It is not clear to me if you get anything working, or nothing. 
However, I'll come back to you because it would be very useful if there 
is an example, showing integration between GGL and ESRI.

> The Examples link at the bottom of the main page of documentation is
> broken (last line), 

This was not a hyperlink (could be though, sorry)

> and custom_point_example.cpp doesn't compile -
> cs::cartesian needs to be ggl::cs::cartesian.
>   

This surprises me. It is declared within a macro, so ggl namespace is 
not necessary there. I've not heard of this problem before, and it seems 
to compile fine normally for everyone (so I must now say: for most 
people). Are you sure?

> 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:
>   

Normally, including <ggl/ggl.hpp> is enough, but indeed this should also 
compile, will be add the dependency.

> 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)
> [...] I had to have "using namespace math" just to register my point correctly.
>   

Yes, that was a known issue and should have been documented. We didn't 
search for the solution yet, it must be simple.

> 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.
>   
Spherical coordinate systems use get_as_radian internaly (using 
is_radian), but it should never be necessary to overload it yourself. 
The normal procedure is registering your point as cs::spherical<degree> 
coordinate system. That would have been fine, and that was expected for 
the haversine function. You probably used cs::cartesian, and it didn't 
know if it was degree or radian.
Anyway, I see your problem though, we will correct this and/or document 
this better.

> I finally got haversine working with my point type by defining
> get_as_radius as above, and the result was correct.  
I must say, it is a great workaround, nice that you got it working also 
like that.

> 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.
>   
To be complete, the vincenty is also part of the gis extension, it was 
first not included, but later we wanted to show that 07 graph example, 
showing nicely the integration between BGL and GGL (there were not too 
many comments on this though).
I understand that you implemented your own strategy, very cool. And 
indeed, you should specialize your strategy_tag, if (and only if) you 
want to use that one by default. Another way would have been  that you 
specified the strategy as additional parameter.

Great you got it all working like this! Of course, everything should 
work, but you did use some quite advanced features of GGL, and I like to 
see that you did succeed there. We will work on the documentation so 
these scenario's will cause less problems.

Thanks again, Barend