Re: [PATCH 07/12] d3drm: Implement IDirect3DRMViewport*::Init. (v4)
-----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-----
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
-----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-----
participants (2)
-
Aaryaman Vasishta -
Stefan Dösinger