Appreciate UIImage.scale when decompressing images

[pyrtsa]

Change targetSize to use the same scale as UIImage.size. Also use round (to nearest) to convert scaled dimensions to integer rather than truncating the fractional part.

Since sizes in iOS are expressed in logical points, not pixels, it is more natural to treat targetSize as logical points, so that decompressing an image of image.size == CGSize(width: 400, height: 300) and image.scale == 2.0 down to targetSize == CGSize(width: 200, height: 150) would actually mean the same as decompressImage(image, 0.5) – rather than decompressImage(image, 0.25) which it did before this patch.

[kean]

Hey @pyrtsa . I think I understand why you would want to express targetSize in points, however I'm not entirely sure about this change (which is a major one btw).

There are at least two ways to treat targetSize as points, and they produce different results:

1. targetSize is premultiplied by screen scale (which makes sense, you want your images to have the same pixel size as your views)
2. targetSize is multiplied by image scale (the way it works in your pull request)

Example:

image1: size 400x200, scale 2
image2: size 800x400, scale 1

resizing image to targetSize 200x100 for screen with scale 2: 

1. points X screen scale
image1 -> size 200x100, scale 2
image2 -> size 400x200, scale 1

2. points X image scale
image1 -> size 200x100, scale 2
image2 -> size 200x100, scale 1

It seems less ambiguous to express targetSize in pixels.

And PHImageManager also does the same thing - all the sizes including targetSize in requestImageForAsset(_:targetSize:contentMode:options:resultHandler:) method are in pixels.

[pyrtsa]

That sounds fair enough. I think using a screen scale becomes problematic in the subtle scenario when there is an external screen attached (often with a scale different from UIScreen.mainScreen().scale).

How about doing the following change instead?

1. Add a property named scale to ImageRequest, with the default value of 1.
2. Pass the scale on to ImageDecompressor, and return a UIImage with the given scale (and size).
3. Keep rounding to nearest pixel like in my original patch, because otherwise what gets drawn in decompressImage may crop out a bit of the last pixel row and column.

I'll modify the pull request accordingly.

[kean]

It's late, I can't think anymore ;)

1. I'm not sure what is the best way to handle image scale; Please take a look at ImageDecoder class before making changes; What problem are you solving? Is there a scenario in which you would want to set the image scale manually?
2. keep targetSize in pixels?
3. Seems alright.

[pyrtsa]

Sorry for the delay. This issue turned bikesheddy, which I'm sorry about and I don't like.

Here's another attempt at making it better anyway. I find it unlikely to break existing code but it's a breaking change anyway: now there's a new parameter targetScale (defaults 1.0) that directly sets the target UIImage.scale. This combination, however, makes targetSize behave the way as UIImage.size behaves, at the presence of non-unit targetScale and UIImage.scale.

What do you think?

[kean]

It's ok, we're not in a hurry, thanks that you are investing your time :) It's not really a bikeshedding, the framework is already complete, and even though that change is small, it's still a breaking one.

I understand how you would want targetSize to work. But I'm still not convinced why it should work that way. The default implementation is really simple - targetSize is in pixels, image scale it set to the display scale. The main goal in Nuke is not to cover every scenario imaginable but to make things simple and customizable.

If you want to treat targetSize as points (either one of the scenarios that I described in my first comment) you can implement a custom ImageDecompressor and return it in the factory method in ImageLoaderDelegate. Then you can pass targetScale in a userInfo property of ImageRequest class. That's just one way to do it. And there are other ways to use targetSize too. Hell, you can even use it to modify NSURLRequest if your server has a method to scale images on the fly (and ignore it in image decompressor).

About scale. Again, I'm very reluctant about adding targetScale to the ImageRequest. I tried but I couldn't imagine a scenario in which I would want to set image scale manually when you I make a request. However, I've thought of a scenario in which you would ask your server for an image and it would return either @1x or @2x (or else) image based on some internal logic. Then you would want to set image scale according to the metadata provided by the server. The problem with Nuke in this scenario is that ImageDecoder doesn't have access to anything except for image data (NSData) and that might be a problem if image scale is not embedded in the image data (EXIF or something else).

[pyrtsa]

Yes, this is exactly the bikeshedding I'm talking about – in other words, I don't want to take this discussion further.

You're absolutely right that adding targetScale complicates things. The only unexpected thing in the existing behaviour is that targetSize actually means pixels while UIImage.size is in points.

I'm closing this pull request. I suggest the following fixes could be considered in separate issues, sometime:

1. Fixing the downscaled size computation like in this PR. As said, minor rendering errors due to bad rounding may happen otherwise.
2. Documenting better that targetSize means actual pixels (and reminding about Retina scale).
3. Possibly weighing whether targetSize should be renamed (e.g. targetResolution, maximumResolution, maximumPixelCount, whatever), although just better doc comments and better examples might be enough.

[kean]

There is also an option of removing targetSize altogether :)

[kean]

Thanks for the contribution anyway, I haven't put much thought in image scale before.

[kean]

1. Yes
2. Yes, I would also point out that "in default implementation target size means pixels", and explain how and when it's actually used. I'll work on this today.
3. Nuke lacks documentation in some places

[kean]

docs for targetSize and contentMode 1346119

[pyrtsa]

👍 to all!

[kean]

hey @pyrtsa, could you please create a new pull request that would fix rendering errors due to the lack or rounding? Please branch of the develop branch and create pull request to the develop branch.