Re: [PATCH 02/12] d3drm: Implement frame InverseTransform

[Henri Verbeet]

On Fri, 14 Jun 2019 at 09:00, Jeff Smith  wrote:
> +static void matrix_multiply(D3DRMMATRIX4D d, D3DRMMATRIX4D s1, D3DRMMATRIX4D s2)
> +{
> - d[0][0] = s1[0][0] * s2[0][0] + s1[0][1] * s2[1][0] + s1[0][2] * s2[2][0];
> - d[0][1] = s1[0][0] * s2[0][1] + s1[0][1] * s2[1][1] + s1[0][2] * s2[2][1];
> - d[0][2] = s1[0][0] * s2[0][2] + s1[0][1] * s2[1][2] + s1[0][2] * s2[2][2];
> - d[0][3] = 0.0f;
> - d[1][0] = s1[1][0] * s2[0][0] + s1[1][1] * s2[1][0] + s1[1][2] * s2[2][0];
> - d[1][1] = s1[1][0] * s2[0][1] + s1[1][1] * s2[1][1] + s1[1][2] * s2[2][1];
> - d[1][2] = s1[1][0] * s2[0][2] + s1[1][1] * s2[1][2] + s1[1][2] * s2[2][2];
> - d[1][3] = 0.0f;
> - d[2][0] = s1[2][0] * s2[0][0] + s1[2][1] * s2[1][0] + s1[2][2] * s2[2][0];
> - d[2][1] = s1[2][0] * s2[0][1] + s1[2][1] * s2[1][1] + s1[2][2] * s2[2][1];
> - d[2][2] = s1[2][0] * s2[0][2] + s1[2][1] * s2[1][2] + s1[2][2] * s2[2][2];
> - d[2][3] = 0.0f;
> - d[3][0] = s1[3][0] * s2[0][0] + s1[3][1] * s2[1][0] + s1[3][2] * s2[2][0] + s2[3][0];
> - d[3][1] = s1[3][0] * s2[0][1] + s1[3][1] * s2[1][1] + s1[3][2] * s2[2][1] + s2[3][1];
> - d[3][2] = s1[3][0] * s2[0][2] + s1[3][1] * s2[1][2] + s1[3][2] * s2[2][2] + s2[3][2];
> - d[3][3] = 1.0f;
>
> +}

That's not a regular 4x4 matrix multiplication. (Which may be fine,
but the function name should make that obvious.)
> +static struct d3drm_frame *matrix_expand(D3DRMMATRIX4D d, struct d3drm_frame *s,
> -
>    struct d3drm_frame *shortcut)
>
> +{
> - D3DRMMATRIX4D m;
> - memcpy(d, s->transform, sizeof(D3DRMMATRIX4D));
> - while ((s = s->parent))
> - {
> -
>    if (s == shortcut)
> -
>        return NULL;
> -
>    memcpy(m, d, sizeof(D3DRMMATRIX4D));
> -
>    matrix_multiply(d, m, s->transform);
> - }
> - return shortcut;
>
> +}

You're always calling this with NULL "shortcut", i.e., the "shortcut"
handling is effectively dead code.
> +static void matrix_invert(D3DRMMATRIX4D d, D3DRMMATRIX4D s)
> +{
> - D3DVALUE determinant;
> - D3DVALUE cofac[3];
> -
> - cofac[0] = s[1][1] * s[2][2] - s[1][2] * s[2][1];
> - cofac[1] = s[1][2] * s[2][0] - s[1][0] * s[2][2];
> - cofac[2] = s[1][0] * s[2][1] - s[1][1] * s[2][0];
> -
> - determinant = s[0][0] * cofac[0] + s[0][1] * cofac[1] + s[0][2] * cofac[2];
> -
> - if (determinant == 0.0f)
> - {
> -
>    memcpy(d, identity, sizeof(D3DRMMATRIX4D));
> -
>    return;
> - }
> -
> - d[0][0] = cofac[0] / determinant;
> - d[0][1] = (s[2][1] * s[0][2] - s[2][2] * s[0][1]) / determinant;
> - d[0][2] = (s[0][1] * s[1][2] - s[0][2] * s[1][1]) / determinant;
> - d[0][3] = 0.0f;
> - d[1][0] = cofac[1] / determinant;
> - d[1][1] = (s[2][2] * s[0][0] - s[2][0] * s[0][2]) / determinant;
> - d[1][2] = (s[0][2] * s[1][0] - s[0][0] * s[1][2]) / determinant;
> - d[1][3] = 0.0f;
> - d[2][0] = cofac[2] / determinant;
> - d[2][1] = (s[2][0] * s[0][1] - s[2][1] * s[0][0]) / determinant;
> - d[2][2] = (s[0][0] * s[1][1] - s[0][1] * s[1][0]) / determinant;
> - d[2][3] = 0.0f;
> - d[3][0] = -(d[0][0] * s[3][0] + d[1][0] * s[3][1] + d[2][0] * s[3][2]);
> - d[3][1] = -(d[0][1] * s[3][0] + d[1][1] * s[3][1] + d[2][1] * s[3][2]);
> - d[3][2] = -(d[0][2] * s[3][0] + d[1][2] * s[3][1] + d[2][2] * s[3][2]);
> - d[3][3] = 1.0f;
>
> +}

While this approach has the advantage of being relatively
straightforward, it has the disadvantage of not being the most
numerically stable approach. Note that you don't actually need to
invert the matrix to get the inverse transform though; it probably
makes more sense to decompose it instead.

This is also one of the harder patches in the series, you may want to
consider moving the easier ones to the front instead.