[PATCH 0/2] MR4183: Draft: GdipIsVisiblePathPoint improvements and tests
This includes a partial fix for an issue with GdipIsVisiblePathPoint uncovered by https://bugs.winehq.org/show_bug.cgi?id=55717 There are also some hacky tests to help demonstrate some quirks that are not covered by the fix. Perhaps they will can lead to a better understanding of why the function behaves as it does. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/tests/graphicspath.c | 103 ++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index 908ca8bcaf0..2d8209005b7 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -1883,6 +1883,108 @@ static void test_isvisible(void) ReleaseDC(0, hdc); } +static REAL bisect(REAL minX, REAL maxX, REAL Y, GpGraphics *graphics, GpPath *path) +{ + BOOL resultMin = -1, resultMax = -1; + + GdipIsVisiblePathPoint(path, minX, Y, graphics, &resultMin); + GdipIsVisiblePathPoint(path, maxX, Y, graphics, &resultMax); + + if (resultMin == resultMax) + { + /* trace("initial results for min/max matched (%s)\n", resultMin?"T":"F"); */ + return 0.0; + } + + do { + REAL newX = (minX + maxX) * 0.5; + BOOL resultNew = -1; + + if (newX == minX || newX == maxX) { + /* trace("%10.7f:%s %10.7f:%s\n", minX, resultMin?"T":"F", maxX, resultMax?"T":"F"); */ + return maxX; + } + GdipIsVisiblePathPoint(path, newX, Y, graphics, &resultNew); + if (resultNew == resultMin) + minX = newX; + else + maxX = newX; + } while (1); +} + +static void test_isvisible_line(void) +{ + GpPath *path; + GpGraphics *graphics = NULL; + HDC hdc = GetDC(0); + GpStatus status; + GpMatrix *matrix; + static const REAL scales[] = {0.25, 0.5, 1.0, 2.0, 4.0}; + static const REAL expected[7][5] = { + {0}, /* UnitWorld */ + {9.5, 9.5, 9.5, 9.5, 9.5}, /* UnitDisplay */ + {10.0, 9.0, 9.5, 9.75, 9.875}, /* UnitPixel */ + {10.5, 9.75, 10.125, 9.9375, 10.03125}, /* UnitPoint */ + {9.979167, 9.989584, 9.994792, 9.997396, 9.998698}, /* UnitInch */ + {6.25, 9.375, 10.9375, 10.15625, 9.765626}, /* UnitDocument */ + {10.054167, 9.789584, 9.921876, 9.988022, 10.021094}, /* UnitMillimeter */ + }; + REAL result; + UINT i, j; + + GdipCreateFromHDC(hdc, &graphics); + GdipCreatePath(FillModeAlternate, &path); + + GdipResetWorldTransform(graphics); + + GdipAddPathRectangle(path, 10.0, 0.0, 10.0, 10.0); + + /* test with NULL graphics context */ + result = bisect(6.0, 14.0, 5.0, NULL, path); + ok(fabs(9.5 - result) < 0.000002, "Expected %.8e, got %.8e\n", 9.5, result); + + for (i = UnitWorld; i <= UnitMillimeter + 1; i++) + { + winetest_push_context("%u", i); + status = GdipSetPageUnit(graphics, i); + todo_wine_if(i > UnitMillimeter) + expect((i != UnitWorld && i <= UnitMillimeter) ? Ok : InvalidParameter, status); + if (status != Ok || i > UnitMillimeter) + { + winetest_pop_context(); + continue; + } + + for (j = 0; j < 5; j++) + { + winetest_push_context("%f", scales[j]); + GdipSetPageScale(graphics, scales[j]); + result = bisect(6.0, 14.0, 5.0, graphics, path); + todo_wine_if(expected[i][j] != 9.5) + ok(fabs(expected[i][j] - result) < 0.000002, "Expected %.8e, got %.8e\n", expected[i][j], result); + winetest_pop_context(); + } + + winetest_pop_context(); + } + + /* Mimic the IsOutlineVisible_Line_WithGraphics test from Mono */ + GdipResetPath(path); + GdipAddPathRectangle(path, 10.0, 0.5, 4.0, 1.0); + GdipCreateMatrix2(2.0, 0.0, 0.0, 2.0, 50.0, -50.0, &matrix); + GdipSetWorldTransform(graphics, matrix); + GdipDeleteMatrix(matrix); + GdipSetPageUnit(graphics, UnitMillimeter); + GdipSetPageScale(graphics, 2.0); + result = bisect(9.0, 11.0, 1.0, graphics, path); + todo_wine + ok(fabs(10.024221 - result) < 0.000002, "Expected %.8e, got %.8e\n", 10.024221, result); + + GdipDeletePath(path); + GdipDeleteGraphics(graphics); + ReleaseDC(0, hdc); +} + static void test_empty_rect(void) { GpPath *path; @@ -1976,6 +2078,7 @@ START_TEST(graphicspath) test_widen(); test_widen_cap(); test_isvisible(); + test_isvisible_line(); test_empty_rect(); GdiplusShutdown(gdiplusToken); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4183
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/gdiplus/graphicspath.c | 8 ++++++-- dlls/gdiplus/tests/graphicspath.c | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c index 8e436f96f4e..62c3814f4c1 100644 --- a/dlls/gdiplus/graphicspath.c +++ b/dlls/gdiplus/graphicspath.c @@ -1786,6 +1786,7 @@ GpStatus WINGDIPAPI GdipIsVisiblePathPoint(GpPath* path, REAL x, REAL y, GpGraph GpRegion *region; HRGN hrgn; GpStatus status; + GpPointF pt = {x, y}; if(!path || !result) return InvalidParameter; @@ -1793,13 +1794,16 @@ GpStatus WINGDIPAPI GdipIsVisiblePathPoint(GpPath* path, REAL x, REAL y, GpGraph if(status != Ok) return status; - status = GdipGetRegionHRgn(region, NULL, &hrgn); + status = GdipGetRegionHRgn(region, graphics, &hrgn); if(status != Ok){ GdipDeleteRegion(region); return status; } - *result = PtInRegion(hrgn, gdip_round(x), gdip_round(y)); + if (graphics) + gdip_transform_points(graphics, CoordinateSpaceDevice, CoordinateSpaceWorld, &pt, 1); + + *result = PtInRegion(hrgn, gdip_round(pt.X), gdip_round(pt.Y)); DeleteObject(hrgn); GdipDeleteRegion(region); diff --git a/dlls/gdiplus/tests/graphicspath.c b/dlls/gdiplus/tests/graphicspath.c index 2d8209005b7..221cac9019e 100644 --- a/dlls/gdiplus/tests/graphicspath.c +++ b/dlls/gdiplus/tests/graphicspath.c @@ -1960,7 +1960,7 @@ static void test_isvisible_line(void) winetest_push_context("%f", scales[j]); GdipSetPageScale(graphics, scales[j]); result = bisect(6.0, 14.0, 5.0, graphics, path); - todo_wine_if(expected[i][j] != 9.5) + todo_wine_if(expected[i][j] > 10.0) ok(fabs(expected[i][j] - result) < 0.000002, "Expected %.8e, got %.8e\n", expected[i][j], result); winetest_pop_context(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4183
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=139112 Your paranoid android. === w7u_2qxl (32 bit report) === gdiplus: graphicspath: Timeout === w7u_adm (32 bit report) === gdiplus: graphicspath: Timeout === w7u_el (32 bit report) === gdiplus: graphicspath: Timeout === w8 (32 bit report) === gdiplus: graphicspath: Timeout === w8adm (32 bit report) === gdiplus: graphicspath: Timeout === w864 (32 bit report) === gdiplus: graphicspath: Timeout === w1064v1507 (32 bit report) === gdiplus: graphicspath: Timeout === w1064v1809 (32 bit report) === gdiplus: graphicspath: Timeout === w1064_tsign (32 bit report) === gdiplus: graphicspath: Timeout === w10pro64 (32 bit report) === gdiplus: graphicspath: Timeout === w10pro64_en_AE_u8 (32 bit report) === gdiplus: graphicspath: Timeout === w11pro64 (32 bit report) === gdiplus: graphicspath: Timeout === debian11 (32 bit report) === gdiplus: graphicspath: Timeout === debian11 (32 bit ar:MA report) === gdiplus: graphicspath: Timeout === debian11 (32 bit de report) === gdiplus: graphicspath: Timeout === debian11 (32 bit fr report) === gdiplus: graphicspath: Timeout === debian11 (32 bit he:IL report) === gdiplus: graphicspath: Timeout === debian11 (32 bit hi:IN report) === gdiplus: graphicspath: Timeout === debian11 (32 bit ja:JP report) === gdiplus: graphicspath: Timeout === debian11 (32 bit zh:CN report) === gdiplus: graphicspath: Timeout === debian11b (32 bit WoW report) === gdiplus: graphicspath: Timeout
Bartosz Kosiorek (@gang65) commented about dlls/gdiplus/tests/graphicspath.c:
+ for (j = 0; j < 5; j++) + { + winetest_push_context("%f", scales[j]); + GdipSetPageScale(graphics, scales[j]); + result = bisect(6.0, 14.0, 5.0, graphics, path); + todo_wine_if(expected[i][j] != 9.5) + ok(fabs(expected[i][j] - result) < 0.000002, "Expected %.8e, got %.8e\n", expected[i][j], result); + winetest_pop_context(); + } + + winetest_pop_context(); + } + + /* Mimic the IsOutlineVisible_Line_WithGraphics test from Mono */ + GdipResetPath(path); + GdipAddPathRectangle(path, 10.0, 0.5, 4.0, 1.0); Great work! Thanks.
This is not 100% accurate with test from: https://github.com/madewokherd/mono/blob/develop/mcs/class/System.Drawing/Te... as in Mono test it is drawing single line (not rectangle). I suspect that it could be connected with Cap Endings. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183#note_49838
On Thu Oct 26 09:25:30 2023 +0000, Bartosz Kosiorek wrote:
Great work! Thanks. This is not 100% accurate with test from: https://github.com/madewokherd/mono/blob/develop/mcs/class/System.Drawing/Te... as in Mono test it is drawing single line (not rectangle). I suspect that it could be connected with Cap Endings. The rectangle will use Joints to draw rectangle (without Caps Endings). More information: https://learn.microsoft.com/en-us/windows/win32/gdiplus/-gdiplus-joining-lin... and https://learn.microsoft.com/en-us/windows/win32/gdiplus/-gdiplus-drawing-a-l... @gang65 When `IsOutlineVisible` in mono calls `GdipIsOutlineVisiblePathPoint`, `GdipIsOutlineVisiblePathPoint` widens the line into this shape (as of the current code) which it passes to `GdipIsVisiblePathPoint`. I have tested that this matches the shape passed in from wine's `GdipIsOutlineVisiblePathPoint`, and it is _logically_ the correct shape to result from widening the line in the mono test with the pen in the mono test. Do you have evidence to the contrary with current wine code? I believe it is sufficiently accurate in the context, and it sufficiently demonstrates a bug which, if fixed, will result in fixing the mono test.
From looking at the mono test, the path has no joins (as it's a single line), and the pen has no caps, so I'm not sure what you're getting at there.
While you may be able to fix `GdipIsOutlineVisiblePathPoint` without fixing `GdipIsVisiblePathPoint`, as things stand now I believe that fixing `GdipIsVisiblePathPoint` will fix _both_ (and the mono test). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183#note_49849
Esme Povirk (@madewokherd) commented about dlls/gdiplus/tests/graphicspath.c:
+static void test_isvisible_line(void) +{ + GpPath *path; + GpGraphics *graphics = NULL; + HDC hdc = GetDC(0); + GpStatus status; + GpMatrix *matrix; + static const REAL scales[] = {0.25, 0.5, 1.0, 2.0, 4.0}; + static const REAL expected[7][5] = { + {0}, /* UnitWorld */ + {9.5, 9.5, 9.5, 9.5, 9.5}, /* UnitDisplay */ + {10.0, 9.0, 9.5, 9.75, 9.875}, /* UnitPixel */ + {10.5, 9.75, 10.125, 9.9375, 10.03125}, /* UnitPoint */ + {9.979167, 9.989584, 9.994792, 9.997396, 9.998698}, /* UnitInch */ + {6.25, 9.375, 10.9375, 10.15625, 9.765626}, /* UnitDocument */ + {10.054167, 9.789584, 9.921876, 9.988022, 10.021094}, /* UnitMillimeter */ Hang on, this implies that that IsVisiblePathPoint doesn't rasterize the path? So we can stop doing that?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183#note_50039
On Sat Oct 28 15:23:16 2023 +0000, Esme Povirk wrote:
Hang on, this implies that IsVisiblePathPoint doesn't rasterize the path? So we can stop doing that? This makes me wonder if region functions don't either.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183#note_50040
On Sat Oct 28 15:42:25 2023 +0000, Esme Povirk wrote:
This makes me wonder if region functions don't either. Oh never mind, I guess they're rasterized in device space and the points are rounded. I continue to hate how we have to do this.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4183#note_50041
participants (5)
-
Bartosz Kosiorek (＠gang65) -
Esme Povirk (＠madewokherd)
-
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Marvin