[v1] mesa: rotation of 0-vector

Submitted by Sergii Romantsov on Sept. 12, 2018, 1:30 p.m.

Details

Message ID 1536759035-30661-1-git-send-email-sergii.romantsov@globallogic.com
State New
Headers show
Series "mesa: rotation of 0-vector" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Sergii Romantsov Sept. 12, 2018, 1:30 p.m.
Specification doesn't define behaviour for rotation of 0-vector.
But khronos.org says that vector has to be normalized.
As workaround assumed that for 0-vector x-position will be
defined as 1.0f.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100960
Signed-off-by: Sergii Romantsov <sergii.romantsov@globallogic.com>
---
 src/mesa/main/matrix.c | 5 +++++
 1 file changed, 5 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/main/matrix.c b/src/mesa/main/matrix.c
index 8065a83..631b203 100644
--- a/src/mesa/main/matrix.c
+++ b/src/mesa/main/matrix.c
@@ -415,6 +415,11 @@  _mesa_Rotatef( GLfloat angle, GLfloat x, GLfloat y, GLfloat z )
 
    FLUSH_VERTICES(ctx, 0);
    if (angle != 0.0F) {
+      /* khronos.org says that ||( x,y,z )|| = 1 (if not, the GL will normalize this vector)
+      * So that is kind of workaround for empty-vectors.
+      * */
+      if (x == 0 && y == 0 && z == 0)
+         x = 1.0f;
       _math_matrix_rotate( ctx->CurrentStack->Top, angle, x, y, z);
       ctx->NewState |= ctx->CurrentStack->DirtyFlag;
    }

Comments

On 09/12/2018 07:30 AM, Sergii Romantsov wrote:
> Specification doesn't define behaviour for rotation of 0-vector.
> But khronos.org says that vector has to be normalized.
> As workaround assumed that for 0-vector x-position will be
> defined as 1.0f.
> 
> Bugzilla: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.freedesktop.org%2Fshow_bug.cgi%3Fid%3D100960&amp;data=02%7C01%7Cbrianp%40vmware.com%7Cfde139ec0f4448b8090d08d618b3f0ce%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636723558454036605&amp;sdata=IAlt%2FkpHJyb5JHRYaOTfhf5v9V7Xl5iQBNBdqe2PHQs%3D&amp;reserved=0
> Signed-off-by: Sergii Romantsov <sergii.romantsov@globallogic.com>
> ---
>   src/mesa/main/matrix.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/src/mesa/main/matrix.c b/src/mesa/main/matrix.c
> index 8065a83..631b203 100644
> --- a/src/mesa/main/matrix.c
> +++ b/src/mesa/main/matrix.c
> @@ -415,6 +415,11 @@ _mesa_Rotatef( GLfloat angle, GLfloat x, GLfloat y, GLfloat z )
>   
>      FLUSH_VERTICES(ctx, 0);
>      if (angle != 0.0F) {
> +      /* khronos.org says that ||( x,y,z )|| = 1 (if not, the GL will normalize this vector)
> +      * So that is kind of workaround for empty-vectors.
> +      * */

Comment style should be:

/* khronos.org says ...
  * So that ...
  */

You might also note the bug number in the code in case anyone wonders 
what app would hit this.


> +      if (x == 0 && y == 0 && z == 0)
> +         x = 1.0f;

You may as well use 0.0f in the test too, just to be sure no silly 
int/float/double conversion happens.

>         _math_matrix_rotate( ctx->CurrentStack->Top, angle, x, y, z);
>         ctx->NewState |= ctx->CurrentStack->DirtyFlag;
>      }
> 

