[IGSTK-Developers] Re: igstkTransfrom documentation update & Style problems

[Luis Ibanez]

Hi Patrick,

Thanks for checking on the usage of these methods.

I agree with both options.

We should:

A) Remove the SetRotation()/SetTranslation() methods

                OR

B) Rename them to explicitly mention that they clear the
     other component.


I'm slightly incline towards (B).


Ziv: Do you have a preference between these two options ?



     Luis



------------------------------------------------------------------------------------
On 8/6/07, Patrick Cheng <cheng at isis.georgetown.edu> wrote:
>
> Hi Luis and Ziv,
>
> Making good sandwich is important! We need to be careful that we 'NEVER'
> set rotation and translation separately in our code, which will erase
> each other in this case if we follow Luis's solution.
>
> I just did a quick search, at least in Tracker code, which deals with
> all the transform composition, we are using SetTranslationAndRotation()
> all the time.
>
> I am wondering if we should provide
> SetRotation() and SetTranslation() at all?
>
> These methods can be misleading, and I am afraid not many people will
> read the documentation.
>
> A name like:
> SetRotationAndClearTranslation()
> or
> SetTranslationAndClearRotation()
> Will be more explicit name.
>
> Patrick
>
>
>
> Luis Ibanez wrote:
> >
> > Hi Ziv,
> >
> > Again, the problem with setting them separately is that the time stamp
> > is shared.
> >
> > The second setting will override the time stamp of the first setting and
> > the Transform
> > will end up with a Translation component that does not match in time to
> > the Rotation
> > component.
> >
> > E.g. the Translation component may even have expired, and by setting
> > only the
> > Rotation we will extend its validity time artificially.  It is like
> > making a sandwich with
> > fresh cheese and an expired ham.... the cheese may be good, but the
> > total sandwich
> > has gone bad anyways.
> >
> > If the pivot calibration is computing both components separately, then
> > they should only
> > be stored in an igstkTransform when both Translation and Rotation are
> > available, and that
> > should be done using the SetRotationAndTranslation() method.
> >
> >
> >    Luis
> >
> >
> >
> ----------------------------------------------------------------------------
> > On 8/6/07, *Ziv Yaniv* < zivy at isis.georgetown.edu
> > <mailto:zivy at isis.georgetown.edu>> wrote:
> >
> >     Hi Luis,
> >
> >     Setting rotation and translation separately without resetting the
> >     transformation is reasonable behavior. Pivot calibration for example
> >     only sets the translational part of a transformation. The user can
> >     set the rotational part after they get the transformation from the
> >     pivot calibration according to prior knowledge (what Patrick is
> >     doing right now). Of course there are many ways to do this
> >     (composing separate rotation and translation transformations to get
> >     a single transform, or setting the transformation only when you have
> >     both rotation and translation in hand).
> >
> >     In any case I view this as an arbitrary choice and my only
> >     preference is that the documentation and implementation match.
> >
> >
> >                  Ziv
> >
> >
> >
> >
> >     -----Original Message-----
> >     From: "Luis Ibanez" < luis.ibanez at kitware.com
> >     <mailto:luis.ibanez at kitware.com>>
> >     Sent 8/6/2007 12:41:56 PM
> >     To: "'Ziv Yaniv'" < zivy at isis.imac.georgetown.edu
> >     <mailto:zivy at isis.imac.georgetown.edu>>, "IGSTK Developers"
> >     <igstk-developers at public.kitware.com
> >     <mailto:igstk-developers at public.kitware.com>>
> >     Subject: igstkTransfrom documentation update & Style problems
> >
> >     Hi Ziv,
> >
> >     Thanks for updating the documentation of the Transform class:
> >
> >
> http://public.kitware.com/cgi-bin/viewcvs.cgi/Source/igstkTransform.h?root=IGSTK&sortby=date&r2=1.20&r1=1.19
> >      <
> http://public.kitware.com/cgi-bin/viewcvs.cgi/Source/igstkTransform.h?root=IGSTK&sortby=date&r2=1.20&r1=1.19
> >
> >
> >     Unfortunately they introduced several coding style violations.
> >
> http://public.kitware.com/IGSTK/Sites/insight.journal.kitware/KWStyle/20070806-0430-Nightly/BuildError.html
> >      <
> http://public.kitware.com/IGSTK/Sites/insight.journal.kitware/KWStyle/20070806-0430-Nightly/BuildError.html
> >
> >
> >     Also, I'm not sure that the right thing was to modify the
> documentation.
> >     It seems that the SetRotation method actually has the bug of not
> setting
> >     the translation to identity, and the SetTranslation() method has the
> bug
> >     of not setting the rotation to identity.
> >
> >     This should be necessary because the transform has a single
> TimeStamp,
> >     and we assume that it is the same for both Translation and Rotation.
> >
> >     I'll suggest that we put the documentation back, and fix the bug in
> >     the implementation of the methods.
> >
> >     Do you see a reason why not initialized Translation / Rotation may
> >     bring any advantage ?
> >
> >
> >         Thanks
> >
> >
> >            Luis
> >
> >
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > _______________________________________________
> > IGSTK-Developers mailing list
> > IGSTK-Developers at public.kitware.com
> > http://public.kitware.com/cgi-bin/mailman/listinfo/igstk-developers
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/igstk-developers/attachments/20070806/2ed2db4e/attachment-0002.html>