[PATCH 0/13] MR3420: ddraw: Implement Pick() and GetPickRecords().
- The initial development was done by Matthew Wong in 2019. I split his v2 patch into testing and implementation commits, making changes needed to get it to compile. - Some of the feedback given on the v2 patch was addressed by Myah Caron's updated patch versions in 2020, which I split out into a few separate commits. - I also addressed some of the feedback, expanded the tests, and fixed some results based on those tests. _(Following is the commit message from Matthew Wong's v2 patch)_ Implement functions used by some games (notably LEGO Island) for determining which 3D object in a scene was clicked by the mouse cursor. Fighting Steel also uses this function for mouse over. Previous stubs would cause LEGO Island to crash upon any click and Fighting Steel to crash on game start. A patch posted years ago on the bug thread provided the minimum functionality to prevent crashes, but still rendered large portions of the game inaccessible without them implemented correctly. Picking has been implemented by adding a "pick mode" in d3d_execute_buffer_execute() which skips any drawing functions leaving just the vertex processing. Adds click tests for each triangle when in pick mode for creating an array of D3DPICKRECORDs. Add a D3DPICKRECORD array and DWORD counter to d3d_device. These are initiated in d3d_device_init(), allocated/written in d3d_execute_buffer_execute(), and accessed/read in d3d_device1_GetPickRecords(). The counter is used to determine the array size (0 meaning array is not allocated). The array is free'd whenever the data is no longer necessary by d3d_execute_buffer_execute(), d3d_device1_GetPickRecords(), and d3d_device_inner_Release(). Add a compliance test to ddraw1 to test whether certain screen points result in successful picks or not, as well as whether the data returned from GetPickRecords() is valid and correct. Stress testing reveals this patch's Pick() implementation may have slight inaccuracies to the original function; occasionally pixels right on triangle edges result in successful picks when they don't with the original function (and vice versa). It may be some sort of floating point rounding error or other algorithm difference that would be difficult to determine without seeing the original code. In practice, I believe this inaccuracy is so negligible that it won't produce any undesirable results for the user. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=10729 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Matthew Wong <itsmattkc(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 125 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 125 insertions(+) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index cde80c58e99..4cbab6eaee5 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15432,6 +15432,130 @@ static void test_enum_devices(void) ok(!refcount, "Device has %lu references left.\n", refcount); } +static void test_pick(void) +{ + static D3DTLVERTEX tquad[] = + { + {{320.0f}, {480.0f}, { 1.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{ 0.0f}, {480.0f}, {-0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{320.0f}, { 0.0f}, { 1.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{ 0.0f}, { 0.0f}, {-0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + }; + IDirect3DExecuteBuffer *execute_buffer; + D3DEXECUTEBUFFERDESC exec_desc; + IDirect3DViewport *viewport; + IDirect3DDevice *device; + IDirectDraw *ddraw; + UINT inst_length; + HWND window; + HRESULT hr; + void *ptr; + DWORD rec_count; + D3DRECT pick_rect; + UINT screen_width = 640; + UINT screen_height = 480; + UINT hits = 0; + UINT nohits = 0; + int i, j; + + window = create_window(); + ddraw = create_ddraw(); + ok(!!ddraw, "Failed to create a ddraw object.\n"); + if (!(device = create_device(ddraw, window, DDSCL_NORMAL))) + { + skip("Failed to create a 3D device, skipping test.\n"); + IDirectDraw_Release(ddraw); + DestroyWindow(window); + return; + } + + viewport = create_viewport(device, 0, 0, screen_width, screen_height); + + memset(&exec_desc, 0, sizeof(exec_desc)); + exec_desc.dwSize = sizeof(exec_desc); + exec_desc.dwFlags = D3DDEB_BUFSIZE | D3DDEB_CAPS; + exec_desc.dwBufferSize = 1024; + exec_desc.dwCaps = D3DDEBCAPS_SYSTEMMEMORY; + + hr = IDirect3DDevice_CreateExecuteBuffer(device, &exec_desc, &execute_buffer, NULL); + ok(SUCCEEDED(hr), "Failed to create execute buffer, hr %#lx.\n", hr); + hr = IDirect3DExecuteBuffer_Lock(execute_buffer, &exec_desc); + ok(SUCCEEDED(hr), "Failed to lock execute buffer, hr %#lx.\n", hr); + memcpy(exec_desc.lpData, tquad, sizeof(tquad)); + ptr = ((BYTE *)exec_desc.lpData) + sizeof(tquad); + emit_process_vertices(&ptr, D3DPROCESSVERTICES_COPY, 0, 4); + emit_tquad(&ptr, 0); + emit_end(&ptr); + inst_length = (BYTE *)ptr - (BYTE *)exec_desc.lpData; + inst_length -= sizeof(tquad); + hr = IDirect3DExecuteBuffer_Unlock(execute_buffer); + ok(SUCCEEDED(hr), "Failed to unlock execute buffer, hr %#lx.\n", hr); + + set_execute_data(execute_buffer, 4, sizeof(tquad), inst_length); + + /* Perform a number of picks, we should have a specific amount by the end */ + for (i = 0; i < screen_width; i += 80) + { + for (j = 0; j < screen_height; j += 60) + { + pick_rect.x1 = i; + pick_rect.y1 = j; + + hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); + ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + if (rec_count > 0) + hits++; + else + nohits++; + } + } + + /* + We should have gotten precisely equal numbers of hits and no hits since our quad + covers exactly half the screen + */ + todo_wine ok(hits == nohits, "Got a non-equal amount of pick successes/failures: %i vs %i.\n", hits, nohits); + + /* Try some specific pixel picks */ + pick_rect.x1 = 480; + pick_rect.y1 = 360; + hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); + ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); + + pick_rect.x1 = 240; + pick_rect.y1 = 120; + hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); + ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + todo_wine ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); + + if (rec_count == 1) + { + D3DPICKRECORD record; + + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + + /* Tests D3DPICKRECORD for correct information */ + ok(record.bOpcode == 3, "Got incorrect bOpcode: %i.\n", record.bOpcode); + ok(record.bPad == 0, "Got incorrect bPad: %i.\n", record.bPad); + ok(record.dwOffset == 24, "Got incorrect dwOffset: %lu.\n", record.dwOffset); + ok(record.dvZ > 0.99 && record.dvZ < 1.01, "Got incorrect dvZ: %f.\n", record.dvZ); + } + + destroy_viewport(device, viewport); + IDirect3DExecuteBuffer_Release(execute_buffer); + IDirect3DDevice_Release(device); + IDirectDraw_Release(ddraw); + DestroyWindow(window); +} + START_TEST(ddraw1) { DDDEVICEIDENTIFIER identifier; @@ -15551,4 +15675,5 @@ START_TEST(ddraw1) run_for_each_device_type(test_texture_wrong_caps); test_filling_convention(); test_enum_devices(); + test_pick(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index 4cbab6eaee5..e350e071b94 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15493,7 +15493,9 @@ static void test_pick(void) set_execute_data(execute_buffer, 4, sizeof(tquad), inst_length); - /* Perform a number of picks, we should have a specific amount by the end */ + /* Perform a number of picks, we should have a specific amount by the end. + * We should get precisely equal numbers of hits and no hits since our quad + * covers exactly half the screen. */ for (i = 0; i < screen_width; i += 80) { for (j = 0; j < screen_height; j += 60) @@ -15503,34 +15505,34 @@ static void test_pick(void) hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - if (rec_count > 0) - hits++; - else + if (rec_count == 0) nohits++; + else if (rec_count != ~0) + hits++; } } - /* - We should have gotten precisely equal numbers of hits and no hits since our quad - covers exactly half the screen - */ - todo_wine ok(hits == nohits, "Got a non-equal amount of pick successes/failures: %i vs %i.\n", hits, nohits); + todo_wine ok(hits + nohits > 0, "Did not get any pick hits or misses.\n"); + ok(hits == nohits, "Got a non-equal amount of pick hits/misses: %i vs %i.\n", hits, nohits); /* Try some specific pixel picks */ pick_rect.x1 = 480; pick_rect.y1 = 360; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); + todo_wine ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); pick_rect.x1 = 240; pick_rect.y1 = 120; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); todo_wine ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index e350e071b94..263bdb3562a 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15551,6 +15551,11 @@ static void test_pick(void) ok(record.dvZ > 0.99 && record.dvZ < 1.01, "Got incorrect dvZ: %f.\n", record.dvZ); } + if (0) /* This crashes on Windows. */ + { + IDirect3DDevice_GetPickRecords(device, NULL, NULL); + } + destroy_viewport(device, viewport); IDirect3DExecuteBuffer_Release(execute_buffer); IDirect3DDevice_Release(device); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index 263bdb3562a..7a71041a685 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15548,7 +15548,7 @@ static void test_pick(void) ok(record.bOpcode == 3, "Got incorrect bOpcode: %i.\n", record.bOpcode); ok(record.bPad == 0, "Got incorrect bPad: %i.\n", record.bPad); ok(record.dwOffset == 24, "Got incorrect dwOffset: %lu.\n", record.dwOffset); - ok(record.dvZ > 0.99 && record.dvZ < 1.01, "Got incorrect dvZ: %f.\n", record.dvZ); + ok(compare_float(record.dvZ, 1.0, 4096), "Got incorrect dvZ: %.8e.\n", record.dvZ); } if (0) /* This crashes on Windows. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 63 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index 7a71041a685..830ecd5d665 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15436,10 +15436,21 @@ static void test_pick(void) { static D3DTLVERTEX tquad[] = { + /* Left half of viewport */ {{320.0f}, {480.0f}, { 1.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, {{ 0.0f}, {480.0f}, {-0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, {{320.0f}, { 0.0f}, { 1.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, {{ 0.0f}, { 0.0f}, {-0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + /* Lower-left quarter of viewport */ + {{320.0f}, {480.0f}, {-0.3f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{ 0.0f}, {480.0f}, { 1.7f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{320.0f}, {240.0f}, {-0.3f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{ 0.0f}, {240.0f}, { 1.7f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + /* Right half of lower-left quarter of viewport */ + {{320.0f}, {480.0f}, { 0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{160.0f}, {480.0f}, { 0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{320.0f}, {240.0f}, { 0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, + {{160.0f}, {240.0f}, { 0.5f}, {1.0f}, {0xff00ff00}, {0x00000000}, {0.0f}, {0.0f}}, }; IDirect3DExecuteBuffer *execute_buffer; D3DEXECUTEBUFFERDESC exec_desc; @@ -15483,15 +15494,17 @@ static void test_pick(void) ok(SUCCEEDED(hr), "Failed to lock execute buffer, hr %#lx.\n", hr); memcpy(exec_desc.lpData, tquad, sizeof(tquad)); ptr = ((BYTE *)exec_desc.lpData) + sizeof(tquad); - emit_process_vertices(&ptr, D3DPROCESSVERTICES_COPY, 0, 4); + emit_process_vertices(&ptr, D3DPROCESSVERTICES_COPY, 0, 12); emit_tquad(&ptr, 0); + emit_tquad(&ptr, 4); + emit_tquad(&ptr, 8); emit_end(&ptr); inst_length = (BYTE *)ptr - (BYTE *)exec_desc.lpData; inst_length -= sizeof(tquad); hr = IDirect3DExecuteBuffer_Unlock(execute_buffer); ok(SUCCEEDED(hr), "Failed to unlock execute buffer, hr %#lx.\n", hr); - set_execute_data(execute_buffer, 4, sizeof(tquad), inst_length); + set_execute_data(execute_buffer, 12, sizeof(tquad), inst_length); /* Perform a number of picks, we should have a specific amount by the end. * We should get precisely equal numbers of hits and no hits since our quad @@ -15518,7 +15531,7 @@ static void test_pick(void) todo_wine ok(hits + nohits > 0, "Did not get any pick hits or misses.\n"); ok(hits == nohits, "Got a non-equal amount of pick hits/misses: %i vs %i.\n", hits, nohits); - /* Try some specific pixel picks */ + /* Pick a pixel where no quads are present */ pick_rect.x1 = 480; pick_rect.y1 = 360; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); @@ -15528,6 +15541,7 @@ static void test_pick(void) ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); todo_wine ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); + /* Pick a pixel where one quad is present */ pick_rect.x1 = 240; pick_rect.y1 = 120; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); @@ -15551,6 +15565,49 @@ static void test_pick(void) ok(compare_float(record.dvZ, 1.0, 4096), "Got incorrect dvZ: %.8e.\n", record.dvZ); } + /* Pick a pixel where three quads are present */ + pick_rect.x1 = 240; + pick_rect.y1 = 360; + hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); + ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + rec_count = ~0; + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + todo_wine ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); + + if (rec_count == 3) + { + D3DPICKRECORD record[3] = {0}; + const D3DPICKRECORD empty_record = {0}; + + /* If the count is wrong, do not populate any records */ + rec_count = 1; + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); + ok(memcmp(&record[0], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + ok(memcmp(&record[1], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + ok(memcmp(&record[2], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + + rec_count = 3; + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + + /* Documentation states this list is z-ordered, but it appears that it is not. */ + ok(record[0].bOpcode == 3, "Got incorrect bOpcode: %i.\n", record[0].bOpcode); + ok(record[0].bPad == 0, "Got incorrect bPad: %i.\n", record[0].bPad); + ok(record[0].dwOffset == 24, "Got incorrect dwOffset: %lu.\n", record[0].dwOffset); + ok(compare_float(record[0].dvZ, 1.0, 4096), "Got incorrect dvZ: %.8e.\n", record[0].dvZ); + ok(record[1].bOpcode == 3, "Got incorrect bOpcode: %i.\n", record[1].bOpcode); + ok(record[1].bPad == 0, "Got incorrect bPad: %i.\n", record[1].bPad); + ok(record[1].dwOffset == 44, "Got incorrect dwOffset: %lu.\n", record[1].dwOffset); + ok(compare_float(record[1].dvZ, 0.2, 4096), "Got incorrect dvZ: %.8e.\n", record[1].dvZ); + ok(record[2].bOpcode == 3, "Got incorrect bOpcode: %i.\n", record[2].bOpcode); + ok(record[2].bPad == 0, "Got incorrect bPad: %i.\n", record[2].bPad); + ok(record[2].dwOffset == 64, "Got incorrect dwOffset: %lu.\n", record[2].dwOffset); + ok(compare_float(record[2].dvZ, 0.5, 4096), "Got incorrect dvZ: %.8e.\n", record[2].dvZ); + } + if (0) /* This crashes on Windows. */ { IDirect3DDevice_GetPickRecords(device, NULL, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/tests/ddraw1.c | 41 +++++++++++++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index 830ecd5d665..b009cb755a3 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15513,8 +15513,8 @@ static void test_pick(void) { for (j = 0; j < screen_height; j += 60) { - pick_rect.x1 = i; - pick_rect.y1 = j; + pick_rect.x1 = pick_rect.x2 = i; + pick_rect.y1 = pick_rect.y2 = j; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); @@ -15532,8 +15532,8 @@ static void test_pick(void) ok(hits == nohits, "Got a non-equal amount of pick hits/misses: %i vs %i.\n", hits, nohits); /* Pick a pixel where no quads are present */ - pick_rect.x1 = 480; - pick_rect.y1 = 360; + pick_rect.x1 = pick_rect.x2 = 480; + pick_rect.y1 = pick_rect.y2 = 360; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); rec_count = ~0; @@ -15542,8 +15542,8 @@ static void test_pick(void) todo_wine ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); /* Pick a pixel where one quad is present */ - pick_rect.x1 = 240; - pick_rect.y1 = 120; + pick_rect.x1 = pick_rect.x2 = 240; + pick_rect.y1 = pick_rect.y2 = 120; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); rec_count = ~0; @@ -15566,8 +15566,8 @@ static void test_pick(void) } /* Pick a pixel where three quads are present */ - pick_rect.x1 = 240; - pick_rect.y1 = 360; + pick_rect.x1 = pick_rect.x2 = 240; + pick_rect.y1 = pick_rect.y2 = 360; hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); rec_count = ~0; @@ -15608,6 +15608,31 @@ static void test_pick(void) ok(compare_float(record[2].dvZ, 0.5, 4096), "Got incorrect dvZ: %.8e.\n", record[2].dvZ); } + /* Pick a rectange, though it appears that only the upper-left corner is checked. */ + pick_rect.x1 = 240; + pick_rect.y1 = 120; + pick_rect.x2 = screen_width - 1; + pick_rect.y2 = screen_height - 1; + hr = IDirect3DDevice_Pick(device, execute_buffer, viewport, 0, &pick_rect); + ok(SUCCEEDED(hr), "Failed to perform pick, hr %#lx.\n", hr); + rec_count = ~0; + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + todo_wine ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); + + if (rec_count == 1) + { + D3DPICKRECORD record; + + hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record); + ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); + + ok(record.bOpcode == 3, "Got incorrect bOpcode: %i.\n", record.bOpcode); + ok(record.bPad == 0, "Got incorrect bPad: %i.\n", record.bPad); + ok(record.dwOffset == 24, "Got incorrect dwOffset: %lu.\n", record.dwOffset); + ok(compare_float(record.dvZ, 1.0, 4096), "Got incorrect dvZ: %.8e.\n", record.dvZ); + } + if (0) /* This crashes on Windows. */ { IDirect3DDevice_GetPickRecords(device, NULL, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Matthew Wong <itsmattkc(a)gmail.com> Implement functions used by some games for determining which 3D object in a scene was either moused over or clicked on by the mouse. Picking has been implemented by adding a "pick mode" in d3d_execute_buffer_execute() which skips any drawing functions leaving just the vertex processing. Adds click tests for each triangle when in pick mode for creating an array of D3DPICKRECORDs. Pick() implementation may have slight inaccuracies to the original function; occasionally pixels right on triangle edges result in successful picks when they don't with the original function (and vice versa). In practice, I believe this inaccuracy is so negligible that it won't produce any undesirable results for the user. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=10729 Signed-off-by: Matthew Wong <itsmattkc(a)gmail.com> --- dlls/ddraw/ddraw_private.h | 6 +- dlls/ddraw/device.c | 64 ++++++++++-- dlls/ddraw/executebuffer.c | 207 +++++++++++++++++++++++++++++++++++-- dlls/ddraw/tests/ddraw1.c | 16 +-- 4 files changed, 268 insertions(+), 25 deletions(-) diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h index 01a9579651c..912813ccc48 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -336,6 +336,10 @@ struct d3d_device struct d3d_viewport *current_viewport; D3DVIEWPORT7 active_viewport; + /* Pick data */ + D3DPICKRECORD *pick_records; + DWORD pick_record_count; + /* Required to keep track which of two available texture blending modes in d3ddevice3 is used */ BOOL legacyTextureBlending; D3DTEXTUREBLEND texture_map_blend; @@ -569,7 +573,7 @@ struct d3d_execute_buffer *unsafe_impl_from_IDirect3DExecuteBuffer(IDirect3DExec /* The execute function */ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *execute_buffer, - struct d3d_device *device); + struct d3d_device *device, D3DRECT *pick_rect); /***************************************************************************** * IDirect3DVertexBuffer diff --git a/dlls/ddraw/device.c b/dlls/ddraw/device.c index 80556e96787..ac4388ab639 100644 --- a/dlls/ddraw/device.c +++ b/dlls/ddraw/device.c @@ -349,6 +349,10 @@ static ULONG WINAPI d3d_device_inner_Release(IUnknown *iface) IDirect3DDevice3_DeleteViewport(&This->IDirect3DDevice3_iface, &vp->IDirect3DViewport3_iface); } + /* Free pick records array if any are allocated */ + if (This->pick_record_count > 0) + free(This->pick_records); + TRACE("Releasing render target %p.\n", This->rt_iface); rt_iface = This->rt_iface; This->rt_iface = NULL; @@ -758,7 +762,7 @@ static HRESULT WINAPI d3d_device1_Execute(IDirect3DDevice *iface, /* Execute... */ wined3d_mutex_lock(); - hr = d3d_execute_buffer_execute(buffer, device); + hr = d3d_execute_buffer_execute(buffer, device, NULL); wined3d_mutex_unlock(); return hr; @@ -1025,16 +1029,37 @@ static HRESULT WINAPI d3d_device1_NextViewport(IDirect3DDevice *iface, * x2 and y2 are ignored. * * Returns: - * D3D_OK because it's a stub + * D3D_OK on success + * DDERR_INVALIDPARAMS if any of the parameters == NULL * *****************************************************************************/ static HRESULT WINAPI d3d_device1_Pick(IDirect3DDevice *iface, IDirect3DExecuteBuffer *buffer, IDirect3DViewport *viewport, DWORD flags, D3DRECT *rect) { - FIXME("iface %p, buffer %p, viewport %p, flags %#lx, rect %s stub!\n", + struct d3d_device *device = impl_from_IDirect3DDevice(iface); + struct d3d_execute_buffer *buffer_impl = unsafe_impl_from_IDirect3DExecuteBuffer(buffer); + struct d3d_viewport *viewport_impl = unsafe_impl_from_IDirect3DViewport(viewport); + HRESULT hr; + + TRACE("iface %p, buffer %p, viewport %p, flags %#lx, rect %s.\n", iface, buffer, viewport, flags, wine_dbgstr_rect((RECT *)rect)); - return D3D_OK; + /* Sanity checks */ + if (!iface || !buffer || !viewport) + { + return DDERR_INVALIDPARAMS; + } + + if (FAILED(hr = IDirect3DDevice3_SetCurrentViewport + (&device->IDirect3DDevice3_iface, &viewport_impl->IDirect3DViewport3_iface))) + return hr; + + /* Execute the pick */ + wined3d_mutex_lock(); + hr = d3d_execute_buffer_execute(buffer_impl, device, rect); + wined3d_mutex_unlock(); + + return hr; } /***************************************************************************** @@ -1050,13 +1075,37 @@ static HRESULT WINAPI d3d_device1_Pick(IDirect3DDevice *iface, IDirect3DExecuteB * D3DPickRec: Address to store the resulting D3DPICKRECORD array. * * Returns: - * D3D_OK, because it's a stub + * D3D_OK always * *****************************************************************************/ static HRESULT WINAPI d3d_device1_GetPickRecords(IDirect3DDevice *iface, DWORD *count, D3DPICKRECORD *records) { - FIXME("iface %p, count %p, records %p stub!\n", iface, count, records); + struct d3d_device *device; + + TRACE("iface %p, count %p, records %p.\n", iface, count, records); + + wined3d_mutex_lock(); + + device = impl_from_IDirect3DDevice(iface); + + /* Set count to the number of pick records we have */ + *count = device->pick_record_count; + + /* It is correct usage according to documentation to call this function with records == NULL + to retrieve _just_ the record count, which the caller can then use to allocate an + appropriately sized array, then call this function again to fill that array with data. */ + if (records && count) + { + /* If we have a destination array and records to copy, copy them now */ + memcpy(records, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); + + /* We're now done with the old pick records and can free them */ + free(device->pick_records); + device->pick_record_count = 0; + } + + wined3d_mutex_unlock(); return D3D_OK; } @@ -6893,6 +6942,9 @@ static HRESULT d3d_device_init(struct d3d_device *device, struct ddraw *ddraw, c ddraw->d3ddevice = device; + /* Initialize pick records array */ + device->pick_record_count = 0; + wined3d_stateblock_set_render_state(ddraw->state, WINED3D_RS_ZENABLE, d3d_device_update_depth_stencil(device)); if (version == 1) /* Color keying is initially enabled for version 1 devices. */ diff --git a/dlls/ddraw/executebuffer.c b/dlls/ddraw/executebuffer.c index 13e639eda3f..bc95b3e2a82 100644 --- a/dlls/ddraw/executebuffer.c +++ b/dlls/ddraw/executebuffer.c @@ -45,15 +45,111 @@ static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) { TRACE("lpData : %p\n", lpDesc->lpData); } -HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d_device *device) +#define TRIANGLE_SIZE 3 +/***************************************************************************** + * d3d_execute_buffer_pick_test + * + * Determines whether a "point" is inside a "triangle". Mainly used when + * executing a "pick" from an execute buffer to determine whether a pixel + * coordinate (often a mouse coordinate) is inside a triangle (and + * therefore clicking or hovering over a 3D object in the scene). This + * function uses triangle rasterization algorithms to determine if the + * pixel falls inside (using the top-left rule, in accordance with + * documentation). + * + * Params: + * x: The X coordinate of the point to verify. + * y: The Y coordinate of the point to verify. + * verts: An array of vertices describing the screen coordinates of the + * triangle. This function expects 3 elements in this array. + * + * Returns: + * TRUE if the pixel coordinate is inside this triangle + * FALSE if not + * + *****************************************************************************/ +static BOOL d3d_execute_buffer_pick_test(LONG x, LONG y, D3DTLVERTEX* verts) +{ + UINT i; + + for (i = 0; i < TRIANGLE_SIZE; i++) + { + D3DTLVERTEX* v1 = &verts[(i) % TRIANGLE_SIZE]; + D3DTLVERTEX* v2 = &verts[(i + 1) % TRIANGLE_SIZE]; + D3DVALUE bias = 0.0f; + + /* Edge function - determines whether pixel is inside triangle */ + D3DVALUE w = (v2->sx - v1->sx) * (y - v1->sy) - (v2->sy - v1->sy) * (x - v1->sx); + + /* Force top-left rule */ + if ((v1->sy == v2->sy && v1->sx > v2->sx) || (v1->sy < v2->sy)) + bias = 1.0f; + + if (w < bias) + return FALSE; + } + + return TRUE; +} + +/***************************************************************************** + * d3d_execute_buffer_z_value_at_coords + * + * Returns the Z point of a triangle given an X, Y coordinate somewhere inside + * the triangle. Used as the `dvZ` parameter of D3DPICKRECORD. + * + * Params: + * x: The X coordinate of the point to verify. + * y: The Y coordinate of the point to verify. + * verts: An array of vertices describing the screen coordinates of the + * triangle. This function expects 3 elements in this array. + * + * Returns: + * A floating-point Z value that can be used directly as the dvZ member of a + * D3DPICKRECORD. + * + *****************************************************************************/ +static D3DVALUE d3d_execute_buffer_z_value_at_coords(LONG x, LONG y, D3DTLVERTEX* verts) +{ + UINT i; + + D3DVALUE z1 = 0; + D3DVALUE z2 = 0; + + for (i = 0; i < TRIANGLE_SIZE; i++) + { + D3DTLVERTEX* v1 = &verts[i]; + D3DTLVERTEX* v2 = &verts[(i + 1) % TRIANGLE_SIZE]; + D3DTLVERTEX* v3 = &verts[(i + 2) % TRIANGLE_SIZE]; + + z1 += v3->sz * (x - v1->sx) * (y - v2->sy) - v2->sz * (x - v1->sx) * (y - v3->sy); + z2 += (x - v1->sx) * (y - v2->sy) - (x - v1->sx) * (y - v3->sy); + } + + return z1 / z2; +} + +HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d_device *device, + D3DRECT* pick_rect) { DWORD is = buffer->data.dwInstructionOffset; char *instr = (char *)buffer->desc.lpData + is; unsigned int i, primitive_size; - struct wined3d_map_desc map_desc; + struct wined3d_map_desc map_desc, vert_map_desc; struct wined3d_box box = {0}; HRESULT hr; + /* Variables used for picking */ + const unsigned int vertex_size = get_flexible_vertex_size(D3DFVF_TLVERTEX); + D3DTLVERTEX verts[TRIANGLE_SIZE]; + + /* Check for any un-freed pick records */ + if (device->pick_record_count > 0) + { + free(device->pick_records); + device->pick_record_count = 0; + } + TRACE("ExecuteData :\n"); if (TRACE_ON(ddraw)) _dump_executedata(&(buffer->data)); @@ -69,6 +165,26 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d instr += sizeof(*current); primitive_size = 0; + /* We can skip these opcodes when Picking */ + if (pick_rect) + { + switch (current->bOpcode) + { + /* None of these opcodes seem to be necessary for picking */ + case D3DOP_POINT: + case D3DOP_LINE: + case D3DOP_STATETRANSFORM: + case D3DOP_STATELIGHT: + case D3DOP_STATERENDER: + case D3DOP_TEXTURELOAD: + case D3DOP_SPAN: + instr += count * size; + continue; + default: + break; + } + } + switch (current->bOpcode) { case D3DOP_POINT: @@ -174,6 +290,72 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d { case 3: indices[(i * primitive_size) + 2] = ci->v3; + + if (pick_rect) + { + UINT j; + + /* Get D3DTLVERTEX objects for each triangle vertex */ + for (j = 0; j < TRIANGLE_SIZE; j++) + { + /* Get index of vertex from D3DTRIANGLE struct */ + switch (j) { + case 0: box.left = vertex_size * ci->v1; break; + case 1: box.left = vertex_size * ci->v2; break; + case 2: box.left = vertex_size * ci->v3; break; + } + + box.right = box.left + vertex_size; + if (FAILED(hr = wined3d_resource_map(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), + 0, &vert_map_desc, &box, WINED3D_MAP_WRITE))) + { + return hr; + } + else + { + /* Copy vert data into stack array */ + verts[j] = *((D3DTLVERTEX*)vert_map_desc.data); + + wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), 0); + } + } + + /* Use vertices acquired above to test for clicking */ + if (d3d_execute_buffer_pick_test(pick_rect->x1, pick_rect->y1, verts)) + { + D3DPICKRECORD* record; + + /* Create new array to fit one more record */ + DWORD new_record_count = device->pick_record_count + 1; + D3DPICKRECORD* new_record_array = malloc(sizeof(*device->pick_records) * new_record_count); + if (device->pick_record_count > 0) + { + memcpy(new_record_array, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); + free(device->pick_records); + } + + /* Fill record parameters */ + record = &new_record_array[device->pick_record_count]; + + record->bOpcode = current->bOpcode; + record->bPad = 0; + + /* Write current instruction offset into file */ + record->dwOffset = instr - (char *)buffer->desc.lpData - is; + + /* Formula for returning the Z value at this X/Y */ + record->dvZ = d3d_execute_buffer_z_value_at_coords(pick_rect->x1, pick_rect->y1, verts); + + /* Point device info to new record information */ + device->pick_records = new_record_array; + device->pick_record_count = new_record_count; + + /* We have a successful pick so we can skip the rest of the triangles */ + instr += size * (count - i - 1); + count = i; + } + } + /* Drop through. */ case 2: indices[(i * primitive_size) + 1] = ci->v2; @@ -184,14 +366,18 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->index_buffer), 0); - wined3d_stateblock_set_stream_source(device->state, 0, - buffer->dst_vertex_buffer, 0, sizeof(D3DTLVERTEX)); - wined3d_stateblock_set_vertex_declaration(device->state, - ddraw_find_decl(device->ddraw, D3DFVF_TLVERTEX)); - wined3d_stateblock_set_index_buffer(device->state, buffer->index_buffer, WINED3DFMT_R16_UINT); - wined3d_device_apply_stateblock(device->wined3d_device, device->state); - d3d_device_sync_surfaces(device); - wined3d_device_context_draw_indexed(device->immediate_context, 0, index_pos, index_count, 0, 0); + /* Skip drawing if we're picking */ + if (!pick_rect) + { + wined3d_stateblock_set_stream_source(device->state, 0, + buffer->dst_vertex_buffer, 0, sizeof(D3DTLVERTEX)); + wined3d_stateblock_set_vertex_declaration(device->state, + ddraw_find_decl(device->ddraw, D3DFVF_TLVERTEX)); + wined3d_stateblock_set_index_buffer(device->state, buffer->index_buffer, WINED3DFMT_R16_UINT); + wined3d_device_apply_stateblock(device->wined3d_device, device->state); + d3d_device_sync_surfaces(device); + wined3d_device_context_draw_indexed(device->immediate_context, 0, index_pos, index_count, 0, 0); + } buffer->index_pos = index_pos + index_count; break; @@ -426,6 +612,7 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d end_of_buffer: return D3D_OK; } +#undef TRIANGLE_SIZE static inline struct d3d_execute_buffer *impl_from_IDirect3DExecuteBuffer(IDirect3DExecuteBuffer *iface) { diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index b009cb755a3..1ce8c6bf71b 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15528,7 +15528,7 @@ static void test_pick(void) } } - todo_wine ok(hits + nohits > 0, "Did not get any pick hits or misses.\n"); + ok(hits + nohits > 0, "Did not get any pick hits or misses.\n"); ok(hits == nohits, "Got a non-equal amount of pick hits/misses: %i vs %i.\n", hits, nohits); /* Pick a pixel where no quads are present */ @@ -15539,7 +15539,7 @@ static void test_pick(void) rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - todo_wine ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); + ok(rec_count == 0, "Got incorrect number of pick records (expected 0): %lu.\n", rec_count); /* Pick a pixel where one quad is present */ pick_rect.x1 = pick_rect.x2 = 240; @@ -15549,7 +15549,7 @@ static void test_pick(void) rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - todo_wine ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); + ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); if (rec_count == 1) { @@ -15573,7 +15573,7 @@ static void test_pick(void) rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - todo_wine ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); + ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); if (rec_count == 3) { @@ -15585,9 +15585,9 @@ static void test_pick(void) hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); - ok(memcmp(&record[0], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); - ok(memcmp(&record[1], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); - ok(memcmp(&record[2], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + todo_wine ok(memcmp(&record[0], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + todo_wine ok(memcmp(&record[1], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + todo_wine ok(memcmp(&record[2], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); rec_count = 3; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); @@ -15618,7 +15618,7 @@ static void test_pick(void) rec_count = ~0; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, NULL); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); - todo_wine ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); + ok(rec_count == 1, "Got incorrect number of pick records (expected 1): %lu.\n", rec_count); if (rec_count == 1) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Myah Caron <qsniyg(a)protonmail.com> --- dlls/ddraw/ddraw_private.h | 1 + dlls/ddraw/device.c | 10 +--------- dlls/ddraw/executebuffer.c | 29 ++++++++++++----------------- 3 files changed, 14 insertions(+), 26 deletions(-) diff --git a/dlls/ddraw/ddraw_private.h b/dlls/ddraw/ddraw_private.h index 912813ccc48..889a64219e5 100644 --- a/dlls/ddraw/ddraw_private.h +++ b/dlls/ddraw/ddraw_private.h @@ -339,6 +339,7 @@ struct d3d_device /* Pick data */ D3DPICKRECORD *pick_records; DWORD pick_record_count; + DWORD pick_record_size; /* Required to keep track which of two available texture blending modes in d3ddevice3 is used */ BOOL legacyTextureBlending; diff --git a/dlls/ddraw/device.c b/dlls/ddraw/device.c index ac4388ab639..5d850c53caf 100644 --- a/dlls/ddraw/device.c +++ b/dlls/ddraw/device.c @@ -349,8 +349,7 @@ static ULONG WINAPI d3d_device_inner_Release(IUnknown *iface) IDirect3DDevice3_DeleteViewport(&This->IDirect3DDevice3_iface, &vp->IDirect3DViewport3_iface); } - /* Free pick records array if any are allocated */ - if (This->pick_record_count > 0) + if (This->pick_record_size > 0) free(This->pick_records); TRACE("Releasing render target %p.\n", This->rt_iface); @@ -1099,10 +1098,6 @@ static HRESULT WINAPI d3d_device1_GetPickRecords(IDirect3DDevice *iface, { /* If we have a destination array and records to copy, copy them now */ memcpy(records, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); - - /* We're now done with the old pick records and can free them */ - free(device->pick_records); - device->pick_record_count = 0; } wined3d_mutex_unlock(); @@ -6942,9 +6937,6 @@ static HRESULT d3d_device_init(struct d3d_device *device, struct ddraw *ddraw, c ddraw->d3ddevice = device; - /* Initialize pick records array */ - device->pick_record_count = 0; - wined3d_stateblock_set_render_state(ddraw->state, WINED3D_RS_ZENABLE, d3d_device_update_depth_stencil(device)); if (version == 1) /* Color keying is initially enabled for version 1 devices. */ diff --git a/dlls/ddraw/executebuffer.c b/dlls/ddraw/executebuffer.c index bc95b3e2a82..99cd5710d7d 100644 --- a/dlls/ddraw/executebuffer.c +++ b/dlls/ddraw/executebuffer.c @@ -143,12 +143,7 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d const unsigned int vertex_size = get_flexible_vertex_size(D3DFVF_TLVERTEX); D3DTLVERTEX verts[TRIANGLE_SIZE]; - /* Check for any un-freed pick records */ - if (device->pick_record_count > 0) - { - free(device->pick_records); - device->pick_record_count = 0; - } + device->pick_record_count = 0; TRACE("ExecuteData :\n"); if (TRACE_ON(ddraw)) @@ -325,17 +320,21 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d { D3DPICKRECORD* record; - /* Create new array to fit one more record */ - DWORD new_record_count = device->pick_record_count + 1; - D3DPICKRECORD* new_record_array = malloc(sizeof(*device->pick_records) * new_record_count); - if (device->pick_record_count > 0) + device->pick_record_count++; + + /* Grow the array if necessary */ + if (device->pick_record_count > device->pick_record_size) { - memcpy(new_record_array, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); - free(device->pick_records); + if (device->pick_record_size == 0) + device->pick_record_size = 1; + else + device->pick_record_size *= 2; + device->pick_records = realloc(device->pick_records, + sizeof(*device->pick_records) * device->pick_record_size); } /* Fill record parameters */ - record = &new_record_array[device->pick_record_count]; + record = &device->pick_records[device->pick_record_count - 1]; record->bOpcode = current->bOpcode; record->bPad = 0; @@ -346,10 +345,6 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d /* Formula for returning the Z value at this X/Y */ record->dvZ = d3d_execute_buffer_z_value_at_coords(pick_rect->x1, pick_rect->y1, verts); - /* Point device info to new record information */ - device->pick_records = new_record_array; - device->pick_record_count = new_record_count; - /* We have a successful pick so we can skip the rest of the triangles */ instr += size * (count - i - 1); count = i; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Myah Caron <qsniyg(a)protonmail.com> --- dlls/ddraw/device.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dlls/ddraw/device.c b/dlls/ddraw/device.c index 5d850c53caf..586d189d882 100644 --- a/dlls/ddraw/device.c +++ b/dlls/ddraw/device.c @@ -1044,8 +1044,15 @@ static HRESULT WINAPI d3d_device1_Pick(IDirect3DDevice *iface, IDirect3DExecuteB iface, buffer, viewport, flags, wine_dbgstr_rect((RECT *)rect)); /* Sanity checks */ - if (!iface || !buffer || !viewport) + if (!buffer) { + WARN("NULL buffer, returning DDERR_INVALIDPARAMS\n"); + return DDERR_INVALIDPARAMS; + } + + if (!viewport) + { + WARN("NULL viewport, returning DDERR_INVALIDPARAMS\n"); return DDERR_INVALIDPARAMS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Myah Caron <qsniyg(a)protonmail.com> --- dlls/ddraw/executebuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ddraw/executebuffer.c b/dlls/ddraw/executebuffer.c index 99cd5710d7d..3dfc0832deb 100644 --- a/dlls/ddraw/executebuffer.c +++ b/dlls/ddraw/executebuffer.c @@ -160,7 +160,6 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d instr += sizeof(*current); primitive_size = 0; - /* We can skip these opcodes when Picking */ if (pick_rect) { switch (current->bOpcode) @@ -173,6 +172,7 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d case D3DOP_STATERENDER: case D3DOP_TEXTURELOAD: case D3DOP_SPAN: + FIXME("ignoring opcode %d for picking\n", current->bOpcode); instr += count * size; continue; default: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/executebuffer.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/dlls/ddraw/executebuffer.c b/dlls/ddraw/executebuffer.c index 3dfc0832deb..0d84590f4ac 100644 --- a/dlls/ddraw/executebuffer.c +++ b/dlls/ddraw/executebuffer.c @@ -302,17 +302,13 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, struct d3d box.right = box.left + vertex_size; if (FAILED(hr = wined3d_resource_map(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), - 0, &vert_map_desc, &box, WINED3D_MAP_WRITE))) - { + 0, &vert_map_desc, &box, WINED3D_MAP_READ))) return hr; - } - else - { - /* Copy vert data into stack array */ - verts[j] = *((D3DTLVERTEX*)vert_map_desc.data); - wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), 0); - } + /* Copy vert data into stack array */ + verts[j] = *((D3DTLVERTEX*)vert_map_desc.data); + + wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), 0); } /* Use vertices acquired above to test for clicking */ @@ -801,7 +797,7 @@ static HRESULT WINAPI d3d_execute_buffer_SetExecuteData(IDirect3DExecuteBuffer * desc.byte_width = new_size * sizeof(D3DTLVERTEX); desc.usage = WINED3DUSAGE_STATICDECL; - desc.access = WINED3D_RESOURCE_ACCESS_GPU | WINED3D_RESOURCE_ACCESS_MAP_W; + desc.access = WINED3D_RESOURCE_ACCESS_GPU | WINED3D_RESOURCE_ACCESS_MAP_R | WINED3D_RESOURCE_ACCESS_MAP_W; if (FAILED(hr = wined3d_buffer_create(buffer->d3ddev->wined3d_device, &desc, NULL, NULL, &ddraw_null_wined3d_parent_ops, &dst_buffer))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/device.c | 22 ++++++++++++---------- dlls/ddraw/tests/ddraw1.c | 6 +++--- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/dlls/ddraw/device.c b/dlls/ddraw/device.c index 586d189d882..3c8aefd2307 100644 --- a/dlls/ddraw/device.c +++ b/dlls/ddraw/device.c @@ -1091,21 +1091,23 @@ static HRESULT WINAPI d3d_device1_GetPickRecords(IDirect3DDevice *iface, TRACE("iface %p, count %p, records %p.\n", iface, count, records); + /* This causes an exception on Windows */ + if (!count) + { + ERR("count is NULL, returning DDERR_INVALIDPARAMS\n"); + return DDERR_INVALIDPARAMS; + } + wined3d_mutex_lock(); device = impl_from_IDirect3DDevice(iface); - /* Set count to the number of pick records we have */ - *count = device->pick_record_count; - - /* It is correct usage according to documentation to call this function with records == NULL - to retrieve _just_ the record count, which the caller can then use to allocate an - appropriately sized array, then call this function again to fill that array with data. */ - if (records && count) - { - /* If we have a destination array and records to copy, copy them now */ + /* If passed-in count does not match, fix it and do nothing else */ + if (*count != device->pick_record_count) + *count = device->pick_record_count; + /* If we have a destination array and records to copy, copy them now */ + else if (records) memcpy(records, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); - } wined3d_mutex_unlock(); diff --git a/dlls/ddraw/tests/ddraw1.c b/dlls/ddraw/tests/ddraw1.c index 1ce8c6bf71b..fcf500ed0a9 100644 --- a/dlls/ddraw/tests/ddraw1.c +++ b/dlls/ddraw/tests/ddraw1.c @@ -15585,9 +15585,9 @@ static void test_pick(void) hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); ok(SUCCEEDED(hr), "Failed to get pick records, hr %#lx.\n", hr); ok(rec_count == 3, "Got incorrect number of pick records (expected 3): %lu.\n", rec_count); - todo_wine ok(memcmp(&record[0], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); - todo_wine ok(memcmp(&record[1], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); - todo_wine ok(memcmp(&record[2], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + ok(memcmp(&record[0], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + ok(memcmp(&record[1], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); + ok(memcmp(&record[2], &empty_record, sizeof(D3DPICKRECORD)) == 0, "Got unexpected pick record.\n"); rec_count = 3; hr = IDirect3DDevice_GetPickRecords(device, &rec_count, &record[0]); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
From: Jeff Smith <whydoubt(a)gmail.com> --- dlls/ddraw/executebuffer.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/dlls/ddraw/executebuffer.c b/dlls/ddraw/executebuffer.c index 0d84590f4ac..5ee675ee802 100644 --- a/dlls/ddraw/executebuffer.c +++ b/dlls/ddraw/executebuffer.c @@ -68,14 +68,14 @@ static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) { * FALSE if not * *****************************************************************************/ -static BOOL d3d_execute_buffer_pick_test(LONG x, LONG y, D3DTLVERTEX* verts) +static BOOL d3d_execute_buffer_pick_test(LONG x, LONG y, const D3DTLVERTEX *verts) { UINT i; for (i = 0; i < TRIANGLE_SIZE; i++) { - D3DTLVERTEX* v1 = &verts[(i) % TRIANGLE_SIZE]; - D3DTLVERTEX* v2 = &verts[(i + 1) % TRIANGLE_SIZE]; + const D3DTLVERTEX *v1 = &verts[(i) % TRIANGLE_SIZE]; + const D3DTLVERTEX *v2 = &verts[(i + 1) % TRIANGLE_SIZE]; D3DVALUE bias = 0.0f; /* Edge function - determines whether pixel is inside triangle */ @@ -109,7 +109,7 @@ static BOOL d3d_execute_buffer_pick_test(LONG x, LONG y, D3DTLVERTEX* verts) * D3DPICKRECORD. * *****************************************************************************/ -static D3DVALUE d3d_execute_buffer_z_value_at_coords(LONG x, LONG y, D3DTLVERTEX* verts) +static D3DVALUE d3d_execute_buffer_z_value_at_coords(LONG x, LONG y, const D3DTLVERTEX *verts) { UINT i; @@ -118,9 +118,9 @@ static D3DVALUE d3d_execute_buffer_z_value_at_coords(LONG x, LONG y, D3DTLVERTEX for (i = 0; i < TRIANGLE_SIZE; i++) { - D3DTLVERTEX* v1 = &verts[i]; - D3DTLVERTEX* v2 = &verts[(i + 1) % TRIANGLE_SIZE]; - D3DTLVERTEX* v3 = &verts[(i + 2) % TRIANGLE_SIZE]; + const D3DTLVERTEX *v1 = &verts[i]; + const D3DTLVERTEX *v2 = &verts[(i + 1) % TRIANGLE_SIZE]; + const D3DTLVERTEX *v3 = &verts[(i + 2) % TRIANGLE_SIZE]; z1 += v3->sz * (x - v1->sx) * (y - v2->sy) - v2->sz * (x - v1->sx) * (y - v3->sy); z2 += (x - v1->sx) * (y - v2->sy) - (x - v1->sx) * (y - v3->sy); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
This series is very large; would you mind splitting it up into two or three smaller chunks? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420#note_40387
```diff + /* Create new array to fit one more record */ + DWORD new_record_count = device->pick_record_count + 1; + D3DPICKRECORD* new_record_array = malloc(sizeof(*device->pick_records) * new_record_count); + if (device->pick_record_count > 0) + { + memcpy(new_record_array, device->pick_records, sizeof(*device->pick_records) * device->pick_record_count); + free(device->pick_records); + } ```
This was already there in v1—style issues and all—and it's the kind of thing that tends to make a reviewer's desire to look at the more complicated bits of a patch evaporate. Patch 08/13 somewhat improves on this, but of course doesn't handle e.g. realloc() failure either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420#note_40397
This merge request was closed by Jeffrey Smith. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420
As I expressed in IRC, in this series I was attempting to provide as accurate a chain of ownership for the patches as reasonably possible, as two people have worked on this before me. This is clearly less important that a clean patch series. I am working now to provide something to that end. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420#note_40401
As I expressed in IRC, in this series I was attempting to provide as accurate a chain of ownership for the patches as reasonably possible, as two people have worked on this before me. This is clearly less important that a clean patch series. I am working now to provide something to that end.
For what it's worth, I can't claim to have a good understanding of this procedure, but I get the impression that authorship isn't usually specified with quite this level of granularity, or if it is, it's sufficient to describe co-authorship in the patch subject, e.g. "with fixes by X" or "based on a patch by X". Anyway, with respect to floating point errors, I'm inclined to say we do care about accuracy as best we can. If there are demonstrable errors compared to native, I think we should try to determine why they're occurring, and if they can be fixed. One thing that may help, as pointed out to me by Henri, is to perform these calculations in fixed-point instead of floating-point. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3420#note_41470
participants (6)
-
Henri Verbeet (＠hverbeet) -
Jeff Smith
-
Jeffrey Smith (＠whydoubt)
-
Matthew Wong
-
Myah Caron
-
Zebediah Figura (＠zfigura)