[PATCH v2 0/1] MR289: d3drm: Implement IDirect3DRMFrame2::SetSceneBackgroundImage
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> -- v2: d3drm: Implement IDirect3DRMFrame{1/2/3}::SetSceneBackgroundImage https://gitlab.winehq.org/wine/wine/-/merge_requests/289
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/d3drm/d3drm_private.h | 1 + dlls/d3drm/frame.c | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/dlls/d3drm/d3drm_private.h b/dlls/d3drm/d3drm_private.h index 2fb6bafe951..31fdd4984a1 100644 --- a/dlls/d3drm/d3drm_private.h +++ b/dlls/d3drm/d3drm_private.h @@ -81,6 +81,7 @@ struct d3drm_frame IDirect3DRMFrame3 IDirect3DRMFrame3_iface; IDirect3DRM *d3drm; LONG ref; + IUnknown *backgroundimage; struct d3drm_frame *parent; SIZE_T nb_children; SIZE_T children_size; diff --git a/dlls/d3drm/frame.c b/dlls/d3drm/frame.c index 5cc3ad585d4..9365e02cc47 100644 --- a/dlls/d3drm/frame.c +++ b/dlls/d3drm/frame.c @@ -608,6 +608,8 @@ static ULONG WINAPI d3drm_frame3_Release(IDirect3DRMFrame3 *iface) if (!refcount) { + if (frame->backgroundimage) + IUnknown_Release(frame->backgroundimage); d3drm_object_cleanup((IDirect3DRMObject *)&frame->IDirect3DRMFrame_iface, &frame->obj); for (i = 0; i < frame->nb_children; ++i) { @@ -2222,25 +2224,47 @@ static HRESULT WINAPI d3drm_frame1_SetSceneBackgroundDepth(IDirect3DRMFrame *ifa static HRESULT WINAPI d3drm_frame3_SetSceneBackgroundImage(IDirect3DRMFrame3 *iface, IDirect3DRMTexture3 *texture) { - FIXME("iface %p, texture %p stub!\n", iface, texture); + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame3(iface); + IUnknown *unk = NULL; - return E_NOTIMPL; + TRACE("iface %p, texture %p\n", iface, texture); + + if (texture) + IDirect3DRMTexture3_QueryInterface(texture, &IID_IUnknown, (void**)&unk); + + if (frame->backgroundimage) + IUnknown_Release(frame->backgroundimage); + + frame->backgroundimage = unk; + + return S_OK; } static HRESULT WINAPI d3drm_frame2_SetSceneBackgroundImage(IDirect3DRMFrame2 *iface, IDirect3DRMTexture *texture) { - FIXME("iface %p, texture %p stub!\n", iface, texture); + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame2(iface); + IUnknown *unk = NULL; - return E_NOTIMPL; + TRACE("iface %p, texture %p\n", iface, texture); + + if (texture) + IDirect3DRMTexture_QueryInterface(texture, &IID_IUnknown, (void**)&unk); + + if (frame->backgroundimage) + IUnknown_Release(frame->backgroundimage); + + frame->backgroundimage = unk; + + return S_OK; } static HRESULT WINAPI d3drm_frame1_SetSceneBackgroundImage(IDirect3DRMFrame *iface, IDirect3DRMTexture *texture) { - FIXME("iface %p, texture %p stub!\n", iface, texture); - - return E_NOTIMPL; + struct d3drm_frame *frame = impl_from_IDirect3DRMFrame(iface); + TRACE("iface %p, texture %p\n", iface, texture); + return d3drm_frame2_SetSceneBackgroundImage(&frame->IDirect3DRMFrame2_iface, texture); } static HRESULT WINAPI d3drm_frame3_SetSceneFogEnable(IDirect3DRMFrame3 *iface, BOOL enable) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/289
On 6/21/22 06:53, Alistair Leslie-Hughes wrote:
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/d3drm/d3drm_private.h | 1 + dlls/d3drm/frame.c | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 7 deletions(-)
In general I'm concerned about these commits adding state tracking code that's never actually used. Most notably, it means we can't test anything but the state tracking, and while there's not *likely* to be anything wrong with the state tracking as it is, it's not impossible either. It's also going to make it harder to implement the real workhorse methods. Once you finally get to the point where you need to implement IDirect3DRMViewport::Render() [and, although it hasn't been stated, I'm guessing you're dealing with a d3drm application that is eventually going to call it], you'll have a whole lot of state that you'll suddenly need to implement, or else basically have methods that silently return success without doing anything useful, which is not good for debugging. In this case it's even worse because "backgroundimage" is never read, so it's completely dead code.
On 22/6/22 04:37, Zebediah Figura wrote:
On 6/21/22 06:53, Alistair Leslie-Hughes wrote:
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/d3drm/d3drm_private.h | 1 + dlls/d3drm/frame.c | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 7 deletions(-)
In general I'm concerned about these commits adding state tracking code that's never actually used. Most notably, it means we can't test anything but the state tracking, and while there's not *likely* to be anything wrong with the state tracking as it is, it's not impossible either.
It's also going to make it harder to implement the real workhorse methods. Once you finally get to the point where you need to implement IDirect3DRMViewport::Render() [and, although it hasn't been stated, I'm guessing you're dealing with a d3drm application that is eventually going to call it], you'll have a whole lot of state that you'll suddenly need to implement, or else basically have methods that silently return success without doing anything useful, which is not good for debugging. Yes, IDirect3DRMViewport::Render will be the have to be implemented at some point and I understand the point your making.
For reference: Star Wars Rebellion Currently this just crashes when starting a battle. If I just fixed the crash (have a patch for it already), then it displays a dialog for every function that doesn't return success. There is about 9 more patches to stop all the scenarios, and in my opinion the crash is just as bad as 9+ MessageBox's every render cycle. My thought was to implement the infrastructure as much as possible first, then take on Render. Maybe starting to implement Render next might be good idea. Regards Alistair.
On 6/21/22 17:12, Alistair Leslie-Hughes wrote:
On 22/6/22 04:37, Zebediah Figura wrote:
On 6/21/22 06:53, Alistair Leslie-Hughes wrote:
From: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/d3drm/d3drm_private.h | 1 + dlls/d3drm/frame.c | 38 +++++++++++++++++++++++++++++++------- 2 files changed, 32 insertions(+), 7 deletions(-)
In general I'm concerned about these commits adding state tracking code that's never actually used. Most notably, it means we can't test anything but the state tracking, and while there's not *likely* to be anything wrong with the state tracking as it is, it's not impossible either.
It's also going to make it harder to implement the real workhorse methods. Once you finally get to the point where you need to implement IDirect3DRMViewport::Render() [and, although it hasn't been stated, I'm guessing you're dealing with a d3drm application that is eventually going to call it], you'll have a whole lot of state that you'll suddenly need to implement, or else basically have methods that silently return success without doing anything useful, which is not good for debugging. Yes, IDirect3DRMViewport::Render will be the have to be implemented at some point and I understand the point your making.
For reference: Star Wars Rebellion
Currently this just crashes when starting a battle. If I just fixed the crash (have a patch for it already), then it displays a dialog for every function that doesn't return success. There is about 9 more patches to stop all the scenarios, and in my opinion the crash is just as bad as 9+ MessageBox's every render cycle.
My thought was to implement the infrastructure as much as possible first, then take on Render. Maybe starting to implement Render next might be good idea.
Sure, that kind of iteration is probably fine for local debugging, i.e. stub things out until you have a good idea of what all is going to be necessary. For upstream I think it's probably better to get to a state where we can properly render things, and then start implementing other states. I recognize that implementing the entire workhorse parts of d3drm is probably a much more daunting task than filling out the state tracking. Accordingly please don't hesitate to reach out for assistance :-)
This merge request was approved by Stefan Dösinger. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/289
participants (5)
-
Alistair Leslie-Hughes -
Alistair Leslie-Hughes -
Alistair Leslie-Hughes (＠alesliehughes)
-
Stefan Dösinger
-
Zebediah Figura