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

[Henri Verbeet]

On 4 July 2017 at 09:43, Nikolay Sivov <nsivov(a)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.