[PATCH v8 0/2] MR4165: gdiplus: Fix transformation in GdipIsOutlineVisiblePathPoint
-- v8: gdiplus: Fix transformation in GdipIsOutlineVisiblePathPoint gdiplus/test: Add test for GdipIsOutlineVisiblePathPoint https://gitlab.winehq.org/wine/wine/-/merge_requests/4165
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> --- dlls/gdiplus/tests/graphicspath.c | 142 ++++++++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index 908ca8bcaf0..074311c71a0 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -1883,6 +1883,147 @@ static void test_isvisible(void) ReleaseDC(0, hdc); } +static void test_is_outline_visible_path_point(void) +{ + BOOL result; + GpBitmap *bitmap; + GpGraphics *graphics = NULL; + GpPath *path; + GpPen *pen = NULL; + GpStatus status; + static const int width = 20, height = 20; + + /* Graphics associated with an Image object.*/ + status = GdipCreateBitmapFromScan0(width, height, 0, PixelFormat32bppRGB, NULL, &bitmap); + expect(Ok, status); + status = GdipGetImageGraphicsContext((GpImage *)bitmap, &graphics); + expect(Ok, status); + ok(graphics != NULL, "Expected the graphics context to be initialized.\n"); + + status = GdipCreatePath(FillModeAlternate, &path); + expect(Ok, status); + + status = GdipAddPathRectangle(path, 2.0, 0.0, 13.0, 15.0); + expect(Ok, status); + + status = GdipCreatePen1((ARGB)0xffff00ff, 3.0f, UnitPixel, &pen); + expect(Ok, status); + ok(pen != NULL, "Expected pen to be initialized\n"); + + /* With NULL pen */ + result = 9; + status = GdipIsOutlineVisiblePathPoint(path, 0.0, 1.0, NULL, graphics, &result); + expect(InvalidParameter, status); + expect(9, result); + + /* Without transformation */ + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 0.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 1.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 10.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 16.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 17.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + + /* Translating */ + status = GdipTranslateWorldTransform(graphics, 50.0, 50.0, MatrixOrderPrepend); + expect(Ok, status); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 10.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 15.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 16.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + + /* Scaling */ + status = GdipScaleWorldTransform(graphics, 2.0, 2.0, MatrixOrderPrepend); + expect(Ok, status); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 0.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 1.0, 1.0, pen, graphics, &result); + expect(Ok, status); + todo_wine expect(FALSE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 2.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 3.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 14.0, 1.0, pen, graphics, &result); + expect(Ok, status); + todo_wine expect(FALSE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 15.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(TRUE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 16.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + + /* Page Unit */ + GdipResetWorldTransform(graphics); + status = GdipSetPageUnit(graphics, UnitMillimeter); + expect(Ok, status); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 0.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 1.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 2.0, 1.0, pen, graphics, &result); + expect(Ok, status); + todo_wine expect(TRUE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 3.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 14.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + result = FALSE; + status = GdipIsOutlineVisiblePathPoint(path, 15.0, 1.0, pen, graphics, &result); + expect(Ok, status); + todo_wine expect(TRUE, result); + result = TRUE; + status = GdipIsOutlineVisiblePathPoint(path, 16.0, 1.0, pen, graphics, &result); + expect(Ok, status); + expect(FALSE, result); + + GdipResetWorldTransform(graphics); + GdipDeletePath(path); + GdipDeleteGraphics(graphics); +} + static void test_empty_rect(void) { GpPath *path; @@ -1976,6 +2117,7 @@ START_TEST(graphicspath) test_widen(); test_widen_cap(); test_isvisible(); + test_is_outline_visible_path_point(); test_empty_rect(); GdiplusShutdown(gdiplusToken); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4165
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> --- dlls/gdiplus/graphicspath.c | 18 ++++++------------ dlls/gdiplus/tests/graphicspath.c | 8 ++++---- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c index 8e436f96f4e..54234ea0bd6 100644 --- a/dlls/gdiplus/graphicspath.c +++ b/dlls/gdiplus/graphicspath.c @@ -1728,9 +1728,10 @@ GpStatus WINGDIPAPI GdipIsOutlineVisiblePathPoint(GpPath* path, REAL x, REAL y, { GpStatus stat; GpPath *wide_path; + GpPointF pt = {x, y}; GpMatrix *transform = NULL; - TRACE("(%p,%0.2f,%0.2f,%p,%p,%p)\n", path, x, y, pen, graphics, result); + TRACE("(%p, %0.2f, %0.2f, %p, %p, %p)\n", path, x, y, pen, graphics, result); if(!path || !pen) return InvalidParameter; @@ -1747,22 +1748,15 @@ GpStatus WINGDIPAPI GdipIsOutlineVisiblePathPoint(GpPath* path, REAL x, REAL y, if (stat == Ok) stat = get_graphics_transform(graphics, CoordinateSpaceDevice, CoordinateSpaceWorld, transform); + if (stat == Ok) + GdipTransformMatrixPoints(transform, &pt, 1); } if (stat == Ok) - stat = GdipWidenPath(wide_path, pen, transform, 1.0); - - if (pen->unit == UnitPixel && graphics != NULL) - { - if (stat == Ok) - stat = GdipInvertMatrix(transform); - - if (stat == Ok) - stat = GdipTransformPath(wide_path, transform); - } + stat = GdipWidenPath(wide_path, pen, transform, 0.25f); if (stat == Ok) - stat = GdipIsVisiblePathPoint(wide_path, x, y, graphics, result); + stat = GdipIsVisiblePathPoint(wide_path, pt.X, pt.Y, graphics, result); GdipDeleteMatrix(transform); diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index 074311c71a0..03122cdda5d 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -1964,7 +1964,7 @@ static void test_is_outline_visible_path_point(void) result = TRUE; status = GdipIsOutlineVisiblePathPoint(path, 1.0, 1.0, pen, graphics, &result); expect(Ok, status); - todo_wine expect(FALSE, result); + expect(FALSE, result); result = FALSE; status = GdipIsOutlineVisiblePathPoint(path, 2.0, 1.0, pen, graphics, &result); expect(Ok, status); @@ -1976,7 +1976,7 @@ static void test_is_outline_visible_path_point(void) result = TRUE; status = GdipIsOutlineVisiblePathPoint(path, 14.0, 1.0, pen, graphics, &result); expect(Ok, status); - todo_wine expect(FALSE, result); + expect(FALSE, result); result = FALSE; status = GdipIsOutlineVisiblePathPoint(path, 15.0, 1.0, pen, graphics, &result); expect(Ok, status); @@ -2001,7 +2001,7 @@ static void test_is_outline_visible_path_point(void) result = FALSE; status = GdipIsOutlineVisiblePathPoint(path, 2.0, 1.0, pen, graphics, &result); expect(Ok, status); - todo_wine expect(TRUE, result); + expect(TRUE, result); result = TRUE; status = GdipIsOutlineVisiblePathPoint(path, 3.0, 1.0, pen, graphics, &result); expect(Ok, status); @@ -2013,7 +2013,7 @@ static void test_is_outline_visible_path_point(void) result = FALSE; status = GdipIsOutlineVisiblePathPoint(path, 15.0, 1.0, pen, graphics, &result); expect(Ok, status); - todo_wine expect(TRUE, result); + expect(TRUE, result); result = TRUE; status = GdipIsOutlineVisiblePathPoint(path, 16.0, 1.0, pen, graphics, &result); expect(Ok, status); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4165
On Wed Oct 25 19:33:13 2023 +0000, Jeffrey Smith wrote:
It appears that the commits got squashed back into one. Thanks, fixed
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49803
On Wed Oct 25 20:06:35 2023 +0000, Bartosz Kosiorek wrote:
Thanks, fixed @gang65 You can see what I've been investigation with `GdipIsVisiblePathPoint` here: https://gitlab.winehq.org/wine/wine/-/merge_requests/4183
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49826
On Thu Oct 26 05:42:36 2023 +0000, Jeffrey Smith wrote:
@gang65 You can see what I've been investigation with `GdipIsVisiblePathPoint` here: https://gitlab.winehq.org/wine/wine/-/merge_requests/4183 Does it make sense to merge this now, or should we wait while that work is in progress? The MR looks good to me and I would approve it as-is.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49868
On Thu Oct 26 17:58:38 2023 +0000, Esme Povirk wrote:
Does it make sense to merge this now, or should we wait while that work is in progress? The MR looks good to me and I would approve it as-is. I would merge it as it is. I think it is next step forward till perfect implementation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49885
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165
Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/graphicspath.c:
+ if (stat == Ok) + GdipTransformMatrixPoints(transform, &pt, 1); }
if (stat == Ok) - stat = GdipWidenPath(wide_path, pen, transform, 1.0); - - if (pen->unit == UnitPixel && graphics != NULL) - { - if (stat == Ok) - stat = GdipInvertMatrix(transform); - - if (stat == Ok) - stat = GdipTransformPath(wide_path, transform); - } + stat = GdipWidenPath(wide_path, pen, transform, 0.25f); It's not obvious to me why this is 0.25f. Could you explain?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49887
Jeffrey Smith (@whydoubt) commented about dlls/gdiplus/graphicspath.c:
if (stat == Ok) - stat = GdipWidenPath(wide_path, pen, transform, 1.0); - - if (pen->unit == UnitPixel && graphics != NULL) - { - if (stat == Ok) - stat = GdipInvertMatrix(transform); - - if (stat == Ok) - stat = GdipTransformPath(wide_path, transform); - } + stat = GdipWidenPath(wide_path, pen, transform, 0.25f);
if (stat == Ok) - stat = GdipIsVisiblePathPoint(wide_path, x, y, graphics, result);
It seem that with this change `GdipIsOutlineVisiblePathPoint` performs World -> Device space conversion on both the path and point. `GdipIsVisiblePathPoint` (once fixed) is _also_ going to perform a World -> Device space conversion. So I think either world-space path & point should be passed (as they are now), or NULL should be passed for graphics here to keep if from applying a space conversion. In the latter case, whatever fix is found to deal with the quirks identified there may need to be duplicated here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49900
On Thu Oct 26 20:38:16 2023 +0000, Jeffrey Smith wrote:
It's not obvious to me why this is 0.25f. Could you explain? Based on my obervations, it gives better accuracy closer to native. Now I am thinking, I should replace this magic number with FlatnessDefault variable instead. https://learn.microsoft.com/en-us/windows/win32/api/gdipluspath/nf-gdipluspa...
I will do that in separate MR as default flatness needs to be changed also in other places. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49932
On Thu Oct 26 21:53:04 2023 +0000, Jeffrey Smith wrote:
It seem that with this change `GdipIsOutlineVisiblePathPoint` performs World -> Device space conversion on both the path and point. `GdipIsVisiblePathPoint` (once fixed) is _also_ going to perform a World -> Device space conversion. So I think either world-space path & point should be passed (as they have been prior to your changes), or NULL should be passed for graphics here to keep if from applying a space conversion. In the latter case, whatever fix is found to deal with the quirks identified there may need to be duplicated here. I think we should completely remove World -> Device space conversion from GdipIsOutlineVisiblePathPoint, e.g. by passing transformation=NULL to `GdipWidenPath`: `GdipWidenPath(wide_path, pen, NULL, 1.0);`.
As a result all conversion will be done in `GdipIsVisiblePathPoint`. It will simplify the code, and boost performance (no need to have redundant calculation in `GdipIsOutlineVisiblePathPoint`).`` I could help with that after merging this PR (which make sure there will be no regression in `GdipIsOutlineVisiblePathPoint`). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49954
On Fri Oct 27 07:26:25 2023 +0000, Bartosz Kosiorek wrote:
I think we should completely remove World -> Device space conversion from GdipIsOutlineVisiblePathPoint, e.g. by passing transformation=NULL to `GdipWidenPath`: `GdipWidenPath(wide_path, pen, NULL, 1.0);`. As a result all conversion will be done in `GdipIsVisiblePathPoint`. It will simplify the code, and boost performance (no need to have redundant calculation in `GdipIsOutlineVisiblePathPoint`).`` I could help with that after merging this PR (which make sure there will be no regression in `GdipIsOutlineVisiblePathPoint`). I agree, keeping the path widening in world space sounds like a good solution.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49964
On Fri Oct 27 12:14:51 2023 +0000, Jeffrey Smith wrote:
I agree, keeping the path widening in world space sounds like a good solution. I don't follow. We have to widen in device space to handle pixel units correctly, don't we?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49976
On Fri Oct 27 15:49:12 2023 +0000, Esme Povirk wrote:
I don't follow. We have to widen in device space to handle pixel units correctly, don't we? The idea is first widen (without transformation) and then convert path to device space in `GdipIsVisiblePathPoint`,
as Jeffrey would like to reintroduce World -> Device tranformation with: https://gitlab.winehq.org/wine/wine/-/merge_requests/4183/diffs#9d342cb9ef84... This is only the idea, which needs to be verified and tested. I am sorry for confusion. I would like to merge this MR, to have small steps, and to preserve `GdipIsOutlineVisiblePathPoint` tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165#note_49984
This merge request was approved by Esme Povirk. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4165
participants (4)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Esme Povirk (＠madewokherd)
-
Jeffrey Smith (＠whydoubt)