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

[Aaryaman Vasishta]

On Sun, Jul 10, 2016 at 9:39 PM, Stefan Dösinger <stefandoesinger at gmail.com>
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160710/6c78b479/attachment.html>