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

Shared Modifier.zoomable() shares invalid transformation when SubSamplingImage is used #108

Open
SzyQ opened this issue Oct 27, 2024 · 15 comments

Comments

@SzyQ
Copy link

SzyQ commented Oct 27, 2024

Hi!

I have observed that shared ZoomableState like in this concept here is not scaling and translating content equally when SubSamplingImage is used.

See the behaviour on a video:
https://github.com/user-attachments/assets/151706a0-f15e-4803-86a8-d5a3bf73ace0

Modifier.zoomable() works perfectly fine on other types of Composables, so presumably transformations applied by SubSamplingImage are not reflected to the ZoomableState.

Dirty modification of the sample code to showcase the problem:

@Composable
private fun MediaPage(
  model: MediaItem,
  isActivePage: Boolean,
  modifier: Modifier = Modifier,
) {
  val zoomableState = rememberZoomableState()
  val focusRequester = remember { FocusRequester() }

  val flickState = rememberFlickToDismissState(dismissThresholdRatio = 0.05f)
  CloseScreenOnFlickDismissEffect(flickState)

  var mutatedState by remember { mutableStateOf(model) }

  Box(contentAlignment = Alignment.Center) {

    when (val model = mutatedState) {
      is MediaItem.Image -> {
        Box(
          modifier = Modifier.background(Color.Green.copy(alpha = 0.2f))
        ) {
          // TODO: handle errors here.
          val imageState = rememberZoomableImageState(zoomableState)
          ZoomableAsyncImage(
            modifier = modifier.focusRequester(focusRequester),
            state = imageState,
            model = ImageRequest.Builder(LocalContext.current)
              .data(model.fullSizedUrl)
              .placeholderMemoryCacheKey(model.placeholderImageUrl)
              .crossfade(300)
              .build(),
            contentDescription = model.caption,
          )

          // Focus the image so that it can receive keyboard and mouse shortcut events.
          if (isActivePage) {
            LaunchedEffect(Unit) {
              focusRequester.requestFocus()
            }
          }

          AnimatedVisibility(
            modifier = Modifier.align(Alignment.Center),
            visible = !imageState.isImageDisplayed
          ) {
            CircularProgressIndicator(color = Color.White)
          }
        }
      }
      is MediaItem.Video -> {
        Box(
          contentAlignment = Alignment.Center,
          modifier = Modifier
            .fillMaxSize()
            .zoomable(zoomableState).background(Color.Blue.copy(alpha = 0.2f)),
        ) {
          var size by remember { mutableStateOf(androidx.compose.ui.geometry.Size.Unspecified) }
          LaunchedEffect(size) {
            zoomableState.setContentLocation(ZoomableContentLocation.scaledInsideAndCenterAligned(size)) //is that correct?
          }
          AsyncImage(
            onSuccess = {
              size = it.painter.intrinsicSize
            },
            model = model.fullSizedUrl,
            contentDescription = model.caption
          )
        }
      }
    }
    Button(onClick = {
      mutatedState = when (val model = mutatedState) {
        is MediaItem.Image -> {
          MediaItem.Video(model.fullSizedUrl, model.placeholderImageUrl, model.caption)
        }
        is MediaItem.Video -> {
          MediaItem.Image(model.fullSizedUrl, model.placeholderImageUrl, model.caption)
        }
      }
    }) {
      Text(text = "Switch")
    }
  }

Is such usage actually supported?

Thanks!
Szymon

@saket
Copy link
Owner

saket commented Oct 28, 2024

I now realize that that example needs clarification. While a ZoomableState instance can be hoisted outside, it cannot be shared between two active composables. The example assumes that media items will maintain their format during runtime.

Although it's technically possible to share a single ZoomableState instance across multiple zoomable composables by calling resetZoom(snap()), I wouldn't recommend doing it.

@rharter
Copy link
Contributor

rharter commented Oct 28, 2024

I believe that the example above doesn't share the state between two active composables, if that's the rub. Only one will be active at a time. This is to implement MotionPhoto support similar to Google Photos, in which you can turn the motion photo (i.e. ExoPlayer) on and off and also transform the video and photo.

@saket
Copy link
Owner

saket commented Oct 28, 2024

Sorry I should have probably written "if both are ever active".

This is to implement MotionPhoto support similar to Google Photos, in which you can turn the motion photo (i.e. ExoPlayer) on and off and also transform the video and photo.

Ooh that's interesting. I noticed that Google Photos uses the same width and height when it switches between images and motion videos. Can your app do the same? I would kinda expect ZoomableState to work if the old and the new composables have the same layout and content size.

@SzyQ
Copy link
Author

SzyQ commented Oct 28, 2024

Making both view the same size should not be a problem, however in the modified example above both ZoomableAsyncImage and AsyncImage are loading the same image aligned the same way.
It would work if instead of AsyncImage there would be ZoomableAsyncImage, but the problem appears when calling zoomableState.setContentLocation. ZoomableAsyncImage is using ZoombaleContentLocation.unscaledAndTopLeftAligned and AsyncImage is using the other type.
The resulted transformation differ between those two visually.

@rharter
Copy link
Contributor

rharter commented Oct 28, 2024

This is a tricky one to wrap my head around, but I think that the underlying issue is that there are two transformations that are being combined into one. There is an initial transformation that's needed to fit the content to the layout, which is combined with the user specified transformations.

In order to store user transformations that can be applied to differently-sized content, I think those would need to be separate. That way you can basically have a userTransformations state holder that applies after the content is transformed to it's container.

Currently, I think the issue is that the ZoomableState uses the original size of the content as the content size and takes care of the positioning it, applying the user-specified transformation on top of that, so even if two pieces of content are put in the same container, the same ZoomableState won't affect them the same because what you want is to update the content transformation to fit the container first, which will effectively make the content uniform, then you want to apply the user specified transformations on top of that.

@saket
Copy link
Owner

saket commented Nov 4, 2024

There is an initial transformation that's needed to fit the content to the layout, which is combined with the user specified transformations.

The timing of this couldn't have been any better because I just fixed this. @SzyQ wanna give 0.14.0-SNAPSHOT a try to see if it fixes or improves the situation?

@SzyQ
Copy link
Author

SzyQ commented Nov 4, 2024

Sure, is this snapshot in some particular maven that I can use?

@saket
Copy link
Owner

saket commented Nov 4, 2024

Yes, it's available in the snapshot repository:

maven(url = "https://oss.sonatype.org/content/repositories/snapshots")

@SzyQ
Copy link
Author

SzyQ commented Nov 4, 2024

Gave it a shot on my real-life example and the result was not consistent. I am switching between ZoomableImage and a Video, both have different sizes so that may be a reason for invalid transformations. I will try to provide more valuable feedback once I can spend more time on some simpler sample.

@saket
Copy link
Owner

saket commented Nov 5, 2024

Yep, the size needs to be the same. Are you not synchronizing the size of your images with their videos?

I'm imagining that your support for motion photos will work similarly to Google Photos:

screen-20241104-204426.2.mp4

@SzyQ
Copy link
Author

SzyQ commented Nov 17, 2024

Hey, sorry for late reply. The video has original size, which is the size of video inside motion jpg file. That can be different then the image itself. My usecase is very specific and currently what works just fine is using PainterDelegate(that one is not calling zoomableState.setContentLocation, which lets me display video on top of the image) instead of SubSamplingDelegate. I'm closing the ticket, thanks for help. Really cool lib!

@SzyQ SzyQ closed this as completed Nov 17, 2024
@saket
Copy link
Owner

saket commented Nov 18, 2024 via email

@SzyQ
Copy link
Author

SzyQ commented Nov 19, 2024

Well, not with SubSamplingDelegate, because it modifies shared ZoomableState while displayed. Ideally ZoomableState would need to just represent values of the zoom performed by user, where internal state of the Modifier.zoomable transform such ZoomableState to it's own world.
I didn't invest more time into this, since Painter delegate was good enough and is not modifying ZoomableState, what makes it possible to share it between two Compostables displayed at the same time(image and video).

@saket saket reopened this Nov 19, 2024
@saket
Copy link
Owner

saket commented Nov 19, 2024

since Painter delegate was good enough

Do you not need sub-sampling of large images? Based on the discussion above it sounded like you're using telephoto for displaying arbitrary images from the user where a painter delegate will either OOM or display a downsized image.

@saket
Copy link
Owner

saket commented Dec 10, 2024

@SzyQ telephoto now retains pan across size changes (1274e79). Can you give it a try by using 0.15.0-SNAPSHOT?

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

3 participants