[Review] We're halfway through the GGL review
Hey all, We're halfway through the GGL review (see attached review announcement) it is planned to end November 15th. Now would be a good time to send in your reviews! Regards Hartmut ------------------- Meet me at BoostCon http://boostcon.com
Hi all, here is my review. I followed all the discussions since your first preview, because I am interested in a good geometry library. I liked your initial design and was really impressed by the cooperative improvement during the iterations. But before going into details, I think that GGL should become a part of boost, so make this a Yes vote.
- What is your evaluation of the design?
I think it is clean and easy to follow. The one exception to this is the way union / intersections are handled, which I have not fully understood yet. I would have expected to be able something along the lines of: polygon p1 = ...; polygon p2 = ...; polygon_set ps1 = make_union(p1, p2); polygon_set ps2; intersection(ps1, p2, ps2); I also don't find it too obvious how the extention to 3d geometries is going to work, yet.
- What is your evaluation of the implementation?
I have only skimmed through it, but it looks quite clean. It is easy to find each entity, and see what they are doing.
- What is your evaluation of the documentation?
On the whole I am very pleased with the documentation, its easy to read and reasonably complete. I have a general problem with doxygen generated docu with regard to generic libraries. It not easy to separate the important classes and fuctions from the implementation details. GGL helps with this through Grouping by modules and a overview Page, but it could be even better. I think the Modules should them self be further grouped, and the most important should be highlighted. Something along the lines of (just examples, no informed reordering): Modules Page ------------ Entities (?) Geometries Coordinate Systems (?) Algorithms (? Functions) distance union ... Utility / Machinery core concepts traits ... And ideally (but I think that it is not feasible in doxygen) better Navigation inside these Groups
- What is your evaluation of the potential usefulness of the library?
I think it is immensely useful to have this in boost. An example like qt_example shows that geometry plays a role even in everyday programming and is not specific to any particular (niche) domain. I think it is important that we have a common vocabulary which scales form there to very complex and involved algorithm. I would love to have it extended to 3d Geometry (especially Polygon/Triangle meshes) but I wouldn't make my vote depend on this. However I think that 3d should become part of the default distribution once it is there.
- Did you try to use the library? With what compiler? Did you have any problems?
Compiling some example and playing around with them a bit. No problems on OS X 10.6 with gcc 4.2.1
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
As stated above reading the Docu, and all communication on this list from the get-go. Glancing at the Implementation, and trying out some examples.
- Are you knowledgeable about the problem domain?
Yes, I work in medical visualisation, and have developed geographical tools while studying. I have used CGAL and (unfortunately) ACIS and evaluated a few others, so I have a good Idea what a c++ geometry library should or should not look like. Kind regards, and great Work Fabio Fracassi
Hi Fabio, Thanks for your interest and for reviewing our library!
polygon p1 = ...; polygon p2 = ...;
polygon_set ps1 = make_union(p1, p2); polygon_set ps2; intersection(ps1, p2, ps2);
The polygon_set is implemented as a multi-polygon within GGL (and indeed you can call it polygon_set, if it fulfils the concept). We didn't merge the Multi* concepts into the generic algorithms, in other words, we don't require anyone to use a multi* (of course algorithms can be used for multi*). Therefore, union and intersection work with output iterators. So: union_inserter(p1, p2, std::back_inserter(ps1)); It is possible that there will be a "intersection" and "union_" as well, detecting if ps1 is a multi-polygon and automatically inserting. But that is more a sort of synonym, similar way to do things. The make_union would require an r-value and if C++0x is there, it can be implemented without copying the whole resultset. Indeed, it fits into the design and naming conventions.
On the whole I am very pleased with the documentation, its easy to read and reasonably complete. I have a general problem with doxygen generated docu with regard to generic libraries.[...]
I understand. Several people have addressed this so we will certainly improve the documentation. Thanks for your structure proposal. Regards, Barend
Barend Gehrels wrote:
Hi Fabio,
Thanks for your interest and for reviewing our library!
polygon p1 = ...; polygon p2 = ...;
polygon_set ps1 = make_union(p1, p2); polygon_set ps2; intersection(ps1, p2, ps2);
The polygon_set is implemented as a multi-polygon within GGL (and indeed you can call it polygon_set, if it fulfils the concept). We didn't merge the Multi* concepts into the generic algorithms, in other words, we don't require anyone to use a multi* (of course algorithms can be used for multi*).
Ok I read about multi* in the docu, but didn't make the connection. I also find the ggl/multi/... includes somewhat surprising. Maybe you can use them in one of the examples, to make things clearer.
Therefore, union and intersection work with output iterators. So: union_inserter(p1, p2, std::back_inserter(ps1));
Hm, I tried: #include <boost/ggl/ggl.hpp> #include <boost/ggl/geometries/cartesian3d.hpp> #include <boost/ggl/multi/geometries/multi_polygon.hpp> #include <boost/ggl/algorithms/union.hpp> ... typedef multi_polygon<polygon_3d> polygon_set_3d; polygon_set_3d ps; polygon_3d poly1; polygon_3d poly2; union_inserter(poly1, poly2, std::back_inserter(ps)); but got this error: error: no matching function for call to 'union_inserter(ggl::polygon_3d&, ggl::polygon_3d&, std::back_insert_iterator<ggl::multi_polygon<ggl::polygon<ggl::point<double, 3ul, ggl::cs::cartesian>, std::vector, std::vector, true, std::allocator, std::allocator>, std::vector, std::allocator> >)' Am I doing something wrong?
It is possible that there will be a "intersection" and "union_" as well, detecting if ps1 is a multi-polygon and automatically inserting. But that is more a sort of synonym, similar way to do things.
Yes, I know just a bit of syntactic sugar, but IMO it makes the interface much easier to use. Besides I would probably not call it union_ but geometric_union or some such, but thats just personal preference.
The make_union would require an r-value and if C++0x is there, it can be implemented without copying the whole resultset. Indeed, it fits into the design and naming conventions.
Of course, still I like this syntax best, as it is most natural.
On the whole I am very pleased with the documentation, its easy to read and reasonably complete. I have a general problem with doxygen generated docu with regard to generic libraries.[...] I understand. Several people have addressed this so we will certainly improve the documentation. Thanks for your structure proposal.
Fine, while I was further investigating ggl docu I found another wish: For each class/function add a short usage example including all relevant includes (something short like the snipplet above) This makes it immediately clear how things work together. Regards, Fabio
Hi Fabio,
Maybe you can use them in one of the examples, to make things clearer.
Sure, that makes sense. We'll do that.
Therefore, union and intersection work with output iterators. So: union_inserter(p1, p2, std::back_inserter(ps1));
Hm, I tried:
#include <boost/ggl/ggl.hpp> #include <boost/ggl/geometries/cartesian3d.hpp> #include <boost/ggl/multi/geometries/multi_polygon.hpp> #include <boost/ggl/algorithms/union.hpp>
...
typedef multi_polygon<polygon_3d> polygon_set_3d; polygon_set_3d ps;
polygon_3d poly1; polygon_3d poly2;
union_inserter(poly1, poly2, std::back_inserter(ps));
There are two things: - I forgot one thing, sorry, the inserter-versions always have their output type as a required template parameter. That one can, AFAIK, not be deduced from the output iterator. And it might be a different type of the input polygons. If someone knows if that is possible somehow, I would be happy to know. So it should be: union_inserter<polygon_2d>(p1, p2, std::back_inserter(ps1)); - You're using the 3d polygons here. The union would make a 2D union-operation then of your 3D polygons (so polygons with for all vertices a height). It would ignore the third dimension (because it is not a polyhedron union). I just tried it, to not give you again wrong information. But it is not working yet like this, a.o. because the functions convert, area and point-in-polygon which should support 3D then. Especially area implementation would probably be different for 3D. Though it is used as a helper-function in the union, so its usage could be avoided.
Besides I would probably not call it union_ but geometric_union or some such, but thats just personal preference.
Yes, but it is already in the namespace ggl. We thought that union_ conforms to boost/mpl conventions (not_, if_)
For each class/function add a short usage example including all relevant includes (something short like the snipplet above) This makes it immediately clear how things work together.
That makes also sense, thanks. Regards, Barend
Barend Gehrels wrote:
Hi Fabio,
Hm, I tried:
#include <boost/ggl/ggl.hpp> #include <boost/ggl/geometries/cartesian3d.hpp> #include <boost/ggl/multi/geometries/multi_polygon.hpp> #include <boost/ggl/algorithms/union.hpp>
...
typedef multi_polygon<polygon_3d> polygon_set_3d; polygon_set_3d ps; polygon_3d poly1; polygon_3d poly2; union_inserter(poly1, poly2, std::back_inserter(ps));
There are two things:
- I forgot one thing, sorry, the inserter-versions always have their output type as a required template parameter. That one can, AFAIK, not be deduced from the output iterator. And it might be a different type of the input polygons. If someone knows if that is possible somehow, I would be happy to know. So it should be: union_inserter<polygon_2d>(p1, p2, std::back_inserter(ps1));
Ok, one more argument for both an example and a union_ function then ;)
- You're using the 3d polygons here. The union would make a 2D union-operation then of your 3D polygons (so polygons with for all vertices a height). It would ignore the third dimension (because it is not a polyhedron union). I just tried it, to not give you again wrong information. But it is not working yet like this, a.o. because the functions convert, area and point-in-polygon which should support 3D then. Especially area implementation would probably be different for 3D. Though it is used as a helper-function in the union, so its usage could be avoided.
I didn't really expect 3d union to work (i.e. create a polyhedron ) I wanted to find out what would happen. I'm not quite sure what I would expect (projection on the xy plane like you suggest or another projection most likely). But the docu said that 3d was not a focus of GGL yet, so its ok that there are missing pieces. I'm content that the design allows for both union_(poly3d_1, poly3d_2, poly3d_result) and union_(poly3d_1, poly3d_2, polyhedron) to do the right thing, once the building blocks are in place. Thanks and Regards Fabio
Hi,
I also don't find it too obvious how the extention to 3d geometries is going to work, yet.
There are several cases: 1) Some concept/algorithm pairs are already fully dimension-agnostic, like point/distance: you can ask the distance between two 2D points, two 3D points, two 15D points if you want. 2) Some others are only handled in 2D. For them, the appropriate algorithms will have to be extended with 3D versions (or agnostic versions) of the algorithm. 3) Some 3D-related concepts don't even exist yet (fundamental ones, like planes, or structured ones, like meshes). They will have to be created (the *concepts*, not only one of their model) and appropriate algorithms can then be implemented. So to make things clear, when we say "GGL can be later extended for 3D and other stuff", we're not stating that everything will magically fit into case 2). Rather that we can rely on case 3) when needed to add virtually a infinity of other geometry stuff without breaking consistency. Regards Bruno
participants (4)
-
Barend Gehrels -
Bruno Lalande -
Fabio Fracassi -
Hartmut Kaiser