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

Make ImageProcessor-s public #621

Closed
antonfirsov opened this issue Jun 17, 2018 · 12 comments
Closed

Make ImageProcessor-s public #621

antonfirsov opened this issue Jun 17, 2018 · 12 comments
Labels
Milestone

Comments

@antonfirsov
Copy link
Member

Currently all processors are internal. We agreed that we should open up their API for 1.0.0-RC1, after an extensive review.

@antonfirsov antonfirsov added this to the 1.0.0-rc1 milestone Jun 17, 2018
@DanStout
Copy link

DanStout commented Jul 3, 2018

Yes, please! I wanted to split the Sobel edge detection processor into separate X and Y processors, but found that all the required classes and their dependencies are all marked internal.

@JimBobSquarePants
Copy link
Member

Yeah we’ve been keeping them internal to work on the API. Is there something missing from the current edge detection processors? We could work together to fix that?

@DanStout
Copy link

DanStout commented Jul 3, 2018

The one in particular I was looking for was to be able to use the Sobel processor on X and Y independently. My plan was at first just to create two subclasses of EdgeDetectorProcessor - SobelXProcessor and SobelYProcessor

@JimBobSquarePants
Copy link
Member

Perhaps we should make that operation an Enum?

@DanStout
Copy link

DanStout commented Jul 4, 2018

As in with values X, Y, and XY? That would make sense, I think.

@JimBobSquarePants
Copy link
Member

I'll have to have a look to see if we can use separableness without breaking anything. Feel free to have a dig around yourself.

@vpenades
Copy link
Contributor

vpenades commented Sep 3, 2018

I would consider opening also the pixel blender API, at least the method that lets you process a single pixel.

Maybe its not even required to expose the whole PixelBlender class.... Personally, I would consider adding a method that returns a delegate, something like this:

delegate TPixel PixelFunction<TPixel>(TPixel backdrop, TPixel source, float opacity);

PixelFunction<TPixel>  <somewhere>.GetPixelBlendingFunction(PixelAlphaComposition composition, PixelColorBlending blending);

Where <somewhere> would be TPixel itself.

this would allow something like this:

var pixelFunc = Rgba32.GetPixelBlendingFunction(SrcAtop,Multiply);

@antonfirsov
Copy link
Member Author

@vpenades I'm strongly against per-pixel delegates, because it's a performance-killer.

@vpenades
Copy link
Contributor

vpenades commented Sep 3, 2018

@antonfirsov I know, I am aware of that, and I guess you want to avoid exposing them to prevent end users implementing linear operations using naive loops.

but there's some scenarios in which they can be useful, specially for random pixel access.

Let's say I want to render a star field, drawing random pixels ?

There's lots of procedural algorythms that require non linear access to the pixels, and they cannot benefit from the pixel blenders.

The only alternative I could find around this is to add a .Mutate( c->DrawPointList(...) ); which could be parallelized.... but most probably users would call it using one pixel at a time. But i'm sure it would still be much faster than drawing 1 pixel sized rectangles which is what I'm using right now.

One of my pet project was to port some of these examples to .Net with ImageSharp. All these examples use random pixel access with blending.

@antonfirsov
Copy link
Member Author

antonfirsov commented May 10, 2019

I opened this issue, but now I'm confused about our goal. Currently it is reasonable to define 3 levels of "API opennes":

  1. Non-generic IImageProcessor classes are public. We have a clear plan for this in in Epic: introduce pixel-agnostic API across the whole library #907.
  2. All utilities classes needed to implement image processors by 3rd parties are public. (eg. Buffer2D<T>) Nice goal, but no free lunch here: those classes need an extensive review, and as soon as we touch Enable images of arbitrary size #898, we may break everything.
  3. Making current generic image processor implementations (ImageProcessor<T>-s) public. I think we should avoid this, even in long term.

Which level do we want to reach for RC1 / 1.0?

@JimBobSquarePants
Copy link
Member

I’d go with 1 and maybe a sprinkle of 2 if we need it. With 1 we can call multiple processors from within a new one which allows building complex new ones. Anything else can be negotiated as something to add to the source.

@JimBobSquarePants
Copy link
Member

I'm closing this since steps 1 and 2 have been completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants