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

Hi, Here are a few comments - Henri may have more comments afterwards, and I haven't run the tests on Windows yet. Please use "D3DPICKRECORD *pick_records" - the C++ style is misleading when combined with declaring more than one variable in one line. I think the comment is redundant, but I don't have strong opinions on this. 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. ... 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. Just for reference, there's no reason to write a WARN here because SetCurrentViewport should do it if it fails. Here it might make sense to check count != NULL. If that crashes on Windows add a comment that Windows doesn't check it either. 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. 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. This should be initialized to 0 by heap_alloc() already. INT -> unsigned int 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... Those might add vertices to the pick list. And those change the transform matrices, thus moving your vertices around if you are using D3DPROCESSVERTICES_* ops other than _COPY. 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. You are reading from the buffer, but not writing so WINED3D_MAP_READ is appropriate. 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. 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.