-
-
Notifications
You must be signed in to change notification settings - Fork 888
Expose pixelblenders. #967 #1028
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1028 +/- ##
==========================================
- Coverage 89.87% 89.87% -0.01%
==========================================
Files 1103 1103
Lines 48897 48896 -1
Branches 3441 3441
==========================================
- Hits 43945 43944 -1
Misses 4249 4249
Partials 703 703
Continue to review full report at Codecov.
|
antonfirsov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can't get input or do the investigation by our own, I suggest to leave PorterDuffFunctions internal for now.
Currently the only critical API surfaces are PixelBlender<T> and PixelOperations<T>.GetPixelBlender(). (The latter is not yet exposed.)
src/ImageSharp/PixelFormats/PixelBlenders/PorterDuffFunctions.Generated.tt
Outdated
Show resolved
Hide resolved
| /// At amount = 0, "from" is returned, at amount = 1, "to" is returned. | ||
| /// At amount = 0, "background" is returned, at amount = 1, "source" is returned. | ||
| /// </param> | ||
| public void Blend<TPixelSrc>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method has no direct tests. It shouldn't be public yet probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably ok. We've got some heavy implicit testing via the calling Blend method. I've reordered the class slightly to make the call flow more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TPixel != TPixelSrc remains untested, but in this case the compiler should guarantee correctness in theory. ReSharper's MemberCanBePrivate inspections make me always suspicious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Resharper is wrong there. We use the very same pattern with our CloneAs<TPixel2> APIs
antonfirsov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overloads for PixelOperations<T>.GetPixelBlender() are still internal.
There are several questions around those API-s that shall be answered before opening them up:
- Is
PixelOperations<T>indeed the best place to hold them? In other words: is it an SRP violation we accept and want to live with? - Do we need the
virtualkeyword? There is no POC for overriding default implementations, which is a must-have when defining extensible API-s in my opinion. Adding thevirtualkeyword later is not a breaking change
|
@antonfirsov Will review your comments tomorrow, it's late now. Still finding my way around this part of the API so I'll probably miss a few things. Keen to hear more about this also.
|
It means that the scope of the
In my opinion these are different concerns. Not sure if it is a good idea to mix them. Publishing API-s is hard ... |
|
@antonfirsov I see your concern but am not sure how we could split the two areas of functionality. Currently I'm very tempted to leave the structure as-is. |
@antonfirsov The answer to the same question from Eric Lippart suggests otherwise. I'd rather not introduce the risk should we ever create optimized versions of the blenders. |
|
@JimBobSquarePants you're right on virtual. I've also found a rule and an explanation in the official BCL coding guidelines. Regarding |
|
Yay! One step closer to splitting out drawing! |
Prerequisites
Description
Exposes the PixelBlender API to the public.