[PATCH v3] d2d1: Make ID2D1Device::CreateImageBrush() accept only bitmap as a source image.
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Signed-off-by: Dmitry Timoshkov dmitry@baikal.ru --- dlls/d2d1/brush.c | 27 ++++++++++++++++++++++++++- dlls/d2d1/d2d1_private.h | 1 - 2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/dlls/d2d1/brush.c b/dlls/d2d1/brush.c index b9a673fe54c..99aaedeb43f 100644 --- a/dlls/d2d1/brush.c +++ b/dlls/d2d1/brush.c @@ -1209,7 +1209,19 @@ static void STDMETHODCALLTYPE d2d_image_brush_SetImage(ID2D1ImageBrush *iface, I TRACE("iface %p, image %p.\n", iface, image);
if (image) + { + ID2D1Bitmap *bitmap; + + if (FAILED(ID2D1Image_QueryInterface(image, &IID_ID2D1Bitmap, (void **)&bitmap))) + { + FIXME("ID2D1Image doesn't support ID2D1Bitmap interface.\n"); + return; + } + + ID2D1Bitmap_Release(bitmap); ID2D1Image_AddRef(image); + } + if (brush->u.image.image) ID2D1Image_Release(brush->u.image.image); brush->u.image.image = image; @@ -1327,7 +1339,20 @@ HRESULT d2d_image_brush_create(ID2D1Factory *factory, ID2D1Image *image, if (!(*brush = heap_alloc_zero(sizeof(**brush)))) return E_OUTOFMEMORY;
- d2d_brush_init(*brush, factory, D2D_BRUSH_TYPE_IMAGE, + if (image) + { + ID2D1Bitmap *bitmap; + + if (FAILED(ID2D1Image_QueryInterface(image, &IID_ID2D1Bitmap, (void **)&bitmap))) + { + FIXME("ID2D1Image doesn't support ID2D1Bitmap interface.\n"); + return E_FAIL; + } + + ID2D1Bitmap_Release(bitmap); + } + + d2d_brush_init(*brush, factory, D2D_BRUSH_TYPE_BITMAP, brush_desc, (ID2D1BrushVtbl *)&d2d_image_brush_vtbl); if (((*brush)->u.image.image = image)) ID2D1Image_AddRef((*brush)->u.image.image); diff --git a/dlls/d2d1/d2d1_private.h b/dlls/d2d1/d2d1_private.h index 29469f5142e..6183d9d4c99 100644 --- a/dlls/d2d1/d2d1_private.h +++ b/dlls/d2d1/d2d1_private.h @@ -42,7 +42,6 @@ enum d2d_brush_type D2D_BRUSH_TYPE_LINEAR, D2D_BRUSH_TYPE_RADIAL, D2D_BRUSH_TYPE_BITMAP, - D2D_BRUSH_TYPE_IMAGE, D2D_BRUSH_TYPE_COUNT, };
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
On 5/26/22 17:15, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. That means that ID2D1Device::CreateImageBrush() can't be really passed anything but a ID2D1Bitmap* represented as a ID2D1Image*.
I've added QueryInterface+FIXME just in case, probably it could be dropped.
v2: Fix test crashes with image == NULL. v3: Add a QueryInterface() check to SetImage().
Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:15, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote:
Dmitry Timoshkov dmitry@baikal.ru wrote:
> ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. > That means that ID2D1Device::CreateImageBrush() can't be really passed anything > but a ID2D1Bitmap* represented as a ID2D1Image*. > > I've added QueryInterface+FIXME just in case, probably it could be dropped. > > v2: Fix test crashes with image == NULL. > v3: Add a QueryInterface() check to SetImage(). Is there anything that could be improved in this patch to make it acceptable? Probably I should add once again, that this patch allows a bitmap work as an image brush (unlike current broken state) and actually makes work the app that have here.
Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case has fixed painting in my application. Do you think that the patch I attached in my previous reply with this fix is an acceptable solution, or there are other things you'd like to see addressed?
On 5/26/22 17:36, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:15, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/25/22 18:58, Dmitry Timoshkov wrote: > Dmitry Timoshkov dmitry@baikal.ru wrote: > >> ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. >> That means that ID2D1Device::CreateImageBrush() can't be really passed anything >> but a ID2D1Bitmap* represented as a ID2D1Image*. >> >> I've added QueryInterface+FIXME just in case, probably it could be dropped. >> >> v2: Fix test crashes with image == NULL. >> v3: Add a QueryInterface() check to SetImage(). > Is there anything that could be improved in this patch to make it acceptable? > Probably I should add once again, that this patch allows a bitmap work as an > image brush (unlike current broken state) and actually makes work the app that > have here. > Same thing I mention last time - it's backwards. Bitmap brush could probably be implemented as image brush, but not the other way. I don't think you'll need any shader changes for that.
Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case has fixed painting in my application. Do you think that the patch I attached in my previous reply with this fix is an acceptable solution, or there are other things you'd like to see addressed?
I think we should avoid duplication of bind_bitmap() function, since it's exactly the same. Probably passing image brush description there, which could be filled from bitmap brush description, expect for source rectangle.
By the way, in application you're testing, how does it set this source rectangle?
Nikolay Sivov nsivov@codeweavers.com wrote:
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case has fixed painting in my application. Do you think that the patch I attached in my previous reply with this fix is an acceptable solution, or there are other things you'd like to see addressed?
I think we should avoid duplication of bind_bitmap() function, since it's exactly the same. Probably passing image brush description there, which could be filled from bitmap brush description, expect for source rectangle.
Thanks.
By the way, in application you're testing, how does it set this source rectangle?
I haven't looked into particular details about this in the logs. Probably I could try to figure that out if that makes a difference and/or is really important for the implementation.
On 5/26/22 20:40, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
By the way, in application you're testing, how does it set this source rectangle?
I haven't looked into particular details about this in the logs. Probably I could try to figure that out if that makes a difference and/or is really important for the implementation.
Having source rectangle seems to be the only difference from using regular bitmap brush. So it's possible application is actually using. This can wait of course, but also might not be hard to fix.
Nikolay Sivov nsivov@codeweavers.com wrote:
By the way, in application you're testing, how does it set this source rectangle?
I haven't looked into particular details about this in the logs. Probably I could try to figure that out if that makes a difference and/or is really important for the implementation.
Having source rectangle seems to be the only difference from using regular bitmap brush. So it's possible application is actually using. This can wait of course, but also might not be hard to fix.
The application doesn't seem to fail to render its UI with current implementation (which completely ignores sourceRectangle passed with D2D1_IMAGE_BRUSH_PROPERTIES).
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:36, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:15, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
> On 5/25/22 18:58, Dmitry Timoshkov wrote: >> Dmitry Timoshkov dmitry@baikal.ru wrote: >> >>> ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. >>> That means that ID2D1Device::CreateImageBrush() can't be really passed anything >>> but a ID2D1Bitmap* represented as a ID2D1Image*. >>> >>> I've added QueryInterface+FIXME just in case, probably it could be dropped. >>> >>> v2: Fix test crashes with image == NULL. >>> v3: Add a QueryInterface() check to SetImage(). >> Is there anything that could be improved in this patch to make it acceptable? >> Probably I should add once again, that this patch allows a bitmap work as an >> image brush (unlike current broken state) and actually makes work the app that >> have here. >> > Same thing I mention last time - it's backwards. Bitmap brush could > probably be implemented as image brush, but not the other way. I don't > think you'll need any shader changes for that. Do you mean something like the attached patch, or do I miss something? With the attached patch (on top of just sent ::Map()/::Unmap() patches) I get areas filled with black in the application that I'm working on, unlike with the proposed patch where I get correctly filled areas with a proper brush.
I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case has fixed painting in my application. Do you think that the patch I attached in my previous reply with this fix is an acceptable solution, or there are other things you'd like to see addressed?
I think we should avoid duplication of bind_bitmap() function, since it's exactly the same. Probably passing image brush description there, which could be filled from bitmap brush description, expect for source rectangle.
Thanks for the helpful pointers. Does the attached patch match your suggestions? The patch works prety well with the application that I have here.
On 6/3/22 15:40, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:36, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 17:15, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
On 5/26/22 14:34, Dmitry Timoshkov wrote: > Nikolay Sivov nsivov@codeweavers.com wrote: > >> On 5/25/22 18:58, Dmitry Timoshkov wrote: >>> Dmitry Timoshkov dmitry@baikal.ru wrote: >>> >>>> ID2D1Bitmap derives from ID2D1Image, which in turn derives from ID2D1Resource. >>>> That means that ID2D1Device::CreateImageBrush() can't be really passed anything >>>> but a ID2D1Bitmap* represented as a ID2D1Image*. >>>> >>>> I've added QueryInterface+FIXME just in case, probably it could be dropped. >>>> >>>> v2: Fix test crashes with image == NULL. >>>> v3: Add a QueryInterface() check to SetImage(). >>> Is there anything that could be improved in this patch to make it acceptable? >>> Probably I should add once again, that this patch allows a bitmap work as an >>> image brush (unlike current broken state) and actually makes work the app that >>> have here. >>> >> Same thing I mention last time - it's backwards. Bitmap brush could >> probably be implemented as image brush, but not the other way. I don't >> think you'll need any shader changes for that. > Do you mean something like the attached patch, or do I miss something? > With the attached patch (on top of just sent ::Map()/::Unmap() patches) > I get areas filled with black in the application that I'm working on, > unlike with the proposed patch where I get correctly filled areas with > a proper brush. > I suspect black color is from this in sample_brush():
return float4(0.0, 0.0, 0.0, brush.opacity);
Which means you didn't set brush type correctly in the constant buffer.
I also think that's the source of the problem. However that's the shader code, and you mentioned above that I don't need them to make image brush work. Or did I misunderstand something? Do you have a suggestion for that?
Brush type is set in d2d_brush_fill_cb(), trying setting it "correctly" to bitmap brush type.
Thank you very much, setting cb->type to TYPE_BITMAP in the TYPE_IMAGE case has fixed painting in my application. Do you think that the patch I attached in my previous reply with this fix is an acceptable solution, or there are other things you'd like to see addressed?
I think we should avoid duplication of bind_bitmap() function, since it's exactly the same. Probably passing image brush description there, which could be filled from bitmap brush description, expect for source rectangle.
Thanks for the helpful pointers. Does the attached patch match your suggestions? The patch works prety well with the application that I have here.
Yes, along those lines.
+static void d2d_brush_bind_bitmap(struct d2d_brush *brush, struct d2d_device_context *context, + unsigned int brush_idx) +{ + D2D1_IMAGE_BRUSH_PROPERTIES image_brush_desc;
+ image_brush_desc.sourceRectangle.left = 0.0f; + image_brush_desc.sourceRectangle.top = 0.0f; + image_brush_desc.sourceRectangle.right = brush->u.bitmap.bitmap->pixel_size.width; + image_brush_desc.sourceRectangle.bottom = brush->u.bitmap.bitmap->pixel_size.height;
This needs a test, so we don't assume coordinate system here. We have tests for filling with 4x4 bitmap brush, it will be a matter of creating image brush with same bitmap, and rectangle as (0,0-4,4) vs (0,0-1,1). If that shows no difference it means this is using normalized coordinates, and we should initialize it here as (0,0-1,1).
@@ -1043,7 +1043,7 @@ static void STDMETHODCALLTYPE d2d_device_context_FillGeometry(ID2D1DeviceContext if (FAILED(context->error.code)) return;
- if (opacity_brush && brush_impl->type != D2D_BRUSH_TYPE_BITMAP) + if (opacity_brush && !(brush_impl->type == D2D_BRUSH_TYPE_BITMAP || brush_impl->type == D2D_BRUSH_TYPE_IMAGE)) { d2d_device_context_set_error(context, D2DERR_INCOMPATIBLE_BRUSH_TYPES); return;
Same here, we have a test for this case already, that needs to be extended to verify this change.
Nikolay Sivov nsivov@codeweavers.com wrote:
+static void d2d_brush_bind_bitmap(struct d2d_brush *brush, struct d2d_device_context *context, + unsigned int brush_idx) +{ + D2D1_IMAGE_BRUSH_PROPERTIES image_brush_desc;
+ image_brush_desc.sourceRectangle.left = 0.0f; + image_brush_desc.sourceRectangle.top = 0.0f; + image_brush_desc.sourceRectangle.right = brush->u.bitmap.bitmap->pixel_size.width; + image_brush_desc.sourceRectangle.bottom = brush->u.bitmap.bitmap->pixel_size.height;
This needs a test, so we don't assume coordinate system here. We have tests for filling with 4x4 bitmap brush, it will be a matter of creating image brush with same bitmap, and rectangle as (0,0-4,4) vs (0,0-1,1). If that shows no difference it means this is using normalized coordinates, and we should initialize it here as (0,0-1,1).
I've added the test in the attached version of the patch, is that what you had in mind? The test shows that (0,0-4,4) vs (0,0-1,1) source rectangles lead to different painting results under Windows. However, since source rectangle is completely ignored in current image brush implementation that doesn't really change anything, so I'm not sure what kind of result this test is supposed to have for the proposed patch.
@@ -1043,7 +1043,7 @@ static void STDMETHODCALLTYPE d2d_device_context_FillGeometry(ID2D1DeviceContext if (FAILED(context->error.code)) return;
- if (opacity_brush && brush_impl->type != D2D_BRUSH_TYPE_BITMAP) + if (opacity_brush && !(brush_impl->type == D2D_BRUSH_TYPE_BITMAP || brush_impl->type == D2D_BRUSH_TYPE_IMAGE)) { d2d_device_context_set_error(context, D2DERR_INCOMPATIBLE_BRUSH_TYPES); return;
Same here, we have a test for this case already, that needs to be extended to verify this change.
I guess you mean the tests in test_opacity_brush()? Anyway, it looks like omitting this part of the patch changes nothing in my application, moreover MSDN states that when the opacity brush is specified in FillGeometry() brush must be an ID2D1BitmapBrush. So, this part is clearly wrong, and if desired should be sent as a separate change.
Thanks again for the helpful comments.
On 6/7/22 12:12, Dmitry Timoshkov wrote:
Nikolay Sivov nsivov@codeweavers.com wrote:
+static void d2d_brush_bind_bitmap(struct d2d_brush *brush, struct d2d_device_context *context, + unsigned int brush_idx) +{ + D2D1_IMAGE_BRUSH_PROPERTIES image_brush_desc;
+ image_brush_desc.sourceRectangle.left = 0.0f; + image_brush_desc.sourceRectangle.top = 0.0f; + image_brush_desc.sourceRectangle.right = brush->u.bitmap.bitmap->pixel_size.width; + image_brush_desc.sourceRectangle.bottom = brush->u.bitmap.bitmap->pixel_size.height;
This needs a test, so we don't assume coordinate system here. We have tests for filling with 4x4 bitmap brush, it will be a matter of creating image brush with same bitmap, and rectangle as (0,0-4,4) vs (0,0-1,1). If that shows no difference it means this is using normalized coordinates, and we should initialize it here as (0,0-1,1).
I've added the test in the attached version of the patch, is that what you had in mind? The test shows that (0,0-4,4) vs (0,0-1,1) source rectangles lead to different painting results under Windows. However, since source rectangle is completely ignored in current image brush implementation that doesn't really change anything, so I'm not sure what kind of result this test is supposed to have for the proposed patch.
It shows if we should be using normalized rectangle for regular bitmap brushes, or rectangle in pixel coordinates, like you did. It looks like pixel coordinates are used, after I tried some visual tests.
@@ -1043,7 +1043,7 @@ static void STDMETHODCALLTYPE d2d_device_context_FillGeometry(ID2D1DeviceContext if (FAILED(context->error.code)) return;
- if (opacity_brush && brush_impl->type != D2D_BRUSH_TYPE_BITMAP) + if (opacity_brush && !(brush_impl->type == D2D_BRUSH_TYPE_BITMAP || brush_impl->type == D2D_BRUSH_TYPE_IMAGE)) { d2d_device_context_set_error(context, D2DERR_INCOMPATIBLE_BRUSH_TYPES); return;
Same here, we have a test for this case already, that needs to be extended to verify this change.
I guess you mean the tests in test_opacity_brush()? Anyway, it looks like omitting this part of the patch changes nothing in my application, moreover MSDN states that when the opacity brush is specified in FillGeometry() brush must be an ID2D1BitmapBrush. So, this part is clearly wrong, and if desired should be sent as a separate change.
Yes, I think it's fine to leave this for a separate change. I'd expect bitmap brush and (image brush with a bitmap) be treated the same way here. Documentation could as well be outdated, because image brush was introduced with later versions, device contexts were not used in initially released d2d API.
Thanks again for the helpful comments.
-
Dmitry Timoshkov -
Nikolay Sivov