Skip to content

Conversation

@tteichmeister
Copy link
Contributor

Hi,

It would be helpful to provide additional drawing functions for the image.
Especially to fill a rectangle or a circle. Currently it is only possible to draw a rectangle/circle but not to fill it.

Furthermore, an additional function to draw a text with a background color would be good.
Especially if you want to label objects and other stuff, with a background color as a contrast, it can help to improve the readability of the text.

(Here I marked the object with DrawRectangle and using the new DrawText method to draw the text below)
image

Therefore I provided 3 methods:

  • FillRectangle
  • FillCircle
  • DrawText (including an additional color for the background color)

I also including the operators which are also included for the existing DrawText, DrawRectangle...

DrawText including background color
FillRectangle and FillCircle
@ghost
Copy link

ghost commented May 1, 2021

CLA assistant check
All CLA requirements met.

revert accidental removing lines from Resources.resx
@chitsaw chitsaw self-assigned this May 3, 2021
Copy link
Contributor

@chitsaw chitsaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting this PR - just a couple of tiny changes but otherwise everything looks great! Thanks also for including unit tests :-). For some reason there is one test (Image_GrayDrawTestWithBackground) that fails the CI build due to too many different pixels vs the reference image. I wonder if that test passes for you at all? We've sometimes noticed in the past that some of these imaging tests can fail on some machines but not others, possibly due to differences in GDI+ versions. There are tolerance and outlier parameters in the AssertAreImagesEqual method for that reason, but normally the default tolerances have worked for us.

remove temporary debug lines
new image for TestImage_GrayDrawTextWithBackground
@tteichmeister
Copy link
Contributor Author

Thank you for submitting this PR - just a couple of tiny changes but otherwise everything looks great! Thanks also for including unit tests :-). For some reason there is one test (Image_GrayDrawTestWithBackground) that fails the CI build due to too many different pixels vs the reference image. I wonder if that test passes for you at all? We've sometimes noticed in the past that some of these imaging tests can fail on some machines but not others, possibly due to differences in GDI+ versions. There are tolerance and outlier parameters in the AssertAreImagesEqual method for that reason, but normally the default tolerances have worked for us.

@chitsaw
It's strange, because before I submitted it, the Test was working on my machine. When I was doing it now again, this test didn't passed also on my machine. So I provided a new verification image for this test. I think I uploaded the wrong one. Since the checks passed now :-)

Thank you for your review and to let me contribute this to your project :-).

@chitsaw chitsaw merged commit a0c174e into microsoft:master May 4, 2021
@chitsaw
Copy link
Contributor

chitsaw commented May 4, 2021

@tteichmeister thanks for making those changes. Yes, the tests pass now with the new image. Thanks so much for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants