[vlc-devel] [PATCH 11/18] input: use vlc_viewpoint_from/to_euler

[Steve Lhomme]

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.

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.

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 ?

> 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(&param.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
>