Composite filters

[alamofire]

Composite filters

[damienrambout]

About compound filter identifiers, we could inner filter identifiers separated with _.
Of course, the order of inner filters is very important to produce the same compound identifier.

[cnoon]

Honestly I think this is a GREAT idea! Do you want to put a PR together with this functionality or would you like me to do it? If you do end up putting it together...I have a few suggestions:

1. Let's create a CompositeImageFilter protocol to enforce the filters property and the ImageFilter conformance.
2. Would be slick to use a protocol extension to create the identifier. Definitely needs to be joined by either a - or _ in the correct order that the filters are added.
3. We should rework all the Multi-Pass image filters to be CompositeImageFilters instead.
4. Let's definitely gets tests to cover this addition.

Like I said, if you want to roll this yourself that would be awesome. Otherwise I'll implement this probably next week.

Thanks! 🍻

[damienrambout]

Cool! Yes, I'll try to implement it this week-end and PR by next sunday.

About you suggestions:

1. Of course, always protocol first. CompositeImageFilter will only add a var filters: [ImageFilter] { get } property.
2. Agreed, composite filters should only be worried about providing the filters in the correct order. The protocol extension will provide a default implementation for identifier and filter.
3. Yes, I think multi-pass filters will be much lighter (less code) this way :-)
4. I'll do my best for tests coverage, based on the existing tests.

When you'll get the PR, feel free to accommodate the code to your standards. 👌 👍

[damienrambout]

Several remarks:

1. When converting filters such as AspectScaledToFillSizeWithRoundedCornersFilter, the image comparison test fails because of the radius not being scaled. In the current version of AspectScaledToFillSizeWithRoundedCornersFilter, I see the radius is multiplied by the input image scale. Shouldn't this be the responsibility of RoundedCornersFilter which would automatically apply the scale based on the input image? I don't understand why multi-pass filters apply a scale when using af_imageWithRoundedCornerRadius and RoundedCornersFilter does not.
2. Should current multi-pass filters still conform to Sizable and Roundable? Especially, AspectScaledToFillSizeCircleFilter and ScaledToSizeCircleFilter should not be considered Roundable anymore. It was interesting to get an automatic identifier based on size and corner radius, but now the identifier will be made of composite filter identifiers.
3. Multi-pass filters converted to CompositeImageFilter will not get the same name as before. Instead of ScaledToSizeWithRoundedCornersFilter-size:(200x100)-radius:(20), we'll get ScaledToSizeFilter-size:(200x100)_RoundedCornersFilter-radius:(20). Is this ok for you?

[cnoon]

Hi @damienrambout,

1. The images should look exactly the same between all screen scales. If you don't apply adjust the radius by the scale factor, then the rounded corner is much different on all screen scales. This is exactly why I created Multi-Pass filters the way I did. You'll need to keep this functionality. If it ends up feeling like a hack to add a flag to the RoundedCornersFilter for making all rounded corners the same across all screen scales, then I'd say the CompositeImageFilter protocol may not be a good idea after all.
2. Nope.
3. Fine by me.

[damienrambout]

Hi @cnoon,

Of course all images should look the same between screen scales. This is why I wanted to apply the image scale in the RoundedCornersFilter, which I believe is the correct way to do it. If I do that, multi-pass filter tests will pass (since it will be exactly like you implemented these filters in the first place), but then testThatRoundedCornersFilterReturnsCorrectFilteredImage will not pass on a screen-scale > 1.
In this test, the expected image has no scale specified (pirate-radius-20.png) but it is still loaded using the current screen scale. Shouldn't this kind of images (no scale specified) always be considered like @1x, like ones downloaded from the internet?
If pirate-radius-20.png is loaded using UIImage(named:), we get a an image with a size of (350.0, 210.0) and a scale of 1.0.

[cnoon]

I'm working on this issue now @damienrambout. There's definitely a solution. Let's redirect all future comments to #8. I'm going to close this issue out.