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

Keep ZoomState and position if loading Images in same screen. #104

Open
jizhe7550 opened this issue Oct 14, 2024 · 22 comments
Open

Keep ZoomState and position if loading Images in same screen. #104

jizhe7550 opened this issue Oct 14, 2024 · 22 comments

Comments

@jizhe7550
Copy link

jizhe7550 commented Oct 14, 2024

Hello my situation that I have ImagePreview pager and I will show thumbnail first then Preview then fullSizeImage (3 different paths), which require 3 downloading process in same screen. I am wondering whether it is possible to support

  1. Once thumbnail loading success, then set it as placeholder. Then the screen won't see flashing 3 time dude to the 3 paths changes.
  2. Is it possible to keep the zoomState, then this can keep the position in same page after zooming?

Now my code like this

Box(
        modifier = modifier,
        contentAlignment = Alignment.Center,
    ) {
       // fullSizePath will change from thumbnailPath -> previewPath -> fullSizePath
        var imagePath by remember(fullSizePath) {
            mutableStateOf(fullSizePath)
        }

        val request = ImageRequest.Builder(LocalContext.current)
            .data(imagePath)
            .listener(
                onError = { _, _ ->
                    imagePath = errorImagePath
                }
            )
            .crossfade(300)
            .build()

        ZoomableAsyncImage(
            model = request,
            state = imageState,
            gesturesEnabled = enableZoom && !isMagnifierMode,
            contentDescription = "Image Preview",
            modifier = Modifier.fillMaxSize(),
            onClick = {
                onImageTap()
            },
            onDoubleClick = DoubleClickToZoomListener.cycle(maxZoomFactor = 3f),
        )
    }

I know it may be not easy, so could you please guide if I can custom this logic in somewhere? Thank you very much.

flash.and.position.move.mp4
@saket
Copy link
Owner

saket commented Oct 14, 2024

Is the overall goal here to retain the pan and zoom state across image changes? If so, I'd definitely like to support it but it has proven to be non-trivial in my past attempts.

For your thumbnails, have you tried using coil's placeholder for displaying them? Telephoto will smoothly swap out placeholders when their full quality versions are ready and even block zoom events until then. https://saket.github.io/telephoto/zoomableimage/#placeholders

For smoothly transitioning from a preview image to its full-sized image, is the image resolution known before hand for the full-sized variant?

@jizhe7550
Copy link
Author

Hi saket, thanks for you reply.

Is the overall goal here to retain the pan and zoom state across image changes? If so, I'd definitely like to support it but it has proven to be non-trivial in my past attempts.

In my use case, it is essential to provide good UX. We don't want to reset the current zoom state as soon the fullSize downloaded. If you see my video, each quality transition: thumbnail -> preview -> fullSize, the central position is changing. I guess it is caused by the different size of picture (thumbnail/preview/fullsize). But I have no idea to handle it.

For your thumbnails, have you tried using coil's placeholder for displaying them? Telephoto will smoothly swap out placeholders when their full quality versions are ready and even block zoom events until then.

Great to know!
But does the placeholder support static resource only? In our case, the thumbnail is dynamic which downloaded from some API first.

For smoothly transitioning from a preview image to its full-sized image, is the image resolution known before hand for the full-sized variant?

Unfortunately, we don't know.

Again, thank you so much.

@ErickSumargo
Copy link

Yeah it'd be great @saket if Telephoto can support it.
I also got some reviews from my users they sometimes feel annoyed the zoom is reset when higher quality arrives.

@saket
Copy link
Owner

saket commented Oct 16, 2024

But does the placeholder support static resource only? In our case, the thumbnail is dynamic which downloaded from some API first.

Gotcha. Yea I wish Coil had support for dynamic thumbnails.

Unfortunately, we don't know.

Noted. In theory it should be possible to retain the zoom and offset values if the new image shares the same aspect ratio as the old one. I'll try working on this soon.

@jizhe7550
Copy link
Author

Thank you very much.

@jizhe7550
Copy link
Author

Hi Saket, any progress on this?

