[Insight-developers] Re: Hasty comments on QuadEdgeMesh integration
Luis Ibanez
luis.ibanez at kitware.com
Tue Jan 23 16:19:13 EST 2007
HI Eric, Alex,
We have some additional questions about the QuadEdge classes.
1) The QuadEdges that are allocated in the MakeEdge() method
in the LineCell and in the GeometricalQuadEdge (formerly QEGeom),
...where are they deallocated ?
We couldn't find any calls to the "delete"s that should match
these "new".
We are suggesting to add the deletes to the destructor
of the GeometricalQuadEdge
2) Why is MakeEdge() reimplemented in the LineCell ?
if it derives from the GeometricalQuadEdge and the
method already exist in that class ?
We are suggesting to remove the MakeEdge() method
from the LineCell.
3) Trying to elaborate on the reasons for having the
LineCell to derive from the GeometricalQuadEdge,
we are wondering if you intended the LineCell to
be oriented. Was that part of your motivation ?
We are suggesting to remove the inheritance of
the LineCell from the GeometricalQuadEdge, and
instead have the LineCell keep a pointer to the
first of the 4 QuadEdges that will be created by
the MakeEdge() method. Similar to how the
PolygonCell is doing.
We will appreciate your input regarding these issues,
Thanks
Luis
======================================
On 1/23/07, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>
>
> 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/fb629c9e/attachment.html
More information about the Insight-developers
mailing list