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

[Aaryaman Vasishta]

On Sun, Jul 10, 2016 at 9:39 PM, Stefan Dösinger 
wrote:
> -----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."

True. I actually thought this was fine since the QI test tables were
formatted in a similar way (see test_d3drm_qi, test_texture_qi etc).  But I
guess the current format is considered better, so I'll go with that for the
implementations.
> > - 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?

You mean accessing the struct directly instead of via the interface
functions? I'm fine with that too. The interface functions do provide a way
to get most of what's needed (I could be wrong though, maybe some internal
data that's not revealed outside can still be utilized within calls), but I
guess using the struct directly would be cleaner, and safer in the long
run.
> > -
> > - 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.

Ah, I was under the impression that IDirect3DViewport keeps a reference to
the material (should have studied its implementation here). So I guess a
reference to the material should be stored in the struct as well, and
released on destroy.

Cheers,
Aaryaman