Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split PixelBlendMode into PixelColorBlendingMode and PixelAlphaCompositionMode #679

Merged
merged 13 commits into from Aug 23, 2018

Conversation

vpenades
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

This continues the process started with #535

As described in the issue, GraphicsOptions now has two separated modes for configuring Color Blending and Alpha Composition.

PixelAlphaCompositionMode has "SrcOver" as first value to ensure that a default GraphicsOptions structure will always be SrcOver-Normal which is the mode everybody expects by default.

Now there's a lot more possible combinations, so creating tests for all of them might take a while. Specially considering the test images are in a separate repository.

Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

LGTM. Happy to merge if @tocsoft doesn't spot anything funky.

return false;
}

if (this.options.ColorBlendingMode != PixelColorBlendingMode.Normal)
Copy link
Member

Choose a reason for hiding this comment

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

Much more readable! 👍

@codecov
Copy link

codecov bot commented Aug 21, 2018

Codecov Report

Merging #679 into master will increase coverage by 3.95%.
The diff coverage is 93.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   85.29%   89.24%   +3.95%     
==========================================
  Files         878      878              
  Lines       39092    39209     +117     
  Branches     2538     2557      +19     
==========================================
+ Hits        33342    34994    +1652     
+ Misses       5055     3489    -1566     
- Partials      695      726      +31
Impacted Files Coverage Δ
...ng/Processors/Overlays/BackgroundColorProcessor.cs 100% <100%> (ø) ⬆️
...rocessing/Processors/Overlays/VignetteProcessor.cs 100% <100%> (ø) ⬆️
...Sharp.Tests/Drawing/SolidFillBlendedShapesTests.cs 100% <100%> (ø) ⬆️
...Formats/PixelBlenders/PorterDuffCompositorTests.cs 100% <100%> (ø) ⬆️
...Tests/PixelFormats/PixelOperationsTests.Blender.cs 100% <100%> (ø) ⬆️
...geSharp.Tests/TestUtilities/TestImageExtensions.cs 82.37% <100%> (ø) ⬆️
...rp/Processing/Processors/Overlays/GlowProcessor.cs 93.1% <100%> (ø) ⬆️
...ageSharp.Drawing/Processing/TextGraphicsOptions.cs 100% <100%> (ø) ⬆️
...c/ImageSharp.Drawing/Processing/BrushApplicator.cs 86.95% <100%> (ø) ⬆️
...geSharp.Tests/PixelFormats/PixelOperationsTests.cs 95.37% <100%> (+0.08%) ⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 093b9ba...4083b90. Read the comment docs.

@@ -32,7 +32,7 @@ public struct TextGraphicsOptions

private float? dpiY;

private PixelBlenderMode blenderMode;
private PixelColorBlendingMode blenderMode;
Copy link
Member

Choose a reason for hiding this comment

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

we should add the alpha blending option in here too.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tocsoft I'll take a look

@vpenades
Copy link
Contributor Author

vpenades commented Aug 21, 2018

@JimBobSquarePants @tocsoft Some of the coverage tests are failing because the DrawImageProcessor methods I added are not being called from anywere because there were some missing methods in DrawImageExtensions.

And looking at DrawImageExtensions I've noticed the arguments ordering is a bit of a mess....

Some methods have opacity before blending, and others the opposite way.

Should I rearrange the arguments so location is always first and opacity is always the last?

  • float opacity
  • PixelColorBlendingMode color, float opacity
  • PixelColorBlendingMode color,PixelAlphaCompositionMode alpha, float opacity
  • Point location, float opacity
  • Point location, PixelColorBlendingMode color, float opacity
  • Point location, PixelColorBlendingMode color,PixelAlphaCompositionMode alpha, float opacity

@tocsoft
Copy link
Member

tocsoft commented Aug 21, 2018

the re-arranged arguments look good to me.

@JimBobSquarePants
Copy link
Member

Nice coverage!

@JimBobSquarePants JimBobSquarePants merged commit 52db9db into SixLabors:master Aug 23, 2018
antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019
Split PixelBlendMode into PixelColorBlendingMode and PixelAlphaCompositionMode
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.

None yet

3 participants