Coding Question
Yuval Levy
a question to the coding experts (meaning: everybody who knows what I don't
know) out there.
why are there two (different) definitions of rotate in the same namespace
Panorama.h (around line 145 and 158)? where are they implemented? is there
one that should be preferred over the other?
thanks
Yuv
James Legg
> Hi all,
>
> a question to the coding experts (meaning: everybody who knows what I don't
> know) out there.
>
> why are there two (different) definitions of rotate in the same namespace
> Panorama.h (around line 145 and 158)?
It is a function overload: you can rotate the panorama by either
specifying Euler angles (yaw, pitch and roll: 3 floats), or a matrix
which performs the rotation when you multiply vectors by it (with type
Matrix3). The compiler picks the wanted overload by the argument types.
Hopefully they are both used somewhere where either the rotation matrix
or Euler angles are known.
> where are they implemented?
Right there: src/hugin1/PT/panorama.h, lines 149 to 152, and 158 to 161.
However, both use the HuginBase::RotatePanorama class[1], which has an
overloaded constructor: it also handles Euler angles or a rotation
matrix.
> is there one that should be preferred over the other?
The one with the easiest to supply arguments.
If you are writing something for the Hugin GUI, you probably want to use
PT::RotatePanoCmd[2] instead, like this:
GlobalCmdHist::getInstance().addCommand(
new PT::RotatePanoCmd(pano, yaw, pitch, roll));
This adds it to the undo stack and performs the rotation.
PT::RotatePanoCmd could be extended to support a rotation matrix in its
constructor instead of Euler angles.
[1]
http://hugin.sourceforge.net/docs/html/classHuginBase_1_1RotatePanorama.html
[2]
http://hugin.sourceforge.net/docs/html/classPT_1_1RotatePanoCmd.html
Yuval Levy
On September 27, 2010 10:08:09 pm James Legg wrote:
> > where are they implemented?
>
> Right there: src/hugin1/PT/panorama.h, lines 149 to 152, and 158 to 161.
ok, I have used the wrong twem / not expressed myself well. What I see in
those lines (excuse my layman terms) is a convoluted way to pass the arguments
on to the actual implementation.
> However, both use the HuginBase::RotatePanorama class[1], which has an
> overloaded constructor: it also handles Euler angles or a rotation
> matrix.
That's what I was meaning with "implemented".
src/hugin_base/algorithms/basic/RotatePanorama.cpp is what I was looking for.
And I still have to make sense of it.
I see two fuctions in there - one that does the actual calculations (which I
understand and thus can read), and another one that simply put the three Euler
angles into a matrix after converting them to radians. But how exactly does
it work? I mean who calls whom?
> > is there one that should be preferred over the other?
>
> The one with the easiest to supply arguments.
That's an answer I like to hear.
> If you are writing something for the Hugin GUI, you probably want to use
> PT::RotatePanoCmd[2] instead
yes, this I understood. What I ma trying to do (which is probably trivial to
you) is to expand the numeric transform in the fast preview window. Currently
we have rotate (yaw/pitch/roll) - I would like to have translate too (X/Y/Z).
Today I bumped into a project where I would need translation.
My intention (correct me if I am wrong) is to implement PT::TranslatePanoCmd
(pano, trx, try, trz).
To make it similar to HuginBase::RotatePanorama, I will have to implement a
new class, HuginBase::TranslatePanorama ? does it make sense for such a small
functionality? should I put it in its own file TranslatePanorama.cpp or is it
better to add a new one, TranslatePanorama.cpp ?
> PT::RotatePanoCmd could be extended to support a rotation matrix in its
> constructor instead of Euler angles.
or should I extend PT::RotatePanoCmd to support also translation?
> [1]
> http://hugin.sourceforge.net/docs/html/classHuginBase_1_1RotatePanorama.html
yes, the doxygen documentaiton is good to point details, but we still need
some "higher up" description to guide newbies and occasional contributors. A
top down approach that is human enough not to scare people...
Yuv
T. Modes
> My intention (correct me if I am wrong) is to implement PT::TranslatePanoCmd
> (pano, trx, try, trz).
>
> To make it similar to HuginBase::RotatePanorama, I will have to implement a
> new class, HuginBase::TranslatePanorama ? does it make sense for such a small
> functionality? should I put it in its own file TranslatePanorama.cpp or is it
> better to add a new one, TranslatePanorama.cpp ?
>
> > PT::RotatePanoCmd could be extended to support a rotation matrix in its
> > constructor instead of Euler angles.
>
> or should I extend PT::RotatePanoCmd to support also translation?
>
functionality. Extending RotatePanorama would only make sense, if you
would plan to apply a rotation and a translation at the same time.
> > [1]
> >http://hugin.sourceforge.net/docs/html/classHuginBase_1_1RotatePanora...
> yes, the doxygen documentaiton is good to point details, but we still need
> some "higher up" description to guide newbies and occasional contributors. A
> top down approach that is human enough not to scare people...
>
issue is not high on my priority list.
If you add new classes, please add also some documentation.
Thomas
Yuval Levy
On September 28, 2010 02:57:10 am T. Modes wrote:
> I think, put it a new class and a new file because it's new
> functionality. Extending RotatePanorama would only make sense, if you
> would plan to apply a rotation and a translation at the same time.
I doubt anybody will want to apply a rotation and a translation at the same
time. But from a GUI perspective, I intend to put the Y/X/Z next to the
yaw/pitch/roll fields and trigger the transform from the same button, checking
for non-zero values. To avoid history-confusions (half-redo), I will have to
summarize both the rotation and translation as a single PanoCommand.
> > yes, the doxygen documentaiton is good to point details, but we still
> > need some "higher up" description to guide newbies and occasional
> > contributors. A top down approach that is human enough not to scare
> > people...
>
> Yesterday I started to extend the doc. Give me some time, but this
> issue is not high on my priority list.
I understand. Just make sure that choices are being discussed/agreed amongst
contributors on this list, not simply decided by the first one who gets to
edit the doc.
For instance, we had a discussion a while ago about coding style. I still
have the result of that discussion somewhere on my HDD and intended to put it
on the Hugin website. We can put an "executive summary" / introduction to the
documentation on the Hugin website, with the goal of getting the occasional
contributor up to speed as fast as possible with the dos and donts.
> If you add new classes, please add also some documentation.
I always try to document everything I do (e.g. in the Wiki). Maybe not
perfect, but I trust others will give me feedback when they see something I
can do better. Personally I learn a lot about the coding by reading the
committ mails, but I must admit that I am not always very diligent about it,
for lack of time, so sometimes I just look at the committ message, missing on
an opportunity to learn.
Yuv
T. Modes
> I doubt anybody will want to apply a rotation and a translation at the same
> time. But from a GUI perspective, I intend to put the Y/X/Z next to the
> yaw/pitch/roll fields and trigger the transform from the same button, checking
> for non-zero values. To avoid history-confusions (half-redo), I will have to
> summarize both the rotation and translation as a single PanoCommand.
the tab?
But I think, if you apply yaw/pitch and x/y/z to the same time, this
will maybe break the consistancy between the images.
see the implementation of the drag tool and
http://groups.google.com/group/hugin-ptx/msg/74f7adbc86775535
http://groups.google.com/group/hugin-ptx/browse_thread/thread/424c91083d0c0cd9/
So I would propose instead following solution:
If drag mode is set to standard, the input boxes should be for yaw/
pitch/roll (as current).
If drag mode is set to mosaic, the input boxes should switch to x/y/z
and only work on this parameters (or maybe roll/x/y/z with showing/
hiding the 4. box).
Thomas
Yuval Levy
On September 29, 2010 12:09:51 pm T. Modes wrote:
> So I would propose instead following solution:
> If drag mode is set to standard, the input boxes should be for yaw/
> pitch/roll (as current).
> If drag mode is set to mosaic, the input boxes should switch to x/y/z
> and only work on this parameters (or maybe roll/x/y/z with showing/
> hiding the 4. box).
makes sense. will do. It adds a little bit of spice to my exercise: I don't
think I have ever dealt with toggling visibility of input boxes, but I'll find
out how to do it.
Yuv