[PATCH v2] ddraw: implement Pick() and GetPickRecords().

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=55568

Your paranoid android.

=== wvistau64_zh_CN (32 bit report) ===

ddraw:
ddraw1.c:2801: Test failed: Expected message 0x46, but didn't receive it.
ddraw1.c:2803: Test failed: Expected screen size 1024x768, got 0x0.
ddraw1.c:2809: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2839: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2846: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2872: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2895: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2924: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2950: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:2970: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3006: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3016: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3042: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3065: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3087: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3113: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3133: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).
ddraw1.c:3170: Test failed: Expected (0,0)-(1024,768), got (-8,-8)-(1032,745).

=== wvistau64_fr (32 bit report) ===

ddraw:
ddraw1: Timeout

=== wvistau64_fr (task log) ===

Task errors:
An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9)
The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died)
The previous 2 run(s) terminated abnormally

=== wvistau64_he (32 bit report) ===

ddraw:
ddraw1: Timeout

=== wvistau64_he (task log) ===

Task errors:
An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9)
The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died)
The previous 2 run(s) terminated abnormally

=== w8 (32 bit report) ===

ddraw:
ddraw1.c:11039: Test failed: Got unexpected color 0x00ffffff.

=== debian10 (32 bit report) ===

ddraw:
ddraw1: Timeout
ddraw2: Timeout
ddraw4: Timeout
ddraw7: Timeout
ddrawmodes: Timeout
dsurface: Timeout

=== debian10 (build log) ===

Task errors:
The task timed out

=== debian10 (32 bit WoW report) ===

ddraw:
ddraw1: Timeout
ddraw2: Timeout
ddraw4: Timeout
ddraw7: Timeout
ddrawmodes: Timeout

=== debian10 (build log) ===

Task errors:
The task timed out

[MattKC]

Any updates on reviewing this patch?

On 8/18/2019 2꞉25 PM, MattKC wrote:
>

[MattKC]

I sent this patch back in August but it seemed to disappear with no 
comments or feedback, therefore I am re-submitting it.

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=59037

Your paranoid android.

=== wvistau64_fr (task log) ===

Task errors:
The previous 1 run(s) terminated abnormally

=== w8 (32 bit report) ===

ddraw:
ddraw1.c:11039: Test failed: Got unexpected color 0x00ffffff.

[Stefan Dösinger]

Hi,

Here are a few comments - Henri may have more comments afterwards, and I
haven't run the tests on Windows yet.
> - /* Pick data */
> - D3DPICKRECORD* pick_records;

Please use "D3DPICKRECORD *pick_records" - the C++ style is misleading
when combined with declaring more than one variable in one line.
> -
>    /* Free pick records array if any are allocated */

I think the comment is redundant, but I don't have strong opinions on this.
> -
>    if (This->pick_record_count > 0) {
> -
>        free(This->pick_records);
> -
>    }

Please use heap_alloc() and heap_free() instead of malloc() and free().
The reason is that malloc() and free() go to the Linux libc library
whereas heap_alloc() and heap_free() go through ntdll.dll and Wine's
heap manager. [This is only true for libs that haven't been migrated to
mingw yet though, but consistency...]

Also, please put the brackets into an extra line, as you did in the tests.
> static HRESULT WINAPI d3d_device1_Pick(IDirect3DDevice *iface, IDirect3DExecuteBuffer *buffer,
>          IDirect3DViewport *viewport, DWORD flags, D3DRECT *rect)

...
> - /* Sanity checks */
> - if (!iface || !buffer || !viewport) {
> -
>    return DDERR_INVALIDPARAMS;
> - }

iface can't be NULL because the caller needs it to get through the
vtable and the method pointer - C++ would call it a virtual function.

Please write a WARN whenever you are returning an error to the
application. It may indicate that a problem happened earlier and eases
debugging.
> - if (FAILED(hr = IDirect3DDevice3_SetCurrentViewport
> -
>        (&device->IDirect3DDevice3_iface, &viewport_impl->IDirect3DViewport3_iface)))
> -
>    return hr;

Just for reference, there's no reason to write a WARN here because
SetCurrentViewport should do it if it fails.
> static HRESULT WINAPI d3d_device1_GetPickRecords(IDirect3DDevice *iface,
>          DWORD *count, D3DPICKRECORD *records)
> ...
> - /* Set count to the number of pick records we have */
> - *count = device->pick_record_count;

