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

[Stefan Dösinger]

-----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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXgnMdAAoJEN0/YqbEcdMwk+IP/2DZH4tPgtCN/PHoB/ZdsVwJ
oodz6Sax5LZUaumbXZme4CPgPZKSKr44ue3qho/5cQicRISuJ3G6I6vFmkiiHol6
nFmY9cTJiNeXzd6NcJq5KK6IUmgrzO5YjGgXIbzgzEiTycbws/lo5fVwF74LSYn7
RMPg+U4qMaHVT/w/rb7dWWqPugSbm8cIGfp8Zjz2VrO+JX+S1Qs3ymCZViyMfCRz
A1VtyJh/u72GlfCoRoUiT6DDsMjw1BCMogR4asDkQmoTdkc7XFRU3MYQwUN7cFY2
bxos78d6xozUwvDRoezrtRLh4gQis9gudaNYv4Dt4fZjNOSvCtA8fMbl35gXuJog
ooROiaXSoW5oBrzv++Fvp38cixS0j9MMECNDgs1yCCKt0a/B4/IFEQMvyGt5LzRG
zeJ2XzDEZI3ntsbeVdKPRuH6m7ROlQLfp+Q0ac+MrhJSQtnAd88mXVfU5U8330b4
DNyeSmyPhBpaRp62WZxMCeOct7psy1p4LYkbbE5QZ3eFS2QhHlnPC6PzNtcUbDqI
RG4J+ZhquDnT57ViyjX1vKt6tXPlDOkwJf0AApwQ84OGs7ATNci/7Z1WuyULx9sR
xPrpNYUPbFIxcadmvaa7jO+aaOXoNWFQQz7rJdCVBxUY1T4Bz8+FyzQ0O8Ld/xNj
b7LndQJz3AbU//fQ1gdO
=8gsI
-----END PGP SIGNATURE-----

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

[Aaryaman Vasishta]

On Sun, Jul 10, 2016 at 9:39 PM, Stefan Dösinger <stefandoesinger(a)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

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

[Stefan Dösinger]

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

Am 2016-07-10 um 17:34 schrieb Aaryaman Vasishta:
> 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 you expect that the external API is enough stick to the interface.
> It's no big issue to change it later if needed.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXgoRjAAoJEN0/YqbEcdMwcBcQAI/7D9EYR3M+/ctNPdf1q2C7
YTZnYO6zbSUwflEP5zt7T2rDzZ+eG7PeLowu3LE+pNdUAkbLGGbJQ7tIaBTsYLG2
lOFCaqNehH0OPzW1CpOeu42US2qMRWnsmDJRnopUDiOnHyuNa5PqRlzncKJl1x3/
ci2JRQ2SiVdjvQTp0OGPkAXMLAri7R3kdrIIgZ+gnytWJ91fCeO016Jg1OxeV3Rr
5fOUx6Fk3+uS0e36+D8SRgF5mRBdP2o6g2Hzio+2+zScwonvIehVo/WeeTXRIZob
0AWhvs0Jf6JkQaVLYkR0p3LuRR2PhBsfVccr3pgt10kZG2hYKr4YCAXGsuhlp77r
CasFGOTMv/f4AnzTpQSCvaR34omrZYbvVgHlP1X1VRHnUxUQfgqmDjuaHtAq8Kql
miQ6cNkLW1AybZx43+rRJnKWsuvviXTgSTek8ccMOcPeB07RkS4bpRuepBNYW5mW
Vz9uqMotRXqSpZaaggHZB2fIJ5uah/LPcSbwfdiqVNYcaVexbSeYdZm+tzHPO3Ie
EydiWAYRDJ8LxG4t8hyjNGkUnk+aGa5o2XzGdKQLhY02wXWQKLFAKMez2280NK8T
S8A1AoR04h2elZ85MyGh+bRs4zZwQGm0zKSHkkNa+uoaJLZfmPcNy3X1pMXyHK+b
dDGyJaf2q/kwKA6w/UA9
=2S4r
-----END PGP SIGNATURE-----