[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 <nsivov at codeweavers.com> 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.