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

Subject: Re: [Boost-users] [Review] GGL review starts today, November 5th
From: Michael Fawcett (michael.fawcett_at_[hidden])
Date: 2009-11-23 12:05:44

On Sat, Nov 21, 2009 at 6:57 AM, Barend Gehrels <barend_at_[hidden]> wrote:
>
> 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)

Sorry for reporting the wrong name, you guessed right.  It would have
added more work for sure, delaying the review, but seeing two
libraries operate seamlessly together is always nice.  I hope this is
planned.

> However, I'll come back to you because it would be very useful if there is
> an example, showing integration between GGL and ESRI.

If you need any help with this, let me know.  The licenses can be very
costly, so it's probably not easy to test.

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

Ah, it looked like a broken hyperlink since it was bolded, and usually
Doxygen auto-links.  My mistake, but it would be nice to just jump to
the Examples from there rather than scrolling all the way back up.

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

No, you are right.  I went back to my test programs and must have
jumped to that conclusion when I was having all the other compilation
issues.

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

Is it only in certain cases when other dependencies are necessary?  I
went back and tried this again, and indeed it doesn't compile for me.
This is with MSVC 8.0, SP1.

> 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 did use cartesian, so I was definitely misusing the library, but
every now and then I think people will want to misuse the library.  It
should be possible (even if it's harder) and documented I think.
Either that, or have an "unpecified" coordinate system type.

Basically, think of a use-case where I have hundreds of thousands of
points, where I want to display them in "Cartesian" coordinates on
screen in an Orthographic projection and need both pixel-space and
geodesic distance between points.  Do I have to make a copy of every
point before one of the operations, or can I coax the distance
strategy into doing different things even though my point type can
only be registered in one coordinate system (currently)?

> 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, the documentation was obviously "enough" since I did get it
all working in the end, but it wasn't as smooth as using other boost
libraries has been for me.  Keep up the good work!

--Michael Fawcett