Skip to content

Conversation

@JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Oct 16, 2019

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

Exposes the PixelBlender API to the public.

@JimBobSquarePants JimBobSquarePants changed the title WIP: Expose pixelblenders. Touch #967 WIP: Expose pixelblenders. #967 Oct 16, 2019
@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #1028 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...xelFormats/PixelOperations{TPixel}.PixelBenders.cs 100% <ø> (ø) ⬆️
...ts/PixelBlenders/DefaultPixelBlenders.Generated.cs 77.77% <ø> (ø) ⬆️
...ats/PixelBlenders/PorterDuffFunctions.Generated.cs 38.02% <ø> (ø) ⬆️
.../PixelFormats/PixelBlenders/PorterDuffFunctions.cs 100% <100%> (ø) ⬆️
src/ImageSharp/Color/Color.cs 100% <100%> (ø) ⬆️
...rc/ImageSharp/PixelFormats/PixelBlender{TPixel}.cs 100% <100%> (ø) ⬆️

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 e484a40...e6d7af0. Read the comment docs.

Copy link
Member

@antonfirsov antonfirsov left a 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.)

/// 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>(
Copy link
Member

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.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Oct 22, 2019

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.

Copy link
Member

@antonfirsov antonfirsov Oct 22, 2019

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.

Copy link
Member Author

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

@JimBobSquarePants JimBobSquarePants changed the title WIP: Expose pixelblenders. #967 Expose pixelblenders. #967 Oct 22, 2019
Copy link
Member

@antonfirsov antonfirsov left a 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 virtual keyword? There is no POC for overriding default implementations, which is a must-have when defining extensible API-s in my opinion. Adding the virtual keyword later is not a breaking change

@JimBobSquarePants JimBobSquarePants marked this pull request as ready for review October 22, 2019 13:18
@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Oct 22, 2019

@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.

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?

@antonfirsov
Copy link
Member

antonfirsov commented Oct 22, 2019

Is PixelOperations indeed the best place to hold them? In other words: is it an SRP violation we accept and want to live with?

It means that the scope of the PixelOperations<TPixel> class has not yet been explicitly clarified. Stuff just "happens" to be here.

  • Is it a class where we only want to expose methods executing pixel operations (eg. conversions) only?
  • Or is it a facade for different API-s do stuff specific to a known TPixel? (Run operations, query pixel blenders, query other stuff in future use-cases.)

In my opinion these are different concerns. Not sure if it is a good idea to mix them.

Publishing API-s is hard ...

@JimBobSquarePants
Copy link
Member Author

@antonfirsov I see your concern but am not sure how we could split the two areas of functionality.

Currently PixelOperations is initialized via the IPixel.CreatePixelOperations() method which allows us to specify pixel specific optimizations. If we split pixel blending functionality we lose the ability to create TPixel optimal versions of those blenders unless we created a duplicate initialization method in IPixel.

I'm very tempted to leave the structure as-is.

@JimBobSquarePants
Copy link
Member Author

Do we need the virtual keyword? There is no POC for overriding default implementations, which is a must-have when defining extensible API-s in my opinion. Adding the virtual keyword later is not a breaking change

@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.

@antonfirsov
Copy link
Member

@JimBobSquarePants you're right on virtual. I've also found a rule and an explanation in the official BCL coding guidelines.

Regarding PixelOperations<TPixel>:
Generally, I tend to agree with you. If our decision is explicit, the public API is under control.

@JimBobSquarePants
Copy link
Member Author

Yay! One step closer to splitting out drawing!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants