wined3d: declaration/FVF conversion test

[Ivan Gyurdiev]

Hi, I'm attaching test, which demonstrates incorrect behavior of SetFVF 
and SetVertexDeclaration.
Windows converts one to the other and backwards (at least partially), 
and we do not such thing - this breaks at least 2 demos (dx9_hlsl_*)

I'm posting it here, because:

- I don't have Windows, and I need someone to try it on machine with 
pixel shader support (preferably 3.0, will need to enable pshaders and 
GLSL registry key). The whole first part of the test checks decl to fvf 
conversions, and they're almost all set to 0 in order to pass on H. 
Verbeet and V. Margolen's setups [ which have no pshaders ]. MSDN has a 
whole page on how to convert to an fvf, and the values there are 
definitely *not* 0, so that's why I'm suspicious.

- Secondly, I am leaving for NYC in 2 days to look for a place to live, 
so I have limited time to clean up the test (ok/trace usage and the 
like), and I have no time to implement the fix. I am hoping someone else 
can finish it, because afterwards I go to Denver for a month, where I 
will have limited computing capabilities. Then I start a full time job 
back at NYC, so I'll be pretty busy. Anyway, I'm sure Jave has shaders 
under control and will be done with sm 2, 3, 4, 5.. 10  by the end of 
the summer whether I help or not :)

[Vitaly Budovski]

Ivan Gyurdiev wrote:
> Hi, I'm attaching test, which demonstrates incorrect behavior of 
> SetFVF and SetVertexDeclaration.
> Windows converts one to the other and backwards (at least partially), 
> and we do not such thing - this breaks at least 2 demos (dx9_hlsl_*)
>
> I'm posting it here, because:
>
> - I don't have Windows, and I need someone to try it on machine with 
> pixel shader support (preferably 3.0, will need to enable pshaders and 
> GLSL registry key). The whole first part of the test checks decl to 
> fvf conversions, and they're almost all set to 0 in order to pass on 
> H. Verbeet and V. Margolen's setups [ which have no pshaders ]. MSDN 
> has a whole page on how to convert to an fvf, and the values there are 
> definitely *not* 0, so that's why I'm suspicious.
>
> - Secondly, I am leaving for NYC in 2 days to look for a place to 
> live, so I have limited time to clean up the test (ok/trace usage and 
> the like), and I have no time to implement the fix. I am hoping 
> someone else can finish it, because afterwards I go to Denver for a 
> month, where I will have limited computing capabilities. Then I start 
> a full time job back at NYC, so I'll be pretty busy. Anyway, I'm sure 
> Jave has shaders under control and will be done with sm 2, 3, 4, 5.. 
> 10  by the end of the summer whether I help or not :)
> Hi,

I've attached a patch which seems to fix the first set of failures. This 
sets FVF values(from converted declarations) which were not set before. 
I'm still getting a lot of 0 results myself. Pretty sure I have 
everything enabled.

Comments?

diff --git a/dlls/wined3d/device.c b/dlls/wined3d/device.c
index e635352..7116d2f 100644
--- a/dlls/wined3d/device.c
+++ b/dlls/wined3d/device.c
@@ -4480,6 +4480,66 @@ static HRESULT WINAPI IWineD3DDeviceImpl
     }
 
     if (NULL != pDecl) {
+        DWORD fvf = 0;
+        D3DVERTEXELEMENT9 * declaration = ((IWineD3DVertexDeclarationImpl *)pDecl)->pDeclaration9;
+        if(D3DDECLTYPE_FLOAT3 == declaration->Type &&
+            D3DDECLUSAGE_POSITION == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = D3DFVF_XYZ;
+        } else if(D3DDECLTYPE_FLOAT4 == declaration->Type &&
+            D3DDECLUSAGE_POSITIONT == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = D3DFVF_XYZRHW;
+        } else if(D3DDECLTYPE_FLOAT1 == declaration->Type &&
+            D3DDECLUSAGE_BLENDWEIGHT == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_XYZB1*/;
+        } else if(D3DDECLTYPE_FLOAT2 == declaration->Type &&
+            D3DDECLUSAGE_BLENDWEIGHT == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_XYZB2*/;
+        } else if(D3DDECLTYPE_FLOAT3 == declaration->Type &&
+            D3DDECLUSAGE_BLENDWEIGHT == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_XYZB3*/;
+        } else if(D3DDECLTYPE_FLOAT4 == declaration->Type &&
+            D3DDECLUSAGE_BLENDWEIGHT == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_XYZB4*/;
+        } else if(D3DDECLTYPE_UBYTE4 == declaration->Type &&
+            D3DDECLUSAGE_BLENDINDICES == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_XYZB5*/;
+        } else if(D3DDECLTYPE_FLOAT3 == declaration->Type &&
+            D3DDECLUSAGE_NORMAL == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_NORMAL*/;
+        } else if(D3DDECLTYPE_FLOAT1 == declaration->Type &&
+            D3DDECLUSAGE_PSIZE == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_PSIZE*/;
+        } else if(D3DDECLTYPE_D3DCOLOR == declaration->Type &&
+            D3DDECLUSAGE_COLOR == declaration->Usage &&
+            0 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_DIFFUSE*/;
+        } else if(D3DDECLTYPE_D3DCOLOR == declaration->Type &&
+            D3DDECLUSAGE_COLOR == declaration->Usage &&
+            1 == declaration->UsageIndex) {
+            fvf = 0/*D3DFVF_SPECULAR*/;
+        } else if(D3DDECLTYPE_FLOAT1 == declaration->Type &&
+            D3DDECLUSAGE_TEXCOORD == declaration->Usage) {
+            fvf = 0/*D3DFVF_TEXCOORDSIZE1(declaration->UsageIndex)*/;
+        } else if(D3DDECLTYPE_FLOAT2 == declaration->Type &&
+            D3DDECLUSAGE_TEXCOORD == declaration->Usage) {
+            fvf = 0/*D3DFVF_TEXCOORDSIZE2(declaration->UsageIndex)*/;
+        } else if(D3DDECLTYPE_FLOAT3 == declaration->Type &&
+            D3DDECLUSAGE_TEXCOORD == declaration->Usage) {
+            fvf = 0/*D3DFVF_TEXCOORDSIZE3(declaration->UsageIndex)*/;
+        } else if(D3DDECLTYPE_FLOAT4 == declaration->Type &&
+            D3DDECLUSAGE_TEXCOORD == declaration->Usage) {
+            fvf = 0/*D3DFVF_TEXCOORDSIZE4(declaration->UsageIndex)*/;
+        }
+        IWineD3DDeviceImpl_SetFVF(iface, fvf);
         IWineD3DVertexDeclaration_AddRef(pDecl);
     }
     if (NULL != oldDecl) {

[Ivan Gyurdiev]

> I've attached a patch which seems to fix the first set of failures. 
> This sets FVF values(from converted declarations) which were not set 
> before. I'm still getting a lot of 0 results myself. Pretty sure I 
> have everything enabled.
>
> Comments?

- If the values really are supposed to be (almost) all 0, then so be 
it... but then there's no point to enumerate them all in the code.
- I was thinking this might go into vertexdeclaration.c instead, as 
vdecl_from_fvf() and vdecl_to_fvf() functions, instead of in the device 
code. In general way too many things are in the device code that 
probably should be distributed into more logical files.

Anyway, thanks for working on this...

[H. Verbeet]

On 17/06/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> Comments?
A few points:
  - Taking only the first element of the declaration into account
seems unlikely to be correct
  - Is there a reason you're using pDeclaration9 instead of
pDeclarationWine? It would be usefull if dx8 applications worked as
well.
  - Let's not dump large amounts of code in
IWineD3DDeviceImpl_SetVertexDeclaration

[Ivan Gyurdiev]

H. Verbeet wrote:
> On 17/06/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
>> Comments?
> A few points:
>  - Taking only the first element of the declaration into account
> seems unlikely to be correct
>  - Is there a reason you're using pDeclaration9 instead of
> pDeclarationWine? It would be usefull if dx8 applications worked as
> well.
>  - Let's not dump large amounts of code in
> IWineD3DDeviceImpl_SetVertexDeclaration
Ditto. I should note that fixing the fvf -> decl conversion seems rather 
important, because the FVF loading code will not work with a vertex 
shader at the moment - as far as I can see this will break 3 more demos, 
so that brings the count of broken demos up to 5 (ASCII, Sketch, 
RollerCoaster, dx9_hlsl_* (2) ). Those demos will set a shader, and then 
call SetFVF() afterwards (but primitiveConvertToStridedData will see 
that shader != NULL and will not do anything)

The current code has 2 routines - primitiveConvertToStridedData (for 
FVF) and primitiveDeclarationConvertToStridedData (for declaration). It 
seems as if the FVF should just be converted to a declaration when it is 
set, and a single function should be used afterwards. I could be wrong 
though, haven't looked at this in great detail yet...

[Ivan Gyurdiev]

Ivan Gyurdiev wrote:
> H. Verbeet wrote:
>> On 17/06/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
>>> Comments?
>> A few points:
>>  - Taking only the first element of the declaration into account
>> seems unlikely to be correct
>>  - Is there a reason you're using pDeclaration9 instead of
>> pDeclarationWine? It would be usefull if dx8 applications worked as
>> well.
>>  - Let's not dump large amounts of code in
>> IWineD3DDeviceImpl_SetVertexDeclaration
> Ditto. I should note that fixing the fvf -> decl conversion seems 
> rather important, because the FVF loading code will not work with a 
> vertex shader at the moment - as far as I can see this will break 3 
> more demos, so that brings the count of broken demos up to 5 (ASCII, 
> Sketch, RollerCoaster, dx9_hlsl_* (2) ). Those demos will set a 
> shader, and then call SetFVF() afterwards (but 
> primitiveConvertToStridedData will see that shader != NULL and will 
> not do anything)
Ok I kind of lied here - those demos will not be fixed since they use 
the DrawPrimitiveUP function, which works differently. However, there 
are certainly demos which follow this pattern:

- setStreamSource[0]
- setVertexShader
- setFVF
- drawPrimitive or drawIndexedPrimitive (not UP variant).

and those currently fail, because that case isn't handled at all in the 
FVF loading code. It thinks this is a multi-stream case, when in fact 
only the 0th stream is used. Jason Green has a demo - 
dx9_hdr_texture_loader, which is broken exactly by this.

Instead of changing the FVF loading code, however, we should just 
translate it to a vertex declaration, and use the decl loading code.

[Stefan Dösinger]

> and those currently fail, because that case isn't handled at all in the
> FVF loading code. It thinks this is a multi-stream case, when in fact
> only the 0th stream is used. Jason Green has a demo -
> dx9_hdr_texture_loader, which is broken exactly by this.
> How about getting rid about the fvf in the device entirely and converting the 
> fvf to a vertex decl in SetFVF? This would make the drawprim code 
> simpler(only vertex declaration to consider) and avoid inconsistency. In 
> GetFVF we convert the declaration back to a FVF(and propably cache the 
> result, but don't use that for rendering).

[Ivan Gyurdiev]

Stefan Dösinger wrote:
> > and those currently fail, because that case isn't handled at all in the
> > FVF loading code. It thinks this is a multi-stream case, when in fact
> > only the 0th stream is used. Jason Green has a demo -
> > dx9_hdr_texture_loader, which is broken exactly by this.
>
> How about getting rid about the fvf in the device entirely and converting the 
> fvf to a vertex decl in SetFVF?
> That's what I was suggesting in the next sentence of that email :)
>  This would make the drawprim code 
> simpler(only vertex declaration to consider) and avoid inconsistency. In 
> GetFVF we convert the declaration back to a FVF(and propably cache the 
> result, but don't use that for rendering).

Well, caching is already accomplished by storing the FVF [ so no need 
for backwards conversion]. Furthermore, the backwards conversion isn't 
very clear at the moment - the tests show that almost all FVF fields are 
0'ed when a declaration is implicitly converted to FVF by windows. I 
have no idea why that is yet...

[H. Verbeet]

On 18/06/06, Ivan Gyurdiev <ivg231(a)gmail.com> wrote:
> Well, caching is already accomplished by storing the FVF [ so no need
> for backwards conversion].
> Pretty much agreed. I think we should keep the FVF around (perhaps
> only in the d3d8/d3d9 part?), but use declarations for everything
> internally in wined3d.

[Jason Green]

On 6/15/06, Ivan Gyurdiev <ivg231(a)gmail.com> wrote:
> Hi, I'm attaching test, which demonstrates incorrect behavior of SetFVF
> and SetVertexDeclaration.
> Windows converts one to the other and backwards (at least partially),
> and we do not such thing - this breaks at least 2 demos (dx9_hlsl_*)
>
> I'm posting it here, because:
>
> - I don't have Windows, and I need someone to try it on machine with
> pixel shader support (preferably 3.0, will need to enable pshaders and
> GLSL registry key). The whole first part of the test checks decl to fvf
> conversions, and they're almost all set to 0 in order to pass on H.
> Verbeet and V. Margolen's setups [ which have no pshaders ]. MSDN has a
> whole page on how to convert to an fvf, and the values there are
> definitely *not* 0, so that's why I'm suspicious.

I just tested this on a Windows box that supports Shader model 2.0,
and all tests came back positive without failures.   So, now we need
to clean up the test so it doesn't report twice the number of errors
and get it applied.  Then, we fix Wine.  ;-)

Jason