[PATCH 07/12] d3drm: Implement IDirect3DRMViewport*::Init. (v4)

Sun Jul 10 11:09:01 CDT 2016

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Am 2016-07-07 um 14:52 schrieb Aaryaman Vasishta:
> -        {&CLSID_CDirect3DRMTexture, d3drm_create_texture_object},
> -        {&CLSID_CDirect3DRMDevice, d3drm_create_device_object},
> +        { &CLSID_CDirect3DRMTexture,  d3drm_create_texture_object  },
> +        { &CLSID_CDirect3DRMDevice, d3drm_create_device_object     },
> +        { &CLSID_CDirect3DRMViewport, d3drm_create_viewport_object },
I guess Visual Studio or xcode re-added the spaces between { and &. Henri removed them in the slightly modified version of "d3drm: Use a table in d3drm3_CreateObject() to create objects in a generic manner."

> +    if (FAILED(hr = IDirect3DRMFrame3_QueryInterface(camera, &IID_IDirect3DRMFrame, (void **)&viewport->camera)))
> +        goto cleanup;
Aren't you more likely to need the implementation structure of the camera in the future?

> +
> +    color = IDirect3DRMFrame3_GetSceneBackground(camera);
> +    /* Create material (ambient/diffuse/emissive?), set material */
> +    if (FAILED(hr = IDirect3D_CreateMaterial(d3d1, &material, NULL)))
> +        goto cleanup;
> +
> +    memset(&mat, 0, sizeof(mat));
> +    memcpy(&mat.diffuse, &color, sizeof(color));
> +
> +    if (FAILED(hr = IDirect3DMaterial_SetMaterial(material, &mat)))
> +        goto cleanup;
> +
> +    if (FAILED(hr = IDirect3DMaterial_GetHandle(material, d3d_device, &hmat)))
> +        goto cleanup;
> +
> +    hr = IDirect3DViewport_SetBackground(viewport->d3d_viewport, hmat);
> +
> +cleanup:
> +
> +    if (material)
> +        IDirect3DMaterial_Release(material);
In the success case this will destroy the material you just created. Our IDirect3DViewport::SetBackground implementation does not AddRef the material. I don't think we have any tests for this, but because SetBackground accepts a handle instead of a COM object I don't expect it to AddRef.

Overall it seems questionable to call camera->GetSceneBackground and apply the color here. I'd expect that the camera background can be changed after creating the viewport.