[Insight-developers] Re: Hasty comments on QuadEdgeMesh integration

[Luis Ibanez]

Hi Erick,

Thanks for your input regarding the refactoring of the QuadEdgeMesh classes.

Here are the current issues

1)  Operations in the QuadEdgeMesh are implemented in Function classes
     That derive from FunctionBase.  We see your point on externalizing the
methods
      from the QuadEdgeMesh, that's certainly compatible with the general
ITK design.
     In your paper you explain that you preferred to use Functions instead
of Filters
     in order to save memory. That is, to make changes in-place, as opposed
to
     creating a copy of the mesh as output to a filter. That's a reasonable
argument.
     I would suggest that we keep the function classes but we also offer
them in the
     form of filters that allow to support const-correctness. Using the
data-pipeline is
     also compatible with the VTK design.


2)   QuadEdgeMeshLineCell deriving from the GeometricQuadEdge.
      This is a bit of a conceptual problem. I would suggest that we remove
the
      inheritance from the QuadEdge, and replace it with the LineCell having
a
      reference to the first GeometricQuadEdge of the local ring. The
LineCell
      may provide a method such as (for example) GetFirstQuadEdge(), that
      will return a reference to the QuadEdge, and from that QuadEdge we
could
      continue navigating the local topology.
      From the design point of view (and even the topology) it seems more
      appropriate to say that :

             "The LineCell *has* a reference to a QuadEdge"

      than to say that

            "The LineCell *is* a QuadEdge"

      in particular, considering that the LineCell is the equivalent of the
      "Physical Edge".

     The template parameter of the GeometricQuadEdge,
     still allows you to associate data with the QuadEdge.


3)   Error management

      There is not enough error management in the code.
      For example, there are methods implemented as

              ->GetRot()->GetRot()->GetRot()

      that rely on the assumption that none of the intermediate
      calls return a null pointer.

      We are moving all these methods from the header file
      into the .cxx file, and inserting error management in them.

     The basic principle is that no method should crash.
     We can do error management by returning  null pointers
     in the intermediate cases, or by throwing exceptions.

      The unit test should be such that any method could be
      called in any order, without making the class crash.



4)    Coding style: Method renaming.

     We will expand the current abridged names of methods, and
     for reference we will leave the original name in the Doxygen
     documentation, so that users can refer to the terms used in
     the paper.



Please let us know what you think of these changes,


     Thanks


         Luis