@saket
Copy link
Owner

saket commented Nov 18, 2024

I unfortunately haven't been able to find time to work on this yet. I made some progress in the last release by adapting zoom & pan values to a new viewport size:

/**
* Used when [ZoomableState]'s saved gesture state cannot be restored due to viewport size changes.
* Adjusts zoom and pan values to maintain the content's centroid position in the new viewport.
*/
internal class GestureStateAdjuster(
private val oldFinalZoom: ScaleFactor,
private val oldContentOffsetAtViewportCenter: Offset, // Present in the content's coordinate space.
) {
fun adjustForNewViewportSize(

I think we can write something similar for adapting to new content sizes. Wanna help me out?

@jizhe7550
Copy link
Author

Hello, my job has changed and I am unable to support the work. I will notify my colleagues for follow-up. Thank you very much.

@mega-ht
Copy link

mega-ht commented Nov 28, 2024

Hi @saket,
I was playing with it a bit and noticed something: panning by the value obtained by subtracting the offset of the current high-res image from previous image seems to restore the zoom position. This basically sets the exact offset of old image to new image (replaces the recalculated offset with previous image's offset). Just sharing with you in case it sparks any ideas. I'll look into the implementation further.

zoomableState.panBy(offset = previousImageOffset - currentOffSet)

Here's the sample code

Screen record:

Restore.offset.mp4

@saket
Copy link
Owner

saket commented Nov 28, 2024

Good to know. We can copy over that logic to RealZoomableState somewhere when a change in the content size is detected.

@mega-ht
Copy link

mega-ht commented Nov 29, 2024

I applied the logic based on aspect ratio, and it actually worked nicely! The transition to hi-res image is also extremely smooth (3-4th second of the video). Here's the commit.

However, there’s one existing issue that needs to be addressed. After the new high-resolution image is loaded, when you pan on it(drag by finger/apply gesture), it resets the offset, causing a jumping effect (6th second of the video). I was wondering if the previousOffset or userOffset for new contentSize could be handled inside GestureState instead of contentTransformation. Any thoughts?

Screen_Recording_20241129_160751_Telephoto.mp4

Based on the logs, another approach could be by updating UserOffset based on new image's scale factor compared to old image's UserOffset.

// previous image
12:39:02.987 I ==================================================
12:39:02.987 I gestureStateInputs.baseZoom: BaseZoomFactor(value=ScaleFactor(1.4, 1.4))
12:39:02.987 I gestureState.userZoom: UserZoomFactor(value=1.4814814)
12:39:02.987 I contentZoom.finalZoom(): ScaleFactor(2.0, 2.0)
12:39:02.987 I contentOffset.baseOffset: Offset(0.0, -222.9)
12:39:02.987 I contentOffset.userOffset: UserOffset(value=Offset(130.0, 267.2))
12:39:02.987 I contentOffset.finalOffset(): Offset(130.0, 44.3)
12:39:02.988 I contentSize: Size(800.0, 1199.0)
12:39:02.988 I offset: Offset(-260.0, -88.5)

// new image = current implementation which keeps the old user offset
13:07:08.955  I  gestureStateInputs.baseZoom: BaseZoomFactor(value=ScaleFactor(0.2, 0.2))
13:07:08.955  I  gestureState.userZoom: UserZoomFactor(value=1.4814814)
13:07:08.955  I  contentZoom.finalZoom(): ScaleFactor(0.3, 0.3)
13:07:08.955  I  contentOffset.baseOffset: Offset(0.0, -1293.1)
13:07:08.955  I  contentOffset.userOffset: UserOffset(value=Offset(130.0, 267.2))
13:07:08.955  I  contentOffset.finalOffset(): Offset(130.0, -1025.9)
13:07:08.955  I  contentSize: Size(4640.0, 6952.0)
13:07:08.955  I  offset: Offset(-44.8, 353.8)

// new image = after keeping previousOffset (shared commit)
12:41:33.805 I gestureStateInputs.baseZoom: BaseZoomFactor(value=ScaleFactor(0.2, 0.2))
12:41:33.805 I gestureState.userZoom: UserZoomFactor(value=1.4814814)
12:41:33.805 I contentZoom.finalZoom(): ScaleFactor(0.3, 0.3)
12:41:33.805 I contentOffset.baseOffset: Offset(0.0, -1293.1)
12:41:33.805 I contentOffset.userOffset: UserOffset(value=Offset(130.0, 267.2))
12:41:33.805 I contentOffset.finalOffset(): Offset(130.0, -1025.9)
12:41:33.806 I contentSize: Size(4640.0, 6952.0)
12:41:33.806 I offset: Offset(-260.0, -88.5)

// new image after using panBy = this should be the expected correct values where gesture would work after hi-res image is loaded
12:39:06.020 I gestureStateInputs.baseZoom: BaseZoomFactor(value=ScaleFactor(0.2, 0.2))
12:39:06.020 I gestureState.userZoom: UserZoomFactor(value=1.4814814)
12:39:06.020 I contentZoom.finalZoom(): ScaleFactor(0.3, 0.3)
12:39:06.020 I contentOffset.baseOffset: Offset(0.0, -1293.1)
12:39:06.020 I contentOffset.userOffset: UserOffset(value=Offset(754.0, 1549.8))
12:39:06.020 I contentOffset.finalOffset(): Offset(754.0, 256.7)
12:39:06.020 I contentSize: Size(4640.0, 6952.0)
12:39:06.021 I offset: Offset(-260.0, -88.5)

By the way, I'm really enjoying working on it. You've done an amazing job with this library ❤️

@mega-ht
Copy link

mega-ht commented Nov 30, 2024

Hello @saket,
Good news! It seems that recalculating the UserOffset is the better/correct approach. This also resolves the pan position reset issue when gesture is applied (dragged) after image is loaded, as shown in the last video.

Currently, when a new high-resolution image is loaded, the RealZoomableState continues using the UserOffset from the old low-resolution image, which causes a jump due to invalid final offset. One of the ways to solve it:

  • Store the last GestureState and content/image size.
  • Whenever a new image is loaded, recalculate the UserOffset based on the scale factor between the new and old image sizes, and update gestureState

Here's the code changes

I'm pretty sure you might have a better way to handle this, specially considering other edge cases you’re aware of. This is just the basic idea.

Screen_Recording_20241130_210418_Telephoto.mp4

@hm-tamim
Copy link

hm-tamim commented Dec 3, 2024

Hi @saket , have you got the chance to look at it?

@saket
Copy link
Owner

saket commented Dec 4, 2024

Sorry, no, I haven't been able to find any time to work on telephoto since our last conversation. Your code looks very helpful though. I should be able to adapt it to something.

@hm-tamim
Copy link

hm-tamim commented Dec 4, 2024

Thank you. We are eagerly waiting for a release of Telephoto with this functionality 😄

saket added a commit that referenced this issue Dec 9, 2024
@saket saket closed this as completed in 1274e79 Dec 10, 2024
@saket
Copy link
Owner

saket commented Dec 10, 2024

Done! Can you give it a try by using 0.15.0-SNAPSHOT?

@mega-ht
Copy link

mega-ht commented Dec 10, 2024

Hello Saket, thanks for taking time to work in it. I've ran the app from trunk branch, but it seems there're some issues with the pan offset retention. You can find the sample code here.

@mega-ht
Copy link

mega-ht commented Dec 10, 2024

Pan.issue3.mp4

@saket
Copy link
Owner

saket commented Dec 10, 2024

Interesting, the code works fine for small size → full size, but not for thumbnail → small size. It looks like the calculation doesn't work for images that are upscaled and possibly needs to account for scaling as well.

@hm-tamim
Copy link

Correct. Also there's a slight shake when I pan on the image that was loaded at last.

@mega-ht
Copy link

mega-ht commented Dec 30, 2024

Hi @saket , should we reopen this ticket as it's not fully working?

@saket
Copy link
Owner

saket commented Dec 30, 2024

Yea let's do that

@saket saket reopened this Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants