Re: [2/2] d3dx9: Implement UpdateSemantics mesh method. (try 2)

[Dylan Smith]

Your patches have dos line endings (i.e. "\r\n") which causes git apply to
fail, although the patch command only gives a warning.


@@ -208,6 +221,12 @@ static DWORD WINAPI
> ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface)
>     TRACE("iface (%p)\n", This);
> +    if (!This->vertex_declaration)
> +    {
> +        WARN("Can't get number of bytes per vertex of an invalid vertex
> declaration.\n");
> +        return E_FAIL;
> +    }

GetNumBytesPerVertex returns a DWORD not an HRESULT, so I wouldn't expect
E_FAIL to ever be returned? Unusual behaviour like this should be covered by
a test.


+    /* An application can pass an invalid declaration to UpdateSemantics
> and
> +     * still expect D3D_OK (see tests). If the declaration is invalid,
> then
> +     * subsequent calls to GetDeclaration, GetNumBytesPerVertex, and
> DrawSubset,
> +     * which use the declaration, will fail.
> +     */

D3DXCreateMesh fails when the declaration contains a non-zero Stream value.
I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise
a test could validate the different behaviour.

You tested a declaration with larger fields, but not with the vertex size
being smaller or greater. (E.g. Try removing the last field, or appending a
field.)

I did some testing and couldn't get GetDeclaration or GetNumBytesPerVertex
to fail on windows.  DrawSubset did seem to fail after UpdateSemantics is
called with a declaration that causes CreateVertexDeclaration to fail. This
suggests that the native implementation probably just stores the
D3DVERTEXELEMENT9 array and calls CreateVertexDeclaration on DrawSubset. You
can probably cache the created vertex declaration and release it on
UpdateSemantics for performance.

[2/2] d3dx9: Implement UpdateSemantics mesh method. (try 2)

[Michael Mc Donnell]

On Fri, Jun 10, 2011 at 8:08 AM, Dylan Smith <dylan.ah.smith(a)gmail.com> wrote:
> Your patches have dos line endings (i.e. "\r\n") which causes git apply to
> fail, although the patch command only gives a warning.

Yeah I think this is because of git-imap was changed to convert the
line endings on the fly. I only get the dos line endings if I
downloaded the patch from my imap folder. Does anyone know how to make
git-imap stop converting the line endings?
> > @@ -208,6 +221,12 @@ static DWORD WINAPI
> > ID3DXMeshImpl_GetNumBytesPerVertex(ID3DXMesh *iface)
> >     TRACE("iface (%p)\n", This);
> > +    if (!This->vertex_declaration)
> > +    {
> > +        WARN("Can't get number of bytes per vertex of an invalid vertex
> > declaration.\n");
> > +        return E_FAIL;
> > +    }
>
> GetNumBytesPerVertex returns a DWORD not an HRESULT, so I wouldn't expect
> E_FAIL to ever be returned? Unusual behaviour like this should be covered by
> a test.

Thanks for catching that one. You're right I added the E_FAIL for
GetNumBytesPerVertex too quickly with out testing. I'll write a test
case for that.
> > +    /* An application can pass an invalid declaration to UpdateSemantics
> > and
> > +     * still expect D3D_OK (see tests). If the declaration is invalid,
> > then
> > +     * subsequent calls to GetDeclaration, GetNumBytesPerVertex, and
> > DrawSubset,
> > +     * which use the declaration, will fail.
> > +     */
>
> D3DXCreateMesh fails when the declaration contains a non-zero Stream value.
> I would expect UpdateSemantics to be as strict as D3DXCreateMesh, otherwise
> a test could validate the different behaviour.

Ok, I'll write a test for that too.
> You tested a declaration with larger fields, but not with the vertex size
> being smaller or greater. (E.g. Try removing the last field, or appending a
> field.)

Yeah I see I only test for larger overlapping fields, but not vertex
declaration sizes being smaller or greater. I'll fix that.
> I did some testing and couldn't get GetDeclaration or GetNumBytesPerVertex
> to fail on windows.  DrawSubset did seem to fail after UpdateSemantics is
> called with a declaration that causes CreateVertexDeclaration to fail. This
> suggests that the native implementation probably just stores the
> D3DVERTEXELEMENT9 array and calls CreateVertexDeclaration on DrawSubset. You
> can probably cache the created vertex declaration and release it on
> UpdateSemantics for performance.

You're right again. GetDeclaration returns the invalid declaration,
which means that it is probably cached like you're describing. I'll
add checks for that too.