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.