I had tried to submit a patch for that 5 years agohttp://www.winehq.org/pipermail/wine-devel/2009-December/080391.html, but couldn't figure out how to make a patch/test it, and eventually gave up and forgot about it. But recently I remembered about it, and now I want to try again.
So, this time I've prepared a test that can prove that the previous implementation does not calculate the XFORM matrix correctly, and that my patch will fix it.
The test is a VC6 project, you can find it at https://drive.google.com/file/d/0B9PGUhmmnsm1S2VvZzc3WmhBY3c/edit?usp=sharin.... The test is a demo of PlgBlt that I found on CodeProjecthttp://www.codeproject.com/Articles/17712/The-GDI-magic-Rendering-reflections-and-smooth-sha. You can compare how it works on XP and on Wine. For testing, I directly inserted the original Wine implementation of PlgBlt, as well as my patched version, into the VC6 project, and tested on that. This is because there seems to be an additional problem with MaskBlt, which I'll try tackling next.
I have screenshots of what the demo looks using Windows's PlgBlthttp://imgur.com/q8qKGd7, and Wine's PlgBlt http://imgur.com/smYEV6z.
To test it, simply look at this function in the VC6 project:
BOOL MyPlgBlt( HDC hdcDest, const POINT *lpPoint, HDC hdcSrc, INT nXSrc, INT nYSrc, INT nWidth, INT nHeight, HBITMAP hbmMask, INT xMask, INT yMask) { // return MyPlgBltWinePatched(hdcDest, lpPoint, hdcSrc, nXSrc, nYSrc, nWidth, nHeight, hbmMask, xMask, yMask); return MyPlgBltWineUnpatched(hdcDest, lpPoint, hdcSrc, nXSrc, nYSrc, nWidth, nHeight, hbmMask, xMask, yMask); // return PlgBlt(hdcDest, lpPoint, hdcSrc, nXSrc, nYSrc, nWidth, nHeight, hbmMask, xMask, yMask); }
Uncomment the function you want to test. If you need something else (like binaries), just write.
And this is the patch itself. I've tried to keep the changes minimal, with respect to white-space, etc. I've removed a "return FALSE", because I don't calculate a determinant explicitly.
commit c9ca2cab58fa0cba3d6b24cfb2af507fd1c07227 Author: Alexander Almaleh sashoalm@gmail.com Date: Tue May 20 18:33:55 2014 +0300
Fix for the calculation of the XFORM matrix in PlgBlt.
diff --git a/dlls/gdi32/bitblt.c b/dlls/gdi32/bitblt.c index 4750d1b..1de25b9 100644 --- a/dlls/gdi32/bitblt.c +++ b/dlls/gdi32/bitblt.c @@ -992,22 +992,32 @@ BOOL WINAPI PlgBlt( HDC hdcDest, const POINT *lpPoint, HDC hdcSrc, INT nXSrc, INT nYSrc, INT nWidth, INT nHeight, HBITMAP hbmMask, INT xMask, INT yMask) { + /* we need to use floating-point precision when calculating the + * XFORM matrix to avoid loss of precision due to rounding */ + typedef struct tagPOINTLF + { + double x, y; + } POINTLF; + int oldgMode; /* parallelogram coords */ - POINT plg[3]; + POINTLF plg[3]; /* rect coords */ - POINT rect[3]; + POINTLF rect[3]; XFORM xf; XFORM SrcXf; XFORM oldDestXf; - double det; + int ii;
/* save actual mode, set GM_ADVANCED */ oldgMode = SetGraphicsMode(hdcDest,GM_ADVANCED); if (oldgMode == 0) return FALSE;
- memcpy(plg,lpPoint,sizeof(POINT)*3); + for (ii = 0; ii < 3; ii++) { + plg[ii].x = lpPoint[ii].x; + plg[ii].y = lpPoint[ii].y; + } rect[0].x = nXSrc; rect[0].y = nYSrc; rect[1].x = nXSrc + nWidth; @@ -1015,33 +1025,39 @@ BOOL WINAPI PlgBlt( HDC hdcDest, const POINT *lpPoint, rect[2].x = nXSrc; rect[2].y = nYSrc + nHeight; /* calc XFORM matrix to transform hdcDest -> hdcSrc (parallelogram to rectangle) */ - /* determinant */ - det = rect[1].x*(rect[2].y - rect[0].y) - rect[2].x*(rect[1].y - rect[0].y) - rect[0].x*(rect[2].y - rect[1].y); - - if (fabs(det) < 1e-5) - { - SetGraphicsMode(hdcDest,oldgMode); - return FALSE; - }
TRACE("hdcSrc=%p %d,%d,%dx%d -> hdcDest=%p %d,%d,%d,%d,%d,%d\n", hdcSrc, nXSrc, nYSrc, nWidth, nHeight, hdcDest, plg[0].x, plg[0].y, plg[1].x, plg[1].y, plg[2].x, plg[2].y);
/* X components */ - xf.eM11 = (plg[1].x*(rect[2].y - rect[0].y) - plg[2].x*(rect[1].y - rect[0].y) - plg[0].x*(rect[2].y - rect[1].y)) / det; - xf.eM21 = (rect[1].x*(plg[2].x - plg[0].x) - rect[2].x*(plg[1].x - plg[0].x) - rect[0].x*(plg[2].x - plg[1].x)) / det; - xf.eDx = (rect[0].x*(rect[1].y*plg[2].x - rect[2].y*plg[1].x) - - rect[1].x*(rect[0].y*plg[2].x - rect[2].y*plg[0].x) + - rect[2].x*(rect[0].y*plg[1].x - rect[1].y*plg[0].x) - ) / det; + xf.eM11 = -(rect[1].y*plg[2].x+rect[0].y*(plg[1].x-plg[2].x) + -rect[2].y*plg[1].x+plg[0].x*(rect[2].y-rect[1].y)) + /(rect[0].y*(rect[2].x-rect[1].x)-rect[1].y*rect[2].x+ + rect[2].y*rect[1].x+(rect[1].y-rect[2].y)*rect[0].x); + xf.eM21 = (plg[0].x*(rect[2].x-rect[1].x)-plg[1].x*rect[2].x + +plg[2].x*rect[1].x+(plg[1].x-plg[2].x)*rect[0].x) + /(rect[0].y*(rect[2].x-rect[1].x)-rect[1].y*rect[2].x + +rect[2].y*rect[1].x+(rect[1].y-rect[2].y)*rect[0].x); + xf.eDx = -(rect[0].y*(plg[2].x*rect[1].x-plg[1].x*rect[2].x) + +plg[0].x*(rect[1].y*rect[2].x-rect[2].y*rect[1].x) + +(rect[2].y*plg[1].x-rect[1].y*plg[2].x)*rect[0].x) + /(rect[0].y*(rect[2].x-rect[1].x)-rect[1].y*rect[2].x+rect[2].y + *rect[1].x+(rect[1].y-rect[2].y)*rect[0].x);
/* Y components */ - xf.eM12 = (plg[1].y*(rect[2].y - rect[0].y) - plg[2].y*(rect[1].y - rect[0].y) - plg[0].y*(rect[2].y - rect[1].y)) / det; - xf.eM22 = (plg[1].x*(rect[2].y - rect[0].y) - plg[2].x*(rect[1].y - rect[0].y) - plg[0].x*(rect[2].y - rect[1].y)) / det; - xf.eDy = (rect[0].x*(rect[1].y*plg[2].y - rect[2].y*plg[1].y) - - rect[1].x*(rect[0].y*plg[2].y - rect[2].y*plg[0].y) + - rect[2].x*(rect[0].y*plg[1].y - rect[1].y*plg[0].y) - ) / det; + xf.eM12 = -(rect[1].y*(plg[2].y-plg[0].y)+rect[0].y*(plg[1].y + -plg[2].y)+rect[2].y*(plg[0].y-plg[1].y))/(rect[0].y*(rect[2].x + -rect[1].x)-rect[1].y*rect[2].x+rect[2].y*rect[1].x+(rect[1].y + -rect[2].y)*rect[0].x); + xf.eM22 = ((plg[0].y-plg[1].y)*rect[2].x+(plg[2].y-plg[0].y)*rect[1].x + +(plg[1].y-plg[2].y)*rect[0].x) /(rect[0].y*(rect[2].x-rect[1].x) + -rect[1].y*rect[2].x+rect[2].y*rect[1].x+(rect[1].y-rect[2].y) + *rect[0].x); + xf.eDy = (rect[0].y*(plg[1].y*rect[2].x-plg[2].y*rect[1].x) + -rect[1].y*plg[0].y*rect[2].x+rect[2].y*plg[0].y*rect[1].x + +(rect[1].y*plg[2].y-rect[2].y*plg[1].y)*rect[0].x)/(rect[0].y + *(rect[2].x-rect[1].x)-rect[1].y*rect[2].x+rect[2].y*rect[1].x + +(rect[1].y-rect[2].y)*rect[0].x);
GetWorldTransform(hdcSrc,&SrcXf); CombineTransform(&xf,&xf,&SrcXf);
On Tue, May 20, 2014 at 1:39 PM, Alexander Almaleh sashoalm@gmail.com wrote:
I had tried to submit a patch for that 5 years ago, but couldn't figure out how to make a patch/test it, and eventually gave up and forgot about it. But recently I remembered about it, and now I want to try again.
So, this time I've prepared a test that can prove that the previous implementation does not calculate the XFORM matrix correctly, and that my patch will fix it.
The test is a VC6 project, you can find it at https://drive.google.com/file/d/0B9PGUhmmnsm1S2VvZzc3WmhBY3c/edit?usp=sharin.... The test is a demo of PlgBlt that I found on CodeProject. You can compare how it works on XP and on Wine. For testing, I directly inserted the original Wine implementation of PlgBlt, as well as my patched version, into the VC6 project, and tested on that. This is because there seems to be an additional problem with MaskBlt, which I'll try tackling next.
I can confirm your code works in XP. I think the first thing to do is to create a bug to take care of the issue.
Done! Bug submitted at http://bugs.winehq.org/show_bug.cgi?id=36504.
On Wed, May 21, 2014 at 4:12 PM, Bruno Jesus 00cpxxx@gmail.com wrote:
On Tue, May 20, 2014 at 1:39 PM, Alexander Almaleh sashoalm@gmail.com wrote:
I had tried to submit a patch for that 5 years ago, but couldn't figure
out
how to make a patch/test it, and eventually gave up and forgot about it.
But
recently I remembered about it, and now I want to try again.
So, this time I've prepared a test that can prove that the previous implementation does not calculate the XFORM matrix correctly, and that my patch will fix it.
The test is a VC6 project, you can find it at
https://drive.google.com/file/d/0B9PGUhmmnsm1S2VvZzc3WmhBY3c/edit?usp=sharin... .
The test is a demo of PlgBlt that I found on CodeProject. You can compare how it works on XP and on Wine. For testing, I directly inserted the original Wine implementation of PlgBlt, as well as my patched version,
into
the VC6 project, and tested on that. This is because there seems to be an additional problem with MaskBlt, which I'll try tackling next.
I can confirm your code works in XP. I think the first thing to do is to create a bug to take care of the issue.
Hi Alexander
On Tue, May 20, 2014 at 1:39 PM, Alexander Almaleh sashoalm@gmail.com wrote:
I had tried to submit a patch for that 5 years ago, but couldn't figure
out
how to make a patch/test it, and eventually gave up and forgot about it.
But
recently I remembered about it, and now I want to try again.
The easiest way to get your patches accepted, in my experience, is to write an automated test. That way it is easier for everyone to verify that your patch is doing the right thing. We will be able to run it on the Wine Testbot and see it working.
It might be difficult in your case though. Wine does usually not include big test files so you would probably have to write some code that draws a test image. The test image needs to be easy to verify in code if it has been transformed correctly. You might be able to draw a filled rectangle in one hdc and a filled parallelogram in another. Transform the filled rectangle using PlgBlt and then compare it to the parallelogram. I have not thought it all the way through so it might not work :-)
It seems like there are no automated bitblt tests in the wine source tree. The first step would be to add a file bitblt.c in /dlls/gdi32/tests/ and then add it to the Makefile.in file in that directory. My first commit to Wine [1] added a new test file brsfolder.c to shell32.
You might be able to get your patch in without the automated test, but I think you need to explain clearly what was wrong with the old code and what you are doing instead. A few more simplified examples that test other cases in your bug report might also help.
Best of luck, Michael Mc Donnell
[1] http://source.winehq.org/git/wine.git/?a=commit;h=5c715b46f4312a2e76aa14499e...
Hi, Michael
On Thu, May 22, 2014 at 4:56 AM, Michael Mc Donnell michael@mcdonnell.dkwrote:
The easiest way to get your patches accepted, in my experience, is to write an automated test. That way it is easier for everyone to verify that your patch is doing the right thing. We will be able to run it on the Wine Testbot and see it working.
OK, I'll try to write a unit test. I found this tutorial - https://www.winehq.org/docs/winedev-guide/testing, is it the correct thing to read?
You might be able to get your patch in without the automated test, but I think you need to explain clearly what was wrong with the old code and what you are doing instead. A few more simplified examples that test other cases in your bug report might also help.
The trouble is I don't quite remember the exact steps I took 5 years ago to come up with the new formula. However, I do remember I came up to it by solving a system of linear equations. Since XFORM matrix is applied to each point, I took 3-4 measurements of how points are transformed by PlgBlt, and then used that to make equations of the type applyXFORM(xSrc) = xDest. By solving the equations for the XFORM matrix, I came up with the correct formula I need to get the XFORM matrix from the input variables. I used an online linear equation solver, so there's no human error.
I'll try to recreate the steps, maybe it'll be easier to just explain them. I'll look into making automated tests, too.
Hi Alexander
On Thu, May 22, 2014 at 2:25 AM, Alexander Almaleh sashoalm@gmail.comwrote:
OK, I'll try to write a unit test. I found this tutorial - https://www.winehq.org/docs/winedev-guide/testing, is it the correct thing to read?
Yes that is the correct thing to read.
You might be able to get your patch in without the automated test, but I
think you need to explain clearly what was wrong with the old code and what you are doing instead. A few more simplified examples that test other cases in your bug report might also help.
The trouble is I don't quite remember the exact steps I took 5 years ago to come up with the new formula. However, I do remember I came up to it by solving a system of linear equations. Since XFORM matrix is applied to each point, I took 3-4 measurements of how points are transformed by PlgBlt, and then used that to make equations of the type applyXFORM(xSrc) = xDest. By solving the equations for the XFORM matrix, I came up with the correct formula I need to get the XFORM matrix from the input variables. I used an online linear equation solver, so there's no human error.
I'll try to recreate the steps, maybe it'll be easier to just explain them. I'll look into making automated tests, too.
Sounds good. Like you are saying, figuring out the formulas again would be a good idea. The current code does not explain where the formulas are coming from. Adding that as comments in your patch would make it easier to verify than the current code.
Michael
Ok. Btw, I have some other things on my head now, so it might be a week or two before I come back to this.
On Fri, May 23, 2014 at 1:35 AM, Michael Mc Donnell michael@mcdonnell.dkwrote:
Hi Alexander
On Thu, May 22, 2014 at 2:25 AM, Alexander Almaleh sashoalm@gmail.comwrote:
OK, I'll try to write a unit test. I found this tutorial - https://www.winehq.org/docs/winedev-guide/testing, is it the correct thing to read?
Yes that is the correct thing to read.
You might be able to get your patch in without the automated test, but
I think you need to explain clearly what was wrong with the old code and what you are doing instead. A few more simplified examples that test other cases in your bug report might also help.
The trouble is I don't quite remember the exact steps I took 5 years ago to come up with the new formula. However, I do remember I came up to it by solving a system of linear equations. Since XFORM matrix is applied to each point, I took 3-4 measurements of how points are transformed by PlgBlt, and then used that to make equations of the type applyXFORM(xSrc) = xDest. By solving the equations for the XFORM matrix, I came up with the correct formula I need to get the XFORM matrix from the input variables. I used an online linear equation solver, so there's no human error.
I'll try to recreate the steps, maybe it'll be easier to just explain them. I'll look into making automated tests, too.
Sounds good. Like you are saying, figuring out the formulas again would be a good idea. The current code does not explain where the formulas are coming from. Adding that as comments in your patch would make it easier to verify than the current code.
Michael
Hi Michael,
I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows.
Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix?
On Tue, May 27, 2014 at 11:40 PM, Alexander Almaleh sashoalm@gmail.com wrote:
Ok. Btw, I have some other things on my head now, so it might be a week or two before I come back to this.
On Fri, May 23, 2014 at 1:35 AM, Michael Mc Donnell michael@mcdonnell.dk wrote:
Hi Alexander
On Thu, May 22, 2014 at 2:25 AM, Alexander Almaleh sashoalm@gmail.com wrote:
OK, I'll try to write a unit test. I found this tutorial - https://www.winehq.org/docs/winedev-guide/testing, is it the correct thing to read?
Yes that is the correct thing to read.
You might be able to get your patch in without the automated test,
but I think you need to explain clearly what was wrong with the old code and what you are doing instead. A few more simplified examples that test other cases in your bug report might also help.
The trouble is I don't quite remember the exact steps I took 5 years ago to come up with the new formula. However, I do remember I came up to it by solving a system of linear equations. Since XFORM matrix is applied to each point, I took 3-4 measurements of how points are transformed by PlgBlt, and then used that to make equations of the type applyXFORM(xSrc) = xDest. By solving the equations for the XFORM matrix, I came up with the correct formula I need to get the XFORM matrix from the input variables. I used an online linear equation solver, so there's no human error.
I'll try to recreate the steps, maybe it'll be easier to just explain them. I'll look into making automated tests, too.
Sounds good. Like you are saying, figuring out the formulas again would be a good idea. The current code does not explain where the formulas are coming from. Adding that as comments in your patch would make it easier to verify than the current code.
Michael
Hi Alexander
On Wed, Jun 11, 2014 at 5:36 AM, Alexander Almaleh sashoalm@gmail.com wrote:
I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows.
I do not understand how that would stop you from writing an automated test? You can use todo_wine to show how the code is supposed to work. You can then cross compile the test and upload it to the Wine TestBot. The Wine TestBot will run your test on Windows and report back if it is correct. The automated test can then be applied to the wine source tree. Once your patch with the fix is ready you can submit it together with a patch that removes the todo_wine.
Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix?
I think you will need to be able to explain why the old code is wrong and explain why yours is better. Explaining what the formula is might be a good way to *help* explain it. It is important that your code does not break anything. You have found one example where your code works better but it might break other examples.
I see Nikolay Sivov, the author of the function, has already given you feedback earlier [1]. He will probably be one of the people you need to convince. When he said:
"P.S. I don't see your point on inlining determinant expression."
He probably means you should rewrite your equation to use the determinant, unless you have a very good reason, in which case you need to explain it. I personally think you might be able to fix his code instead of replacing it.
Good luck, Michael
[1] http://www.winehq.org/pipermail/wine-devel/2009-December/080392.html
Hi guys,
by comparing the "new" equations (http://www.winehq.org/pipermail/wine-devel/2009-December/080391.html) and the old ones (http://source.winehq.org/source/dlls/gdi32/bitblt.c) you'll find the following differences:
eM11: identical eM21: identical eDx: identical
eM12: identical eM22: plg <-> rect swapped eDy: identical
It isn't obvious at the first look, but the only difference is that in eM22 plg and rect are swapped. By taking a bit closer look at ...
http://source.winehq.org/source/dlls/gdi32/bitblt.c#L1040
... and comparing the equations for the X-component (eM11, eM21) and the Y-component (eM12, eM22) you'll notice that this must be right, the current equation violate the symmetry - no need to calculate, you can see that with the naked eye ;)
The correct equation for eM22 should be:
xf.eM22 = (rect[1].x*(plg[2].y - plg[0].y) - rect[2].x*(plg[1].y - plg[0].y) - rect[0].x*(plg[2].y - plg[1].y)) / det;
Could someone please test if this change alone is sufficient to fix the issue? Should be way easier to get that upstream than all the other "no-op" changes.
Regards, Sebastian
Am 16.06.2014 04:07, schrieb Michael Mc Donnell:
Hi Alexander
On Wed, Jun 11, 2014 at 5:36 AM, Alexander Almaleh sashoalm@gmail.com wrote:
I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows.
I do not understand how that would stop you from writing an automated test? You can use todo_wine to show how the code is supposed to work. You can then cross compile the test and upload it to the Wine TestBot. The Wine TestBot will run your test on Windows and report back if it is correct. The automated test can then be applied to the wine source tree. Once your patch with the fix is ready you can submit it together with a patch that removes the todo_wine.
Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix?
I think you will need to be able to explain why the old code is wrong and explain why yours is better. Explaining what the formula is might be a good way to *help* explain it. It is important that your code does not break anything. You have found one example where your code works better but it might break other examples.
I see Nikolay Sivov, the author of the function, has already given you feedback earlier [1]. He will probably be one of the people you need to convince. When he said:
"P.S. I don't see your point on inlining determinant expression."
He probably means you should rewrite your equation to use the determinant, unless you have a very good reason, in which case you need to explain it. I personally think you might be able to fix his code instead of replacing it.
Good luck, Michael
[1] http://www.winehq.org/pipermail/wine-devel/2009-December/080392.html
Thank you Sebastian for reviewing the code and digging into the equations. Sounds good that it is possibly a small fix.
Alexander can you verify this with your test case?
Thanks, Michael
On Sun, Jun 15, 2014 at 11:59 PM, Sebastian Lackner sebastian@fds-team.de wrote:
Hi guys,
by comparing the "new" equations (http://www.winehq.org/pipermail/wine-devel/2009-December/080391.html) and the old ones (http://source.winehq.org/source/dlls/gdi32/bitblt.c) you'll find the following differences:
eM11: identical eM21: identical eDx: identical
eM12: identical eM22: plg <-> rect swapped eDy: identical
It isn't obvious at the first look, but the only difference is that in eM22 plg and rect are swapped. By taking a bit closer look at ...
http://source.winehq.org/source/dlls/gdi32/bitblt.c#L1040
... and comparing the equations for the X-component (eM11, eM21) and the Y-component (eM12, eM22) you'll notice that this must be right, the current equation violate the symmetry - no need to calculate, you can see that with the naked eye ;)
The correct equation for eM22 should be:
xf.eM22 = (rect[1].x*(plg[2].y - plg[0].y) - rect[2].x*(plg[1].y - plg[0].y) - rect[0].x*(plg[2].y - plg[1].y)) / det;
Could someone please test if this change alone is sufficient to fix the issue? Should be way easier to get that upstream than all the other "no-op" changes.
Regards, Sebastian
Am 16.06.2014 04:07, schrieb Michael Mc Donnell:
Hi Alexander
On Wed, Jun 11, 2014 at 5:36 AM, Alexander Almaleh sashoalm@gmail.com wrote:
I just realized that it would be impossible to write an automated test for PlgBlt now, because Wine has a second, unrelated but, for which I already wrote another email to wine-devel. It's titled "BitBlt doesn't honor the rotation specified by SetWorldTransform". So even with my fixed PlgBlt, Wine's version would still not work correctly, until that second, unrelated bug is fix. That's why I submitted a Windows project where I compare my patched PlgBlt, and Wine's PlgBlt under Windows.
I do not understand how that would stop you from writing an automated test? You can use todo_wine to show how the code is supposed to work. You can then cross compile the test and upload it to the Wine TestBot. The Wine TestBot will run your test on Windows and report back if it is correct. The automated test can then be applied to the wine source tree. Once your patch with the fix is ready you can submit it together with a patch that removes the todo_wine.
Bearing that in mind, what is your advice? Bruno Jesus already wrote that he can confirm my code runs in Windows. Should I just explain how I reached the formula for the XFORM matrix?
I think you will need to be able to explain why the old code is wrong and explain why yours is better. Explaining what the formula is might be a good way to *help* explain it. It is important that your code does not break anything. You have found one example where your code works better but it might break other examples.
I see Nikolay Sivov, the author of the function, has already given you feedback earlier [1]. He will probably be one of the people you need to convince. When he said:
"P.S. I don't see your point on inlining determinant expression."
He probably means you should rewrite your equation to use the determinant, unless you have a very good reason, in which case you need to explain it. I personally think you might be able to fix his code instead of replacing it.
Good luck, Michael
[1] http://www.winehq.org/pipermail/wine-devel/2009-December/080392.html
-
Alexander Almaleh -
Bruno Jesus -
Michael Mc Donnell -
Sebastian Lackner