[vlc-devel] [PATCH 11/18] input: use vlc_viewpoint_from/to_euler
Alexandre Janniaux
ajanni at videolabs.io
Thu Apr 1 11:24:36 UTC 2021
Hi,
On Thu, Apr 01, 2021 at 12:45:35PM +0200, Steve Lhomme wrote:
> On 2021-04-01 10:51, Alexandre Janniaux wrote:
> > Hi,
> > On Thu, Apr 01, 2021 at 08:02:10AM +0200, Steve Lhomme wrote:
> > > On 2021-03-31 11:25, Alexandre Janniaux wrote:
> > > > From: Alexandre Janniaux <alexandre.janniaux at gmail.com>
> > > >
> > > > ... and also clip pitch to avoid singularities in locations where a
> > >
> > > It looks like a commit title that was cut by GitHub.
> >
> > Not really, github doesn´t wrap at 38 characters. I just
> > didn´t duplicate the short commit message because it brings
> > no value here.
> >
> > > > relative viewpoint update is used. Otherwise, conversions between
> > > > viewpoints and euler angles might lead to corrupted values and blinking
> > > > pitch angles.
> > > > ---
> > > > src/input/input.c | 22 ++++++++++++++++++----
> > > > 1 file changed, 18 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/src/input/input.c b/src/input/input.c
> > > > index 69552bc3e5..b134d4f097 100644
> > > > --- a/src/input/input.c
> > > > +++ b/src/input/input.c
> > > > @@ -1758,6 +1758,10 @@ static void ControlNav( input_thread_t *p_input, int i_type )
> > > > if( b_viewpoint_ch && viewpoint_updated )
> > > > {
> > > > + /* We need to clip the pitch here to avoid singularity poles, otherwise
> > > > + * the next vlc_viewpoint_to_euler might reverse the pitch. */
> > > > + pitch = VLC_CLIP(pitch, -85.f, 85.f);
> > >
> > > These arbitrary values should have a name. And why not use -90, 90 ? In
> > > Matroska you can have pitch values of -90 to 90, both included. Same for MP4
> > > [1]. Does a pitch of 270 turns to 85 ?
> > >
> > > [1] https://github.com/google/spatial-media/blob/master/docs/spherical-video-v2-rfc.md
> >
> > They are arbitrary values, so well, it´s quite hard to name them.
>
> If they are used there must be a reason, that reason should be in the name.
>
> > They are basically the closest values to the singularity points
> > where conversion to/from euler angles doesn´t explode. This is
> > explained in the description of the commit.
>
> I see an explanation on possible consequences, why 85 is the proper value.
> In fact I doubt it's a proper value. 90 sounds more like it would be a
> mathematical limit. But then this value should be checked when the math will
> fail, not on any incoming values.
Because that´s explained in the comment above, and that´s a
hand-tested value obtained by checking when the math fails.
I can use an additional MAX_PITCH/MIN_PITCH constant but even
then this value depends on the rotation convention used in the
computation so it doesn´t even directly make sense except for
the sake of giving a name.
> More generally you are preventing the input from going to 90 degrees pitch
> which might be the user's intention. If the quaternions can't handle such a
> position then I don't see any use for them. If they do, this clipping should
> not happen at all.
You don´t understand the issue here, quaternions can represent this
orientation. But Euler angles (and Euler quaternions by isomorphism)
cannot and never could [1].
The issue happens in situation where you actually do conversion from
and to euler angles.
[1]: https://math.stackexchange.com/questions/8980/euler-angles-and-gimbal-lock#8984
> If you have a headset/HMD and you move your head up, do you think it's a
> good idea to stop following the movement if it goes above 85 degrees
> vertically ? And then restart moving the image when it's over 95 degrees ?
I refer you to the cover letter:
Euler angles have singularity issues, which in the convention
we chosed happens at the north and south poles. It didn't matter
when using usual yaw/pitch controllers since there are not a lot
of way to do this, but it definitively matters when the orientation
is already described by a quaternion. In particular, phone devices
or HMD headset with gyroscopes can easily reach the north and south
poles while having a different roll-like rotation, so can trigger
those bugs.
Using quaternions from the start won't trigger those issues in
cases where the control is not done through the euler-angle-like
paradigm, while still allowing clients to use it as long as they
handle the problem themselves.
Since hotkeys and input were not handling the problem themselves
and were supplying relative transformation, such relative rotations
are clipped to avoid the poles.
> > We can support pitch with singularity values only if there is no
> > singularities, meaning only if we don´t have roll, but it would
> > mean that we´d have multiple conversion function for the two
> > convention used and flag to indicate which convention is in use.
> >
> > Note also that this is the input code for handling controls and
> > for relative updates of the viewpoint, so it has nothing to do
> > immediately with the original pose. It´s also converting from
> > Euler quaternions so initial pitch values don´t really make
> > sense at this point, a pitch of 270 doesn´t exist in this
> > convention since it would alias other pitch/yaw values.
> >
> > This patch does change the previous behaviour, since previous
> > behaviour was not clipped. But because there was no conversion
> > before it could just work as long as the user didn´t change roll.
> >
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > > > +
> > > > priv->viewpoint_changed = true;
> > > > vlc_viewpoint_from_euler( &priv->viewpoint, yaw, pitch, roll );
> > > > ViewpointApply( p_input );
> > > > @@ -2148,10 +2152,20 @@ static bool Control( input_thread_t *p_input,
> > > > else
> > > > {
> > > > priv->viewpoint_changed = true;
> > > > - priv->viewpoint.yaw += param.viewpoint.yaw;
> > > > - priv->viewpoint.pitch += param.viewpoint.pitch;
> > > > - priv->viewpoint.roll += param.viewpoint.roll;
> > > > - priv->viewpoint.fov += param.viewpoint.fov;
> > > > + float previous[3], update[3];
> > > > + vlc_viewpoint_to_euler(&priv->viewpoint, &previous[0],
> > > > + &previous[1], &previous[2]);
> > > > + vlc_viewpoint_to_euler(¶m.viewpoint, &update[0],
> > > > + &update[1], &update[2]);
> > > > +
> > > > + /* We need to clip the pitch here to avoid singularity poles,
> > > > + * otherwise the next vlc_viewpoint_to_euler might reverse
> > > > + * the pitch. */
> > > > + vlc_viewpoint_from_euler(&priv->viewpoint,
> > > > + previous[0] + update[0],
> > > > + VLC_CLIP(previous[1] + update[1], -85.f, 85.f),
> > > > + previous[2] + update[2]);
> > > > + priv->viewpoint.fov += param.viewpoint.fov;
> > > > }
> > > > ViewpointApply( p_input );
> > > > --
> > > > 2.31.0
> > > >
> > > > _______________________________________________
> > > > vlc-devel mailing list
> > > > To unsubscribe or modify your subscription options:
> > > > https://mailman.videolan.org/listinfo/vlc-devel
> > > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list