====================================
On 1/18/07, Eric Boix <eric.boix at ens-lyon.fr> wrote:
>
>   Dear Luis [et al.],
>
> Luis Ibanez wrote:
> > 2) The SanityCheck function was renamed [...snip...]
> >    The decision of removing the base class of
> >    FunctionBase was based on that you don't
> >    really use these methods into classes that
> >    expect functions as inputs. But maybe we
> >    just didn't see enough use cases of this
> >    class.
> OK we got the point and we fully agree with you.
> As for topology checker, we agree that each internal test / value should
> be
> accessible independently. We let you free to decide how. This will most
> surely lead to splitting the actual evaluate method into sub-methods with
> a finer granularity.
>
> Still we have a question:
> our initial design (the reason why we thought inheriting from
> itkFunctionBase was a good idea in the first place) is that the
> current TopologyChecker.Evaluate() method was initially a method of
> the QuadEdgeMesh class. When the QuadEdgeMesh class started to grow out of
> control, we decided to externalize several methods. We still wanted to
> keep a
> strong tie between those classes and the QuadEdgeMesh class they were
> initially belonging to. We thought inheriting from itkFunctionBase was the
> right way to express this relationship. Hence here is the question:
> what do you think would be the best way to enforce this coupling
> between those classes and the QuadEdgeMesh class?
>
>
> > 4) We have a conceptual problem,
> >    and we will appreciate your input:
> >
> >      the QuadEdgeMeshLineCell (formerly QELineCell)
> >      has a double derivation from the TCellInterface
> >      and the additional trait QuadEdgeType...
> >      that ... after tracking it down... it seems to
> >      be that it always have to be a GeometricQuadEdge
> >      (formerly a QEQuadEdgeGeom).
> >
> >      We avoid double inheritance as much as we can,
> >      so we will like to get rid of this one.
> >
> >      It seems strange to us that only the LineCell
> >      had this double derivation and the Polygonal
> >      Cell didn't.
> >
> >      Is it because the line Cell is your geometrical
> >      analogous to an "Edge" ?
> >
> >      Why there are no more Cells ?
> This is THE question at the base of the QE design. Leonardo Florez who
> realized this
> design engaged himself to make a complete and detailed answer later on
> today (Colombian
> time). Leonardo recently branched the QE cvs repository precisely to
> address this
> issue (and closely related ones) but his code in under process right now
> and as far
> as we know there is no release date announced.
>
> In the meantime we will provide some hints:
>
> - In the current itk design, the data we can associate with all the cells
> is
>    uniquely defined, whereas we would like to have one kind of data for
> the
>    edge (let s say, length and dihedral angle) and another type on the
> cells
>    (surface, ...).
> - QE design is based on edges; we decided to force the design to reflect
> this
>    one to one relation by creating the QEGeom layer on the edges and not
> on the
>    other cells. The QEGeom layer allows us to add whatever data we want on
> the
>    edges, independently from the one defined at the TCellInterface for All
>    cells.
> - Let us illustrate that: for a real-world Edge we have:
>     * 1 itkQE:LineCell
>     * 2 QEGeom (primal/dual)
>     * 4 QE
>    and the relationships are the following:
>     * a itkQE::LineCell IS a QEGeom (actually it is always the PRIMAL
> QEGeom,
>       who has a link to the QE layer).
>     * a itkQEPolygonCell HAS a direct link to the QE layer.
>    Thus the two of them are different, and this why itkQE::LineCell has
> two
>    inheritance (one for compliance with itk style, one to link with QE
> design).
>    We do not differenciate between all other 2-dimensional cell types.
>
> Enventually, QE is edge centric with an itk compatibility.
>
> > 5) Another conceptual problem arise with the definition
> >    of the QuadEdgeMeshPoint (formerly QEPoint) and its
> >    relationship with the GeometricalQuadEdge.  It seems
> >    that you anticipate a QuadEdgeMeshPoint to be associated
> >    to a single GeometricalQuadEdge, and a Geometrical
> >    QuadEdge to be associated to two point (origin and
> >    destination).
> >
> >    We were expeting a QuadEdgeMeshPoint to be associated
> >    to multiple edges. E.g. the point could be the origin
> >    of multiple edges, not only of one.
> Unlike the VTK/ITK design where a table of all the links from one point to
> all
> the cells using it is build, in QEMesh, from a given point you can access
> an
> "entry edge" which is an arbitrary edge in the edge ring. You can then
> walk
> around the edge ring. QE design guaranties you that the edge ring is
> ordered
> consistently with the orientation and local 2-manifoldness. Walking over
> the
> edge ring, you can then do many things, including getting one ring points,
> cells, etc.
> This is illustrated by the attached drawing, where QuadEdgeMeshPoint can
> be identified with the "Point Primal Data" square box (and the same is
> true
> for the dual edges).
>
> > 6) A Coding Style issue:
> >      ITK does not use acronyms or abreviations.
> >   [....]
> >    For example:
> >       Onext()   should be    GetOriginRingNext()
> The reason why we chose this naming was to stick to the original paper and
> CGAL did the same choice. Of course you can rename those methods but
> we suggest you leave the original name somewhere (either as a comment or
> in
> the doxygenation) for easier reference to the original paper by Guibas
> and Stolfi.
>
>   Yours,
>   Alexandre and Eric and Leonardo.
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://www.itk.org/mailman/private/insight-developers/attachments/20070123/96fde8ab/attachment.htm