[PATCH] 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=55044

Your paranoid android.

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

Report errors:
ddraw:ddraw1 has no test summary line (early exit of the main process?)
ddraw:ddraw1 has unaccounted for todo messages
ddraw:ddraw2 has no test summary line (early exit of the main process?)
ddraw:ddraw2 has unaccounted for todo messages
ddraw:ddraw4 has no test summary line (early exit of the main process?)
ddraw:ddraw4 has unaccounted for todo messages
ddraw:ddraw7 has no test summary line (early exit of the main process?)
ddraw:ddraw7 has unaccounted for todo messages

=== debian10 (32 bit Chinese:China report) ===

Report errors:
ddraw:ddraw1 has no test summary line (early exit of the main process?)
ddraw:ddraw1 has unaccounted for todo messages
ddraw:ddraw2 has no test summary line (early exit of the main process?)
ddraw:ddraw2 has unaccounted for todo messages
ddraw:ddraw4 has no test summary line (early exit of the main process?)
ddraw:ddraw4 has unaccounted for todo messages
ddraw:ddraw7 has no test summary line (early exit of the main process?)
ddraw:ddraw7 has unaccounted for todo messages

=== debian10 (build log) ===

X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  140 (RANDR)
  Minor opcode of failed request:  7 (RRSetScreenSize)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)

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

ddraw:
ddrawmodes.c:936: Test failed: Fullscreen window has wrong size.
dsurface.c:1153: Test failed: Surface width differs from screen width
dsurface.c:1154: Test failed: Surface height differs from screen height

Report errors:
ddraw:ddraw1 has no test summary line (early exit of the main process?)
ddraw:ddraw1 has unaccounted for todo messages

=== debian10 (64 bit WoW report) ===

Report errors:
ddraw:ddraw1 has no test summary line (early exit of the main process?)
ddraw:ddraw1 has unaccounted for todo messages
ddraw:ddraw2 has no test summary line (early exit of the main process?)
ddraw:ddraw2 has unaccounted for todo messages
ddraw:ddraw4 has no test summary line (early exit of the main process?)
ddraw:ddraw4 has unaccounted for todo messages
ddraw:ddraw7 has no test summary line (early exit of the main process?)
ddraw:ddraw7 has unaccounted for todo messages

=== debian10 (build log) ===

X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  140 (RANDR)
  Minor opcode of failed request:  7 (RRSetScreenSize)
X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  140 (RANDR)
  Minor opcode of failed request:  7 (RRSetScreenSize)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  151 (XFree86-VidModeExtension)
  Minor opcode of failed request:  10 (XF86VidModeSwitchToMode)

[Henri Verbeet]

Hi Matthew,

On Thu, 1 Aug 2019 at 23:36, MattKC  wrote:
> Picking has been implemented by reusing the code from
> d3d_execute_buffer_execute() with the drawing functions removed
> leaving the core vertex processing and adding click tests for creating
> an array of D3DPICKRECORDs.

That strikes me as less than ideal. Which alternatives did you
consider? Also, do you have tests to go along with this?

Henri

[MattKC]

On 8/3/19 1:16 AM, Henri Verbeet wrote:
> Hi Matthew,
>
> On Thu, 1 Aug 2019 at 23:36, MattKC  wrote:
> > Picking has been implemented by reusing the code from
> > d3d_execute_buffer_execute() with the drawing functions removed
> > leaving the core vertex processing and adding click tests for creating
> > an array of D3DPICKRECORDs.
>
> That strikes me as less than ideal. Which alternatives did you
> consider? Also, do you have tests to go along with this?
>
> Henri

Hi Henri,

I would agree it's not very efficient, however I believe it is accurate
to how the function is intended to work according to the SDK
documentation.

The documentation says:

"The IDirect3DDevice::Pick method executes a buffer without performing
any rendering, but returns a z-ordered list of offsets to the primitives
that intersect the upper-left corner of the rectangle specified by
lpRect."

I don't have any tests I'm afraid, I used the two games I mentioned to
develop this patch.

Matt

[Henri Verbeet]

On Fri, 2 Aug 2019 at 23:16, MattKC  wrote:
> I would agree it's not very efficient, however I believe it is accurate
> to how the function is intended to work according to the SDK
> documentation.

DirectDraw 1 applications tend to not be especially performance
critical on modern hardware, so in that regard it's fine. What is a
concern though is d3d_execute_buffer_execute_pick() diverging from
d3d_execute_buffer_execute() when changes are made to one or the
other.
> I don't have any tests I'm afraid, I used the two games I mentioned to
> develop this patch.

Having tests is important, both in order to be sure the implementation
is correct as well as to prevent future changes from breaking it.

[MattKC]

On 8/7/19 12:48 AM, Henri Verbeet wrote:
> What is a concern though is d3d_execute_buffer_execute_pick() diverging
> from d3d_execute_buffer_execute() when changes are made to one or the
> other.

Ah I see, I felt inclined not to modify the d3d_execute_buffer_execute()
function to try and keep my changes as clear as possible, however an
alternative could be adding my triangle click test to
d3d_execute_buffer_execute() with an extra parameter that determines
whether it runs in "pick mode" (click test, no render) or not.

I'm not sure what other alternatives there could be without looping
through the execute buffer (which is essentially the current
solution). Perhaps if I could reliably determine where triangle vertices
are written to during the execute (without having to read the execute
buffer again) I could run the click test on those (keeping
d3d_execute_buffer_execute() unmodified), but I'm not sure how practical
that would be.
> Having tests is important, both in order to be sure the implementation
> is correct as well as to prevent future changes from breaking it.

I can see about writing a test, assuming at this point that I'll
be modifying the patch anyway to address the first issue.