-Brian
On 13/9/18 12:44 am, Brian Paul wrote:
> On 09/12/2018 07:30 AM, Sergii Romantsov wrote:
>> Specification doesn't define behaviour for rotation of 0-vector.
>> But khronos.org says that vector has to be normalized.
>> As workaround assumed that for 0-vector x-position will be
>> defined as 1.0f.
>>
>> Bugzilla: 
>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.freedesktop.org%2Fshow_bug.cgi%3Fid%3D100960&amp;data=02%7C01%7Cbrianp%40vmware.com%7Cfde139ec0f4448b8090d08d618b3f0ce%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C0%7C636723558454036605&amp;sdata=IAlt%2FkpHJyb5JHRYaOTfhf5v9V7Xl5iQBNBdqe2PHQs%3D&amp;reserved=0 
>>
>> Signed-off-by: Sergii Romantsov <sergii.romantsov@globallogic.com>
>> ---
>>   src/mesa/main/matrix.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/src/mesa/main/matrix.c b/src/mesa/main/matrix.c
>> index 8065a83..631b203 100644
>> --- a/src/mesa/main/matrix.c
>> +++ b/src/mesa/main/matrix.c
>> @@ -415,6 +415,11 @@ _mesa_Rotatef( GLfloat angle, GLfloat x, GLfloat 
>> y, GLfloat z )
>>      FLUSH_VERTICES(ctx, 0);
>>      if (angle != 0.0F) {
>> +      /* khronos.org says that ||( x,y,z )|| = 1 (if not, the GL will 
>> normalize this vector)
>> +      * So that is kind of workaround for empty-vectors.
>> +      * */
> 
> Comment style should be:
> 
> /* khronos.org says ...
>   * So that ...
>   */

Thanks for looking into the bug. However the api docs shouldn't be 
quoted. They are unversioned, are at times wrong and should not be used 
as a definative source when making changes or implementing features. 
Please only quote and use the spec for such things.

> 
> You might also note the bug number in the code in case anyone wonders 
> what app would hit this.
> 
> 
>> +      if (x == 0 && y == 0 && z == 0)
>> +         x = 1.0f;

If the spec doesn't define behavior we should probably file a bug 
against the compatibility spec rather than assuming things. Clearly 
recording the problem (including applications it appears in), specifying 
the current behaviors in other drivers and proposing updated wording to 
the spec is usually the fastest way to resolve this type of thing.

Also if this were the correct change it should be made in 
_math_matrix_rotate() this change will make some of the existing code 
there unreachable.

Finally it would be a good idea to have a piglit test for this once any 
spec clarification is done.

> 
> You may as well use 0.0f in the test too, just to be sure no silly 
> int/float/double conversion happens.
> 
>>         _math_matrix_rotate( ctx->CurrentStack->Top, angle, x, y, z);
>>         ctx->NewState |= ctx->CurrentStack->DirtyFlag;
>>      }
>>
> 
> -Brian
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Hello,
thanks for looking on that.
Will update after any answer on
https://github.com/KhronosGroup/OpenGL-API/issues/41
Hope that is correct place for such things.

On Thu, Sep 13, 2018 at 4:57 AM, Timothy Arceri <tarceri@itsqueeze.com>
wrote:

> On 13/9/18 12:44 am, Brian Paul wrote:
>
>> On 09/12/2018 07:30 AM, Sergii Romantsov wrote:
>>
>>> Specification doesn't define behaviour for rotation of 0-vector.
>>> But khronos.org says that vector has to be normalized.
>>> As workaround assumed that for 0-vector x-position will be
>>> defined as 1.0f.
>>>
>>> Bugzilla: https://na01.safelinks.protection.outlook.com/?url=https%3A%
>>> 2F%2Fbugs.freedesktop.org%2Fshow_bug.cgi%3Fid%3D100960&
>>> amp;data=02%7C01%7Cbrianp%40vmware.com%7Cfde139ec0f4448b
>>> 8090d08d618b3f0ce%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%
>>> 7C0%7C636723558454036605&amp;sdata=IAlt%2FkpHJyb5JHRYaOTfhf5
>>> v9V7Xl5iQBNBdqe2PHQs%3D&amp;reserved=0
>>> Signed-off-by: Sergii Romantsov <sergii.romantsov@globallogic.com>
>>> ---
>>>   src/mesa/main/matrix.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/src/mesa/main/matrix.c b/src/mesa/main/matrix.c
>>> index 8065a83..631b203 100644
>>> --- a/src/mesa/main/matrix.c
>>> +++ b/src/mesa/main/matrix.c
>>> @@ -415,6 +415,11 @@ _mesa_Rotatef( GLfloat angle, GLfloat x, GLfloat y,
>>> GLfloat z )
>>>      FLUSH_VERTICES(ctx, 0);
>>>      if (angle != 0.0F) {
>>> +      /* khronos.org says that ||( x,y,z )|| = 1 (if not, the GL will
>>> normalize this vector)
>>> +      * So that is kind of workaround for empty-vectors.
>>> +      * */
>>>
>>
>> Comment style should be:
>>
>> /* khronos.org says ...
>>   * So that ...
>>   */
>>
>
> Thanks for looking into the bug. However the api docs shouldn't be quoted.
> They are unversioned, are at times wrong and should not be used as a
> definative source when making changes or implementing features. Please only
> quote and use the spec for such things.
>
>
>> You might also note the bug number in the code in case anyone wonders
>> what app would hit this.
>>
>>
>> +      if (x == 0 && y == 0 && z == 0)
>>> +         x = 1.0f;
>>>
>>
> If the spec doesn't define behavior we should probably file a bug against
> the compatibility spec rather than assuming things. Clearly recording the
> problem (including applications it appears in), specifying the current
> behaviors in other drivers and proposing updated wording to the spec is
> usually the fastest way to resolve this type of thing.
>
> Also if this were the correct change it should be made in
> _math_matrix_rotate() this change will make some of the existing code there
> unreachable.
>
> Finally it would be a good idea to have a piglit test for this once any
> spec clarification is done.
>
>
>
>> You may as well use 0.0f in the test too, just to be sure no silly
>> int/float/double conversion happens.
>>
>>         _math_matrix_rotate( ctx->CurrentStack->Top, angle, x, y, z);
>>>         ctx->NewState |= ctx->CurrentStack->DirtyFlag;
>>>      }
>>>
>>>
>> -Brian
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>