Re: d3dx9_36: Implement D3DXCreateBox

[Joris Huizer]

Hello,

In this patch, one part seemed strange to me:
You are declaring vertices as static memory:

+    static FLOAT vertices[144]

But you are changing the values during the function:

+
+    for(i = 0; i<  24; i++)
+    {
+        vertices[6 * i    ] *= width;
+        vertices[6 * i + 1] *= height;
+        vertices[6 * i + 2] *= depth;
+    }
+

Given that original values are never restored, I think you can not mark the array as static,
otherwise incorrect values are used after the first call.

The other arrays are not adapted, so they are fine as static memory (And you may in fact mark those static const)

HTH,
Joris

d3dx9_36: Implement D3DXCreateBox

[Joshua Beck]

On 3/27/2011 11:08 AM, Joris Huizer wrote:
> But you are changing the values during the function:
> -
> - for(i = 0; i<  24; i++)
> - {
> -
>    vertices[6 * i    ] *= width;
> -
>    vertices[6 * i + 1] *= height;
> -
>    vertices[6 * i + 2] *= depth;
> - }
> -

On a side note, you could rearrange this loop to use less multiplications:
+    for(i = 0; i<  24; i++)
+    {
+        int sixi = 6 * i;
+        vertices[sixi    ] *= width;
+        vertices[sixi + 1] *= height;
+        vertices[sixi + 2] *= depth;
+    }

Or even better:
+    for(i = 0; i<  144; i+=4)
+    {
+        vertices[i  ] *= width;
+        vertices[++i] *= height;
+        vertices[++i] *= depth;
+    }

d3dx9_36: Implement D3DXCreateBox

[Dmitry Timoshkov]

Joshua Beck  wrote:
> Or even better:
> - for(i = 0; i<  144; i+=4)
> - {
> -
>    vertices[i  ] *= width;
> -
>    vertices[++i] *= height;
> -
>    vertices[++i] *= depth;
> - }

There shouild be i + 1 and i + 2 instead of two ++i otherwise
the loop index gets corrupted.

d3dx9_36: Implement D3DXCreateBox

[Joshua Beck]

On 03/27/2011 10:58 PM, Dmitry Timoshkov wrote:
> Joshua Beck  wrote:
> > Or even better:
> > - for(i = 0; i<   144; i+=4)
> > - {
> > -
> >    vertices[i  ] *= width;
> > -
> >    vertices[++i] *= height;
> > -
> >    vertices[++i] *= depth;
> > - }
>
> There shouild be i + 1 and i + 2 instead of two ++i otherwise
> the loop index gets corrupted.

You mean like this?

+    for(i = 0; i<   144; i+=6)
+    {
+        vertices[i    ] *= width;
+        vertices[i + 1] *= height;
+        vertices[i + 2] *= depth;
+    }

d3dx9_36: Implement D3DXCreateBox

[Dmitry Timoshkov]

Joshua Beck  wrote:
> > > Or even better:
> > > - for(i = 0; i<   144; i+=4)
> > > - {
> > > -
> > >    vertices[i  ] *= width;
> > > -
> > >    vertices[++i] *= height;
> > > -
> > >    vertices[++i] *= depth;
> > > - }
> >
> > There shouild be i + 1 and i + 2 instead of two ++i otherwise
> > the loop index gets corrupted.
>
> You mean like this?
> - for(i = 0; i<   144; i+=6)
> - {
> -
>    vertices[i    ] *= width;
> -
>    vertices[i + 1] *= height;
> -
>    vertices[i + 2] *= depth;
> - }

Looks like I misread the original loop condition. Both your variants
are fine.

d3dx9_36: Implement D3DXCreateBox

[Andrew Nguyen]

On 03/27/2011 04:52 PM, Joshua Beck wrote:
> On a side note, you could rearrange this loop to use less multiplications:
> - for(i = 0; i<  24; i++)
> - {
> -
>    int sixi = 6 * i;
> -
>    vertices[sixi    ] *= width;
> -
>    vertices[sixi + 1] *= height;
> -
>    vertices[sixi + 2] *= depth;
> - }
>
> Or even better:
> - for(i = 0; i<  144; i+=4)
> - {
> -
>    vertices[i  ] *= width;
> -
>    vertices[++i] *= height;
> -
>    vertices[++i] *= depth;
> - }

I find that the second variant you proposed hinders comprehension of the
loop, as Dmitry's responses imply. While the first variant is perfectly
fine, I wanted to point out that in terms of code generation with
optimization enabled, the original and your variants are identical, so
your suggestions don't have any impact on runtime performance of the loop.