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

Wrong image scaling in SwiftUI (layout size is calculated in pixels) #746

Closed
Kinark opened this issue Jan 10, 2024 · 5 comments · Fixed by #777
Closed

Wrong image scaling in SwiftUI (layout size is calculated in pixels) #746

Kinark opened this issue Jan 10, 2024 · 5 comments · Fixed by #777
Labels

Comments

@Kinark
Copy link

Kinark commented Jan 10, 2024

As of now LazyImage renders remote images with the scale of 1 instead of using the scale of the screen. From what I could tell, there's no way of showing the image with the proper scaling without using .resizable() modifier which degrades performance as you and a few other devs talked about here. I've tried:

  • Using thumbnail options in userInfo
  • Setting the screen scale manually in userInfo
  • Using a Resize processor

...but nothing works except if .resizable() is present.

AsyncImage has an optional scale parameter that fixes that, but there's no similar alternative in Nuke.

Take this image as a sample, which has a size of 96x96. In my iPhone 14 Pro, which has a screen scale of 3, it should show as a rectangle with the size of 32x pts, but instead it renders as a 96x pts image. The same happens with AsyncImage if scale param is not set.

Here's a code sample which tests all the alternatives:

let req = ImageRequest(url: URL(string: "https://i.imgur.com/k8gZbJ4.png"), userInfo: [.thumbnailKey: ImageRequest.ThumbnailOptions(size: .init(width: 32, height: 32), unit: .points, contentMode: .aspectFill), .scaleKey: 3])
VStack {
  LazyImage(request: req)
  LazyImage(request: req)
    .processors([.resize(width: 32)])
  LazyImage(url:  URL(string: "https://i.imgur.com/k8gZbJ4.png")!)
    .processors([.resize(width: 32)])
  LazyImage(url:  URL(string: "https://i.imgur.com/k8gZbJ4.png")!)
  AsyncImage(url: URL(string: "https://i.imgur.com/k8gZbJ4.png")!, scale: 3)
  AsyncImage(url: URL(string: "https://i.imgur.com/k8gZbJ4.png")!)
}

This is the the result:
Screenshot 2024-01-09 at 23 45 51

Is there anything I'm missing or is the expected behavior?

@kean
Copy link
Owner

kean commented Jan 10, 2024

I debugged the following scenario:

let request = ImageRequest(url: URL(string: "https://i.imgur.com/k8gZbJ4.png"), userInfo: [..scaleKey: 3])
LazyImage(request: request)

The image displayed by the LazyImage appears to have the correct size and scale.

(lldb) po imageContainer?.image
▿ Optional

  • some : <UIImage:0x60000302c120 anonymous {32, 32} renderingMode=automatic(original)>
    (lldb) po imageContainer?.image.scale
    ▿ Optional
  • some : 3.0

When I run it in a simulator, the image has the correct size:

Screenshot 2024-01-09 at 10 06 37 PM

@kean
Copy link
Owner

kean commented Jan 10, 2024

I think the issue it due to coalescing and/or caching of the requests. The framework tries to minimize the amount of work and reuse the existing cached images and download and processing tasks. The issue is that the .scaleKey isn't used in the cache or coalescing keys. I don't think it was ever designed for a scenario where someone would request the same image with different scales.

I haven't fully confirmed this hypothesis as it's a bit tricky to test, but it seems to be the case.

Setting the screen scale manually in userInfo

Yes, that's the intended way to use it, and is a replacement for the scale parameter in AsyncImage. Unlike AsyncImage, Nuke provides a ton of different options, so, of course, it made sense to extend the existing options rather than adding an ad-hoc parameter to the LazyImage type itself.

@Kinark
Copy link
Author

Kinark commented Feb 18, 2024

I debugged the following scenario:

let request = ImageRequest(url: URL(string: "https://i.imgur.com/k8gZbJ4.png"), userInfo: [..scaleKey: 3])
LazyImage(request: request)

The image displayed by the LazyImage appears to have the correct size and scale.

(lldb) po imageContainer?.image ▿ Optional

  • some : <UIImage:0x60000302c120 anonymous {32, 32} renderingMode=automatic(original)>
    (lldb) po imageContainer?.image.scale
    ▿ Optional
  • some : 3.0

When I run it in a simulator, the image has the correct size:

Screenshot 2024-01-09 at 10 06 37 PM

Sorry for the late response!

So I guess the problem happens when we need to rescale the image with either thumbnailKey or resize preprocessor. Like, the image I provided has a size of 96x96px, which displays nicely in your example in a case where we need the image to be 32x32pt (in a scenario of aspect ratio of 3), but what if my image has a size of 256x256px and I need to display a thumbnail of it in the size of 32x32pts? How can I preprocess the image but still keep the correct aspect ratio? Like, I'd need to preprocess it to be in the size of 96x96px theoretically, right?

Shouldn't let req = ImageRequest(url: URL(string: "https://site.com./a-256x-image.png"), userInfo: [.thumbnailKey: ImageRequest.ThumbnailOptions(size: .init(width: 32, height: 32), unit: .points, contentMode: .aspectFill), .scaleKey: 3]) be enough for my LazyImage to show the image properly preprocessed and in the correct size of 32pts? I mean, shouldn't it handle storing the correct versions for showing a 32x pts image without any .resizable()?

I guess my examples were failing due to the fact that I was always try rescaling it at some point and what it should do by receiving an instruction of preprocessing it for making it 32x pts large (imo) is to, in my 96x96px pig image example, do nothing with the image and store it as it is (since it's in the correct dimensions for displaying with an aspect ratio of 3). Then, when showing it, show it with an aspect ratio of 3.

But I feel I'm probably making a hell of a mess out of something lmfao

@kean kean added the bug label Feb 18, 2024
@kean
Copy link
Owner

kean commented Feb 18, 2024

My suggestions would be to either make sure you use the same scale every time OR disable coalescing using pipeline's configuration.

I'm also going to mark it as a defect because the scale parameter should never break caching or coalescing.

There may also be a need for a different API or implementation for setting the scale. Maybe it should be something similar to the AsyncImage API. It should probably be safe and quick to fetch the image and then change the scale right before the display every time the image is displayed.

@kean kean mentioned this issue Apr 20, 2024
@kean kean closed this as completed in #777 Apr 21, 2024
@kean
Copy link
Owner

kean commented Apr 24, 2024

The coalescing issue were fixed in Nuke 12.6.

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 a pull request may close this issue.

2 participants