Re: [PATCH 1/5] d2d1: Implement d2d_d3d_render_target_CreateSolidColorBrush().
Hi Henri, On 06/16/14 13:13, Henri Verbeet wrote:
diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index 5609db6..9e91728 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -33,4 +33,13 @@ struct d2d_d3d_render_target void d2d_d3d_render_target_init(struct d2d_d3d_render_target *render_target, ID2D1Factory *factory, IDXGISurface *surface, const D2D1_RENDER_TARGET_PROPERTIES *desc) DECLSPEC_HIDDEN;
+struct d2d_brush +{ + ID2D1Brush ID2D1Brush_iface;
Given how you use this object, this should be ID2D1SolidColorBrush. I guess you want to somehow reuse this struct for other brush types, but there should be better ways for this. Then: +void d2d_solid_color_brush_init(struct d2d_brush *brush, ID2D1RenderTarget *render_target, + const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc) +{ + FIXME("Ignoring brush properties.\n"); + + brush->ID2D1Brush_iface.lpVtbl = (ID2D1BrushVtbl *)&d2d_solid_color_brush_vtbl; + brush->refcount = 1; +} This cast should not be needed.
+ LONG refcount; +}; + +void d2d_solid_color_brush_init(struct d2d_brush *brush, ID2D1RenderTarget *render_target, + const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc) DECLSPEC_HIDDEN; + #endif /* __WINE_D2D1_PRIVATE_H */ diff --git a/dlls/d2d1/render_target.c b/dlls/d2d1/render_target.c index be9c1b6..e8c56a0 100644 --- a/dlls/d2d1/render_target.c +++ b/dlls/d2d1/render_target.c @@ -117,9 +117,19 @@ static HRESULT STDMETHODCALLTYPE d2d_d3d_render_target_CreateBitmapBrush(ID2D1Re static HRESULT STDMETHODCALLTYPE d2d_d3d_render_target_CreateSolidColorBrush(ID2D1RenderTarget *iface, const D2D1_COLOR_F *color, const D2D1_BRUSH_PROPERTIES *desc, ID2D1SolidColorBrush **brush) { - FIXME("iface %p, color %p, desc %p, brush %p stub!\n", iface, color, desc, brush); + struct d2d_brush *object;
- return E_NOTIMPL; + TRACE("iface %p, color %p, desc %p, brush %p.\n", iface, color, desc, brush); + + if (!(object = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*object)))) + return E_OUTOFMEMORY; + + d2d_solid_color_brush_init(object, iface, color, desc); + + TRACE("Created brush %p.\n", object); + *brush = (ID2D1SolidColorBrush *)&object->ID2D1Brush_iface;
Same here, the cast should not be needed. Also allocating memory in brush.c seems more logical to me (although that's a matter of taste). Cheers, Jacek
On 16 June 2014 13:44, Jacek Caban <jacek(a)codeweavers.com> wrote:
Given how you use this object, this should be ID2D1SolidColorBrush. No, struct d2d_brush is also used for e.g. 105100.
On 06/16/14 13:53, Henri Verbeet wrote:
On 16 June 2014 13:44, Jacek Caban <jacek(a)codeweavers.com> wrote:
Given how you use this object, this should be ID2D1SolidColorBrush. No, struct d2d_brush is also used for e.g. 105100.
Yup, and that patch has the same problem with parent to child interface casts. Jacek
On 06/16/14 13:59, Henri Verbeet wrote:
On 16 June 2014 13:57, Jacek Caban <jacek(a)codeweavers.com> wrote:
Yup, and that patch has the same problem with parent to child interface casts.
What are you suggesting instead?
I don't know this how will this be used in the future, so my ideas may not be best fit. For the code that you sent so far, a separated struct for all brush types would do the job. I guess that different brush types will store different data in the future, so separated structs will be needed anyway. If having some sort of shared struct is desired, this could be introduced as separated struct: struct d2d1_brash { /* shared stuff, maybe even refcount */ }; struct d2d1_solid_brush { ID2D1SolidColorBrush ID2D1SolidColorBrush_iface LONG refcount; struct brush base_brush; /* Stuff specific to solid brush */ }; This has some drawbacks like the fact that d2d1_brush can't be easily casted to ID2D1Brush. This may be avoided by design or, with some runtime overhead, d2d1_brush could store a pointer to ID2D1Brush (which would be initialized to solid_brush->ID2D1SoludBrush_iface in this case) or have some internal vtbl-like casting functions. Jacek
On 16 June 2014 14:15, Jacek Caban <jacek(a)codeweavers.com> wrote:
I don't know this how will this be used in the future, so my ideas may not be best fit. For the code that you sent so far, a separated struct As far as the API is concerned, brushes are mostly used for drawing commands like e.g. ID2D1RenderTarget::FillRectangle() and the like. These all take ID2D1Brush pointers. The way the implementation is likely to work in the future is that brush types will have an associated fragment shader, and some constant buffer data. I.e., a typical drawing function implementation would do something along the lines of the following:
struct d2d_brush *b = unsafe_impl_from_ID2DBrush(brush); ... ID3D10Device_PSSetShader(device, b->shader); ID3D10Device_PSSetConstantBuffers(device, 0, 1, &b->cb); ... ID3D10Device_DrawIndexed(device, ...);
for all brush types would do the job. I guess that different brush types will store different data in the future, so separated structs will be needed anyway. If having some sort of shared struct is desired, this could be introduced as separated struct:
struct d2d1_brash { /* shared stuff, maybe even refcount */ };
struct d2d1_solid_brush { ID2D1SolidColorBrush ID2D1SolidColorBrush_iface LONG refcount; struct brush base_brush; /* Stuff specific to solid brush */ };
This has some drawbacks like the fact that d2d1_brush can't be easily casted to ID2D1Brush. This may be avoided by design or, with some It's mostly the ID2D1Brush -> struct d2d_brush conversion that's the issue. If you do the above, you either need to do multiple QueryInterface() calls in each drawing function, or introduce a private interface to get at the embedded d2d_brush structure, and then implement that for every single brush. I think the two casts, while not very pretty, are worth it in comparison.
runtime overhead, d2d1_brush could store a pointer to ID2D1Brush (which would be initialized to solid_brush->ID2D1SoludBrush_iface in this case) That would only help for getting an ID2D1Brush from struct d2d_brush, but even then, you'd just reintroduce the casts you were trying to avoid.
On 06/16/14 14:39, Henri Verbeet wrote:
On 16 June 2014 14:15, Jacek Caban <jacek(a)codeweavers.com> wrote:
I don't know this how will this be used in the future, so my ideas may not be best fit. For the code that you sent so far, a separated struct As far as the API is concerned, brushes are mostly used for drawing commands like e.g. ID2D1RenderTarget::FillRectangle() and the like. These all take ID2D1Brush pointers. The way the implementation is likely to work in the future is that brush types will have an associated fragment shader, and some constant buffer data. I.e., a typical drawing function implementation would do something along the lines of the following:
struct d2d_brush *b = unsafe_impl_from_ID2DBrush(brush);
Since you have many different vtbls for ID2DBrush implementation, you can't use the usual vtbl trick for checking if passed pointer is really our implementation. One way this can be solved is to abuse QueryInterface, so that you could call: ID2DBrush_QueryInterface(brush, &IID_brush_struct, (void**)&brush_struct); QueryInterface could just return the struct directly for that special IID. And yeah, returning struct instead of an interface is not pretty, but that's probably fine here. You could even skip AddRef in this case.
... ID3D10Device_PSSetShader(device, b->shader); ID3D10Device_PSSetConstantBuffers(device, 0, 1, &b->cb); ... ID3D10Device_DrawIndexed(device, ...);
I see, this could really use common code.
for all brush types would do the job. I guess that different brush types will store different data in the future, so separated structs will be needed anyway. If having some sort of shared struct is desired, this could be introduced as separated struct:
struct d2d1_brash { /* shared stuff, maybe even refcount */ };
struct d2d1_solid_brush { ID2D1SolidColorBrush ID2D1SolidColorBrush_iface LONG refcount; struct brush base_brush; /* Stuff specific to solid brush */ };
This has some drawbacks like the fact that d2d1_brush can't be easily casted to ID2D1Brush. This may be avoided by design or, with some It's mostly the ID2D1Brush -> struct d2d_brush conversion that's the issue. If you do the above, you either need to do multiple QueryInterface() calls in each drawing function, or introduce a private interface to get at the embedded d2d_brush structure, and then implement that for every single brush.
The QI trick above makes it one QI call and no extra interface.
I think the two casts, while not very pretty, are worth it in comparison.
This, again, depends on how this will look when the implementation is more complete. Such casts are usually a sign of a design problem, but the final decision is up to you. *If* such casts will be only in brush creation code, that's not too bad. You could make them even more local (eg. by replacing d2d_solid_color_brush_init by a function that returns ID2D1SolidBrush interface pointer, so that caller doesn't need to do any more cast and the problem does not spread across the code).
runtime overhead, d2d1_brush could store a pointer to ID2D1Brush (which would be initialized to solid_brush->ID2D1SoludBrush_iface in this case) That would only help for getting an ID2D1Brush from struct d2d_brush, but even then, you'd just reintroduce the casts you were trying to avoid.
Not really. If you have random d2d_brush instance, you can't cast it anyway. If it's not random, then you probably should be able to use specialized structs like d2d1_solid_brush, which could be casted safely. Jacek
On 16 June 2014 16:50, Jacek Caban <jacek(a)codeweavers.com> wrote:
struct d2d_brush *b = unsafe_impl_from_ID2DBrush(brush);
Since you have many different vtbls for ID2DBrush implementation, you can't use the usual vtbl trick for checking if passed pointer is really our implementation. One way this can be solved is to abuse Other places that do this kind of thing just check against multiple vtbls.
QueryInterface, so that you could call:
ID2DBrush_QueryInterface(brush, &IID_brush_struct, (void**)&brush_struct);
QueryInterface could just return the struct directly for that special IID. And yeah, returning struct instead of an interface is not pretty, but that's probably fine here. You could even skip AddRef in this case.
That works, although personally I'd say it's a little more complex than just having the casts, and not really any prettier. I don't care too strongly though, so as far as I'm concerned it comes down to whatever Alexandre prefers.
... ID3D10Device_PSSetShader(device, b->shader); ID3D10Device_PSSetConstantBuffers(device, 0, 1, &b->cb); ... ID3D10Device_DrawIndexed(device, ...);
I see, this could really use common code.
Sure. Eventually we'll probably have something like "d2d_brush_apply(brush, device);", and we'll probably want to queue up and merge draw operations until ID2D1RenderTarget::EndDraw(), but that's the basic idea.
I think the two casts, while not very pretty, are worth it in comparison.
This, again, depends on how this will look when the implementation is more complete. Such casts are usually a sign of a design problem, but the final decision is up to you. *If* such casts will be only in brush creation code, that's not too bad. Yeah, it should only be needed in the creation / initialization code, everything else should just operate on struct d2d_brush.
You could make them even more local (eg. by replacing d2d_solid_color_brush_init by a function that returns ID2D1SolidBrush interface pointer, so that caller doesn't need to do any more cast and the problem does not spread across the code).
Yeah, perhaps. Effectively that just means moving most of the body of d2d_d3d_render_target_CreateSolidColorBrush() to brush.c, and it's another one of those things that I'm not really opposed to, but don't really think are worth it either.
participants (2)
-
Henri Verbeet -
Jacek Caban