alexvasilkov / GestureViews

ImageView and FrameLayout with gestures control and position animation
Apache License 2.0
2.37k stars 384 forks source link

Erratic behavior when animating/setting padding in GestureImageView #165

Closed ParticleCore closed 3 years ago

ParticleCore commented 3 years ago

I was trying to come up with a way to vertically offset the image inside a GestureImageView when I noticed an erratic pattern that is not consistent.

Initially I tried to play with .setViewPort(), but that only controls the size of the image, not the position. Then I checked padding and it works, to a certain degree.

If I set the padding manually in the xml layout file the image is exactly positioned as I expect it to be, but because there is UI that overlaps the view and that UI can be toggled with sliding animation I tried to animate the padding as well.

The top and left padding animations work perfectly, but for some reason the bottom and right do not work the same way, it feels like they are being completely ignored.

This video shows a basic example of what I am describing, please ignore the jerkiness/jumps because those are due to the fullscreen entering/exiting and are irrelevant to this example.

gif5

The top and left padding animation are working as expected, however the right and bottom padding values are not. Worse, they seem to be "taken into account" when the values are set back to normal.

This feels like the viewport position is being correctly updated, but the resizing/scale of it is not.

To replicate this problem replace the following code in this location: https://github.com/alexvasilkov/GestureViews/blob/master/sample/src/main/java/com/alexvasilkov/gestures/sample/demo/adapter/PhotoPagerAdapter.java#L66-L78

With this

    @Override
    public ViewHolder onCreateViewHolder(@NonNull ViewGroup container) {
        final ViewHolder holder = new ViewHolder(container);

        holder.image.setOnClickListener(view -> {
            onImageClick();

            // here we are simply animating all paddings of the view at the same
            // time to 200px or 0px depending on if there is a padding already set
            boolean show = holder.image.getPaddingTop() == 0;
            ValueAnimator.AnimatorUpdateListener updateListener2 = animation -> {
                holder.image.setPadding(
                    (int) animation.getAnimatedValue(),
                    (int) animation.getAnimatedValue(),
                    (int) animation.getAnimatedValue(),
                    (int) animation.getAnimatedValue()
                );
            };
            ValueAnimator valueAnimator = ObjectAnimator.ofInt(show ? 0 : 200, show ? 200 : 0);
            valueAnimator.setDuration(1000);
            valueAnimator.addUpdateListener(updateListener2);
            valueAnimator.start();
            // end of code modification, everything else is the original untouched code

        });

        settingsController.apply(holder.image);

        holder.image.getController().enableScrollInViewPager(viewPager);
        holder.image.getPositionAnimator().addPositionUpdateListener((position, isLeaving) ->
                holder.progress.setVisibility(position == 1f ? View.VISIBLE : View.INVISIBLE));
        return holder;
    }

Can we get this fixed somehow?


Note: I tried to play with changing the UI heights/real positions (not translations, but actual view repositioning) with the GestureView constrained between them, but this interferes with the gesture behavior since the height adjustments are interpreted as gestures as well if the user is touching the view during the UI transitions. I also tried to just leave the view constrained with no views changing dimensions, but when the enter/leave animation is automatically clipped at the edges of the view (as if the view limits were the device screen limits).

The only working solution that has almost worked is to play with the padding and I cannot think of anything else to try.

ParticleCore commented 3 years ago

With the help of the GestureDebug.setDrawDebugOverlay(true); we can understand a bit better what is going on:

gif6

ParticleCore commented 3 years ago

I also tried to do this differently by using an ImageView inside a GestureFrameLayout and although the padding situation is resolved that way, the enter/leave animations are thrown completely out of wack because it is not accounting for those offsets for the animation calculation. The option to use .setTranslationY is not useful since it does not change the size of the view itself and using scale will result in areas around the image where gestures will just not work.

alexvasilkov commented 3 years ago

It turns out GestureImageView.onSizeChanged is not called when paddings are changed (which is expected though) so the view does not auatomatically updates it's viewport and state.

