Re: [PATCH 2/2] d3drm: Store animation options

[Henri Verbeet]

On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options)
> +{
> - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface);
> - static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION
> -
>    | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION;
> -
> - TRACE("iface %p, options %#x.\n", iface, options);
> -
> - if (options && !(options & supported_options))
> -
>    return D3DRMERR_BADVALUE;

Should that be "if (options & ~supported_options)"? The test is
ambiguous about that.
> @@ -6776,6 +6777,42 @@ static void test_animation(void)
>      IDirect3DRMFrame3_Release(frame3);
>      IDirect3DRMFrame_Release(frame);
> - /* Animation options. */
> - options = IDirect3DRMAnimation_GetOptions(animation);
> - ok(options == (D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION),
> -
>        "Unexpected default options %#x.\n", options);
> -
> - /* Undefined mask value */
> - hr = IDirect3DRMAnimation_SetOptions(animation, 0xdeadbeef);
> - ok(hr == D3DRMERR_BADVALUE, "Unexpected hr %#x.\n", hr);

0xdeadbeef is a bit of a poor test value here, since it will trigger
both the "D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED" and
"D3DRMANIMATION_LINEARPOSITION | D3DRMANIMATION_SPLINEPOSITION" paths
as well.

Is it valid to set 0 as an option? Is it valid to set e.g.
D3DRMANIMATION_OPEN, but not one of D3DRMANIMATION_LINEARPOSITION and
D3DRMANIMATION_SPLINEPOSITION? I.e., is it just invalid to set
conflicting options, or should always exactly one of OPEN/CLOSED and
LINEARPOSITION/SPLINEPOSITION be set?

[PATCH 2/2] d3drm: Store animation options

[Nikolay Sivov]

On 27.06.2017 15:43, Henri Verbeet wrote:
> On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> > +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options)
> > +{
> > - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface);
> > - static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION
> > -
> >    | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION;
> > -
> > - TRACE("iface %p, options %#x.\n", iface, options);
> > -
> > - if (options && !(options & supported_options))
> > -
> >    return D3DRMERR_BADVALUE;
>
> Should that be "if (options & ~supported_options)"? The test is
> ambiguous about that.

No, they both are wrong. It should be just "if (!(options &
supported_options)", because 0 is not allowed.
> > @@ -6776,6 +6777,42 @@ static void test_animation(void)
> >      IDirect3DRMFrame3_Release(frame3);
> >      IDirect3DRMFrame_Release(frame);
> > - /* Animation options. */
> > - options = IDirect3DRMAnimation_GetOptions(animation);
> > - ok(options == (D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION),
> > -
> >        "Unexpected default options %#x.\n", options);
> > -
> > - /* Undefined mask value */
> > - hr = IDirect3DRMAnimation_SetOptions(animation, 0xdeadbeef);
> > - ok(hr == D3DRMERR_BADVALUE, "Unexpected hr %#x.\n", hr);
>
> 0xdeadbeef is a bit of a poor test value here, since it will trigger
> both the "D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED" and
> "D3DRMANIMATION_LINEARPOSITION | D3DRMANIMATION_SPLINEPOSITION" paths
> as well.

Right.
> Is it valid to set 0 as an option? Is it valid to set e.g.
> D3DRMANIMATION_OPEN, but not one of D3DRMANIMATION_LINEARPOSITION and
> D3DRMANIMATION_SPLINEPOSITION? I.e., is it just invalid to set
> conflicting options, or should always exactly one of OPEN/CLOSED and
> LINEARPOSITION/SPLINEPOSITION be set?

Setting 0 is invalid. For the rest there's already a test setting only
_OPEN. So yes, only conflicting combinations are checked, and I found
another one actually - when both _SCALEANDROTATION and _POSITION are
set. Bottom line is spline and linear mode bits are bad design, there's
no need in two flags when you'll need one of two modes anyway; and you
can disable both whatever this means.

[PATCH 2/2] d3drm: Store animation options

[Henri Verbeet]

On 27 June 2017 at 16:33, Nikolay Sivov  wrote:
> On 27.06.2017 15:43, Henri Verbeet wrote:
> > On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> > > +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options)
> > > +{
> > > - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface);
> > > - static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION
> > > -
> > >    | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION;
> > > -
> > > - TRACE("iface %p, options %#x.\n", iface, options);
> > > -
> > > - if (options && !(options & supported_options))
> > > -
> > >    return D3DRMERR_BADVALUE;
> >
> > Should that be "if (options & ~supported_options)"? The test is
> > ambiguous about that.
>
> No, they both are wrong. It should be just "if (!(options &
> supported_options)", because 0 is not allowed.

What about unrecognised options? E.g., would 0x80000001 be allowed or not?
> Bottom line is spline and linear mode bits are bad design,

Yeah, but that goes for much of ddraw/d3d up to about version 9 or 10.

[PATCH 2/2] d3drm: Store animation options

[Nikolay Sivov]

On 27.06.2017 17:43, Henri Verbeet wrote:
> On 27 June 2017 at 16:33, Nikolay Sivov  wrote:
> > On 27.06.2017 15:43, Henri Verbeet wrote:
> > > On 27 June 2017 at 10:17, Nikolay Sivov  wrote:
> > > > +static HRESULT WINAPI d3drm_animation2_SetOptions(IDirect3DRMAnimation2 *iface, D3DRMANIMATIONOPTIONS options)
> > > > +{
> > > > - struct d3drm_animation *animation = impl_from_IDirect3DRMAnimation2(iface);
> > > > - static const DWORD supported_options = D3DRMANIMATION_OPEN | D3DRMANIMATION_CLOSED | D3DRMANIMATION_LINEARPOSITION
> > > > -
> > > >    | D3DRMANIMATION_SPLINEPOSITION | D3DRMANIMATION_SCALEANDROTATION | D3DRMANIMATION_POSITION;
> > > > -
> > > > - TRACE("iface %p, options %#x.\n", iface, options);
> > > > -
> > > > - if (options && !(options & supported_options))
> > > > -
> > > >    return D3DRMERR_BADVALUE;
> > >
> > > Should that be "if (options & ~supported_options)"? The test is
> > > ambiguous about that.
> >
> > No, they both are wrong. It should be just "if (!(options &
> > supported_options)", because 0 is not allowed.
>
> What about unrecognised options? E.g., would 0x80000001 be allowed or not?

Yes, it is allowed, and stored as is - unsupported bits are not filtered
out. So patch v2 works correctly in that regard, only additional test is
missing.
> > Bottom line is spline and linear mode bits are bad design,
>
> Yeah, but that goes for much of ddraw/d3d up to about version 9 or 10.