WineD3D patch submission

Mitchell Wheeler mitchell.wheeler at gmail.com
Sat Sep 22 18:08:53 CDT 2007


Stefan Dösinger wrote:
> Hello Mitchell,
>
> First of all, thank you for your effort in improving Wine. People who pick 
> their favorite game(s) and fix bugs is exactly what we need these days :-)
>
> You should really get used to git for submitting patches. It requires a tiny 
> bit of learning effort in the beginning, but it pays off very soon because it 
> makes handling patches much easier.
>
>   
>> Anyways, in my attempt to get it working I fixed up some issues w/
>> "IWineD3DDeviceImpl_UpdateSurface.c" in dlls/wined3d, not entirely sure
>> how you guys do things around here so I thought i'd just post my changes
>> to the function in this mailing list and you can do with it what you
>> want.  (Note: it actually has a few different methods of doing one
>> thing, enclosed in macro blocks to toggle between them.  Technically the
>> first one should work (i think :\) once you guys properly implement
>> surface locking/unlocking, and it's 'simplest', but the last one is the
>> only one that actually works properly (using standard OpenGL calls).
>>     
> I looked at the diff Mirek supplied. You should add an exact description what 
> your modifications do because "i also fixed the existing code that didn't 
> actually do what it was supposed to, heh" does not say much. From reading the 
> code it seems that you implement support for the source rectangle. Your patch 
> is mixed with formatting changes, which is not good. Formatting changes 
> should be separate patches, if really needed.
>
> As for the code, the memcpy codepath should not be in UpdateSurface. If a 
> system memory copy is needed, IWineGDISurface_Blt should be called, and the 
> support for partial rectangles of compressed surfaces added there. Otherwise, 
> gl(Compressed)TexSubImage2D should be used for performance reasons.
>
> As for your memcpy() codepath where you said lockrect is broken: What you are 
> trying here will not work. You have to take the pitch into account when 
> accessing the data returned from lockrect, the data is not in one continuous 
> block.
>   
Thanks for the feedback, I didn't really expect you'd keep the first two 
codepaths, so all of that is understandable.
(Also I could be wrong about this as it was a while ago - but I think 
when I tried the lock/unlock rect codepath, the destination surface 
never got re-uploaded to the GPU, though I didn't really understand how 
it was supposed to work (I've never touched DirectX before, I'm an 
OpenGL person myself) so could very well be mistaken)

As for the commenting/reformatting, I didn't actually intend to submit 
this path (it was a last minute "hmm, they could probably do with this 
functionality" - hence I got a little 'trigger happy' and ended up 
rewriting half the function despite not having to, and not making a 
diff, nor going down the git line).  I'm very familiar with subversion, 
and git looks relatively similar to use - so if I do end up contributing 
to wine again I'll be sure to put in the extra effort.

Thanks again for the feedback, and who knows, you may see more patches 
from me in the future (when I find another game that doesn't work 
properly and a free weekend).

Cheers.





More information about the wine-devel mailing list