[vlc-devel] [PATCH 11/18] input: use vlc_viewpoint_from/to_euler
Steve Lhomme
robux4 at ycbcr.xyz
Thu Apr 1 12:19:03 UTC 2021
On 2021-04-01 13:24, Alexandre Janniaux wrote:
> 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.
That sounds very suspicious. Either it's a mathematical limitation and
then there should be a formula to get the actual value or it's an
implementation detail and should be explained as such.
> 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.
If giving a name to a value makes less sense, there is a problem.
>> 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].
Indeed, I don't understand. You're saying a value of 90 cannot be
converted to quarternion even though it does exist in Euler (see
spherical videos specs above), and even if the value can be represented
in quaternion.
> 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
You lost me at "degenerate submanifolds". I do understand any of this text.
>> 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.
I read that. That still doesn't answer my question. From what I
understand you want to use quaternions when doing the math to add new
angles to the viewpoint to avoid the gimbal lock. But that doesn't say
why doing so needs to clip the input values we can change at a time. It
seems like we're replacing one limitation by another.
>>> 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
> _______________________________________________
> 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