Here it might make sense to check count != NULL. If that crashes on
Windows add a comment that Windows doesn't check it either.
> -
>    /* If we have a destination array and records to copy, copy them now */
> -
>    memcpy(records, device->pick_records, sizeof(D3DPICKRECORD) * device->pick_record_count);

sizeof(D3DPICKRECORD) -> sizeof(*device->pick_records). Just in case
someone changes the type of pick_records the sizeof will do the right thing.

The other question - to be answered by extending the test - is which
count is used here. Should you copy all pick records in the device, or
*count pick records, or whichever is smaller, or return an error if
*count < device->pick_record_count.

See also my comment where you allocate the record array.
> -
>    /* We're now done with the old pick records and can free them */
> -
>    free(device->pick_records);
> -
>    device->pick_record_count = 0;

Please add a test for resetting the stored pick records on
GetPickRecords. It might also reset the next time IDirect3DDevice::Pick
is called, and calling GetPickRecords twice will return the same records
over.
> - /* Initialize pick records array */
> - device->pick_record_count = 0;

This should be initialized to 0 by heap_alloc() already.
> - INT i;

INT -> unsigned int
> - for (i=0;i<TRIANGLE_SIZE;i++)

Can an application pick lines or points by emitting a line or point draw
inside the execute buffer?

The actual math you're doing is a bit over my head now - I'll have to
read up some specs, assuming they exist...
> -
>            /* None of these opcodes seem to be necessary for picking */
> -
>            case D3DOP_POINT:
> -
>            case D3DOP_LINE:

Those might add vertices to the pick list.
> -
>            case D3DOP_STATETRANSFORM:

And those change the transform matrices, thus moving your vertices
around if you are using D3DPROCESSVERTICES_* ops other than _COPY.
> -
>            case D3DOP_STATELIGHT:
> -
>            case D3DOP_STATERENDER:
> -
>            case D3DOP_TEXTURELOAD:
> -
>            case D3DOP_SPAN:

In regular executions those ops affect global device state that persists
after the Execute call returns. They might do the same thing in Pick().

Testing this isn't trivial because d3d1 does not have getters for these
states. So you'd have to do an actual draw to e.g. see the effect of fog
enable / disable after you do a pick() that changes the fog state. I
think this is overkill for your patch, so just writing a FIXME seems
much better.
> -
>                                if (FAILED(hr = wined3d_resource_map(wined3d_buffer_get_resource(buffer->dst_vertex_buffer),
> -
>                                        0, &vert_map_desc, &box, WINED3D_MAP_WRITE))) {

You are reading from the buffer, but not writing so WINED3D_MAP_READ is
appropriate.
> -
>                                /* Create new array to fit one more record */
> -
>                                DWORD new_record_count = device->pick_record_count + 1;
> -
>                                D3DPICKRECORD* new_record_array = malloc(sizeof(D3DPICKRECORD) * new_record_count);
> -
>                                if (device->pick_record_count > 0) {
> -
>                                    memcpy(new_record_array, device->pick_records, sizeof(D3DPICKRECORD) * device->pick_record_count);
> -
>                                    free(device->pick_records);
> -
>                                }

This is inefficient. It'll do O(N) malloc and free calls. You can reduce
this to O(log(N)) calls the following way, and at the same time reduce
the churn between Pick and GetPickEntries calls:

Keep separate pick_record_count and pick_record_size members in the
device, the former one for the number of records currently written to
the array, and the second one for the size of the array. When the array
content gets reset (after GetPickRecords or at the start of Pick,
depending on what the tests say), set pick_record_count to 0, but don't
free the array. You can reuse the allocated memory the next time.

When you run out of space, allocate pick_record_size * 2 records (i.e.,
double the size), or use a different factor. Beware obviously of the
special case of pick_records_size = 0. Use heap_realloc instead of alloc
+ copy + free. heap_realloc will retain existing data, and it will give
you the same pointer back if the heap manager can just append more space
right behind the already allocated block.

See e.g. ddraw_find_decl() in ddraw.c for code that already does
something like this.
> -
>    ok(record.dvZ > 0.99 && record.dvZ < 1.01, "Got incorrect

dvZ: %f.\n", record.dvZ);
There's compare_float() for that.

As for the corner case you mention in the patch description: One tricky
bit about d3d before d3d10 is that the pixel origin is in the top left
corner, not the pixel center. So adding or subtracting 0.5 may be
necessary in some cases.