[PATCH 3/4] d3drm: Initial support for storing animation keys

[Henri Verbeet]

On 4 July 2017 at 09:43, Nikolay Sivov <nsivov at codeweavers.com> wrote:
> +static int d3drm_animation_cmp_min(const struct d3drm_animation_keys *keys, D3DVALUE left, D3DVALUE right, DWORD *cur)
> +{
> +    if (left == right)
> +    {
> +        while (*cur > 0 && left == keys->keys[*cur - 1].time)
> +            (*cur)--;
> +        return 0;
> +    }
Does this really need to happen inside the comparison function? I.e.,
couldn't you just use a single comparison function, bsearch(), and do
the fixup for duplicates afterwards?

> +static DWORD d3drm_animation_get_index(struct d3drm_animation_keys *keys, D3DVALUE time,
> +    int (*compare_func)(const struct d3drm_animation_keys *keys, D3DVALUE left, D3DVALUE right, DWORD *cur))
> +{
> +    int min = 0, max = keys->count - 1;
unsigned/SIZE_T, right?

> +static BOOL d3drm_animation_get_range(struct d3drm_animation_keys *keys, D3DVALUE time_min, D3DVALUE time_max,
> +        DWORD *min, DWORD *max)
> +{
> +    if (keys->count == 0)
> +        return FALSE;
> +
> +    if (time_min > keys->keys[keys->count - 1].time || time_max < keys->keys[0].time)
> +        return FALSE;
> +
> +    *min = d3drm_animation_get_index(keys, time_min, d3drm_animation_cmp_min);
> +    *max = d3drm_animation_get_index(keys, time_max, d3drm_animation_cmp_max);
This is pretty minor, but note that to find *max you'd only have to
search through *min to the end, instead of through the entire range.

> +    if (!key || key->dwSize != sizeof(*key))
> +        return E_INVALIDARG;
We usually print a WARN for cases where the application passes us
questionable data. That's certainly not a strict requirement, but on
occasion it does tend to make it slightly easier to figure out where
things start failing.

> +        memmove(keys->keys + index + 1, keys->keys + index, sizeof(*keys->keys) * (keys->count - index));
We'd typically write those as "&keys->keys[index + 1]", etc. That's
purely stylistic, of course.

> +    if (d3drm_animation_get_range(&animation->rotate, time_min, time_max, &min, &max))
> +    {
> +        *count += max - min + 1;
> +
> +        for (i = min; i <= max && keys; i++, cur++)
> +        {
> +            keys[cur].dwSize = sizeof(*keys);
> +            keys[cur].dwKeyType = D3DRMANIMATION_ROTATEKEY;
> +            keys[cur].dvTime = animation->rotate.keys[i].time;
> +            keys[cur].dwID = 0; /* FIXME */
> +            keys[cur].u.dqRotateKey = animation->rotate.keys[i].u.rotate;
> +        }
> +    }
Minor, but note that it would probably be more convenient if
d3drm_animation_get_range() returned a pointer to
animation->rotate.keys[min] and a count.