In current version you have to do it manually in eacn animation step with:

                controller.getSettings().setViewport(
                        img.getWidth() - img.getPaddingLeft() - img.getPaddingRight(),
                        img.getHeight() - img.getPaddingTop() - img.getPaddingBottom());
                controller.resetState();

Note that the view state (zoom, translation, etc) will be reset without animations in this case. If you want a smoother transition then you need to update the state manually and call updateState() instead.

ParticleCore commented 3 years ago

I see, so the ViewPort is the way to go afterall, but I have to take into account the existing padding (or in this case, the ones that I am actually setting) to allow the view to offset the way that I want without affecting the remaining gesture behavior. I'll try that one out, it seems to be the most logical solution and I haven't thought of it before either.

ParticleCore commented 3 years ago

In the meantime your clue about the onSizeChanged already provided an ugly and easy workaround for this issue. Every time the padding is updated I also set a new value that causes that method to be called, for example view.setTop(int top) which works and I do that by calling the following right after setting the padding values

                int top = holder.image.getTop();
                holder.image.setTop(top + 1);
                holder.image.setTop(top);

Like I mentioned, this is ugly but it works. We cannot set the current value because the view ignores the value if there is no change, which makes sense:

    // this code is part of the Google's android.view.View.java source code
    // of which GestureImageView extends from
    public final void setTop(int top) {
        if (top != mTop) { // can't use the same number

I still have to test the viewport + padding option as well.

ParticleCore commented 3 years ago

I have not been able to make your suggestion work, I'm afraid I might not know exactly how to do it.

I tried doing the following:

binding.image.getController().getSettings().setViewport(
    binding.image.getWidth(),
      myModifiedHeight
);
binding.image.getController().updateState();

But the height of the visible image does not change even if the viewport is changing.

I also tried the following

State state = binding.image.getController().getState();
state.translateTo(0, anyValue); // also tried translateBy
binding.image.getController().updateState();

But it is not doing anything and I have a feeling that is because it is bound to the viewport and it does not allow any translation unless it is to exit or if the image is zoomed in. This means I am stuck without being able to offset it vertically this way.

One last issue is the only one that is working so far is the one I mentioned before by setting top and resetting to trigger onSizeChanged, but I discovered one downside to it which is if the user had the image zoomed and then the UI is toggled the image zoom is lost, it resets it to fit the image to the viewport.

Any pointers that I could follow to get this working?

alexvasilkov commented 3 years ago

I mentioned the exact fix in my previous comment, just put it right after you set new paddings.

You hack should also work, though it is a little bit less efficient.

ParticleCore commented 3 years ago

I mentioned the exact fix in my previous comment, just put it right after you set new paddings.

You hack should also work, though it is a little bit less efficient.

In my head I was so far off from understanding something so simple. Yes, following your example it works right away, but the same issue as my hack persists: any zoom is lost between those changes - which you had already noted before - and that is where I got stuck previously.

After this I went about it this way:

Now the zoom is not lost because it is being restored in between viewport changes, same for rotation and translation.

Here is the full code in case it is useful for anyone else in the future:

            // change your paddings according to your needs
            // in my case I only need to make vertical changes
            image.setPadding(
                image.getPaddingStart(),
                myTopPadding,
                image.getPaddingEnd(),
                myBottomPadding
            );
            // copy your current gesture view state prior to any view port changes
            State state = image.getController().getState().copy();
            // update the viewport minus the padding values that were previously set
            image.getController().getSettings().setViewport(
                image.getWidth() - image.getPaddingLeft() - image.getPaddingRight(),
                image.getHeight() - image.getPaddingTop() - image.getPaddingBottom());
            // restore all the state valus prior to view port changes
            image.getController().getState().set(
                state.getX(),
                state.getY(),
                state.getZoom(),
                state.getRotation()
            );
            // instruct the gesture view to update itself using the new values that were set
            image.getController().updateState();

            (...) // make any further changes you want to your UI such as sliding the
                  // toolbar or any details container at the bottom of the screen out of view
ParticleCore commented 3 years ago

I'll close this now since the issue has been resolved and it wasn't technically an issue with the library itself as much as it was not the proper way to do this. Now that I understood your explanation I was able to resolve my predicament and I thank you for it.