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

constrainDuringPan causes jitter on mobile #1232

Open
gehan-hc opened this issue Jun 29, 2017 · 39 comments · May be fixed by #1298
Open

constrainDuringPan causes jitter on mobile #1232

gehan-hc opened this issue Jun 29, 2017 · 39 comments · May be fixed by #1298
Labels

Comments

@gehan-hc
Copy link

While the constrainDuringPan option stops you being able to pan the image too far away from the edges before it springs back, I would like to able to stop this behaviour completely - is this possible?

For example at the moment you can pan further left than the edge of the image, a bit of background is shown and then it springs back. I'd like the user to only be able to pan to the edge of the image and no further.

Is this possible or is there an option for this? I can't see it myself.

@iangilman
Copy link
Member

Strange... when I use constrainDuringPan: true it does exactly what you're asking for... I'm not allowed to pan off of the image at all, not even for a moment. Well, with the default visibilityRatio: 0.5 I can pan part way off the image, but it's still a hard stop; no springing. With visibilityRatio: 1 you can't pan off the image at all.

What version of OSD are you using? What other settings do you have? Can you share a repro?

@gehan-hc
Copy link
Author

gehan-hc commented Jul 10, 2017

It's actually on mobile only I noticed last week. If you open chrome dev tools and switch to mobile then the behavior will change to how I described. In normal desktop browser mode it's as you would expect.

@gehan-hc
Copy link
Author

I'm on v2.2.1

@iangilman
Copy link
Member

I see! Okay, it looks like it has to do with the second argument passed to panBy (the immediately argument) here:

https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L2679

Looks like we should force it to false whenever constrainDuringPan is on, or it'll get ahead of itself. You can add && !this.constrainDuringPan after the gestureSettings.flickEnabled.

It's only showing up in mobile because of the flickEnabled.

@gehan-hc Would you be up for making a patch for this?

@iangilman iangilman added bug and removed question labels Jul 10, 2017
@iangilman iangilman changed the title More aggressive constrainDuringPan option? constrainDuringPan causes jitter on mobile Jul 10, 2017
@gehan-hc
Copy link
Author

Sure I'll do a patch. You'll receive a PR from 'gehan' though, my personal github

@gehan
Copy link
Contributor

gehan commented Jul 11, 2017

There you go!

@gehan-hc
Copy link
Author

I tested it and while it does now constrain properly, if any of the edges are in view then it kind of 'sticks'. If you zoom in even just a little so that moving doesn't touch the edges then it's really smooth. As soon as your finger movement means that the tapestry would have hit the edge and the constrain logic is called then it gets a bit stuck. For instance when you move the image around it can stop moving until you let go then it springs over when you release.

@iangilman
Copy link
Member

@gehan-hc Thanks for getting the patch rolling! The sticking may have been addressed by #1133, which has just landed. Can you merge the latest master into your branch and test again?

@gehan
Copy link
Contributor

gehan commented Jul 11, 2017 via email

@gehan-hc
Copy link
Author

@iangilman #1133 on it's own almost fixed it - this patch seems to finish the job. So tested and mergeable for me 👍

@gehan-hc
Copy link
Author

gehan-hc commented Jul 12, 2017

PR - #1245

@iangilman
Copy link
Member

Fantastic, thank you!

iangilman added a commit that referenced this issue Jul 12, 2017
Fix for #1232 - constrainDuringPan causes jitter on mobile
@iangilman
Copy link
Member

Closed by #1245

@gehan-hc
Copy link
Author

Awesome. Will you be releasing another version any time soon? Quite a few changes on master since the last release a year ago. For the moment I'm just publishing master as a diff package so i can still npm it

@iangilman
Copy link
Member

Yes, hopefully tomorrow! See #1094

@gehan-hc
Copy link
Author

Awesome!

One thing I noticed actually after I build master was it was harder to move the image around. It was still smooth but it was like the sensitivity had been turned down. Is that a new option?

@iangilman
Copy link
Member

No, that shouldn't be the case. Can you tell me what browser, OS, and pointing device you were using, and what actions you did?

@gehan-hc
Copy link
Author

It's on mobile really (mine is android 8, chrome). I'm just panning around an image and its a lot more work to move about - enough that we can't use the new version really :'(

It's actually this - http://www.ireland.com/features/game-of-thrones-tapestry/

When we move the top image about it moves a lot slow. In that version (2.2.1) on a mobile you can see what I meant about the panning/constrain problem on a mobile. Perhaps I'll remove that fix I made a PR for and see if that changes anything as I think there seems to be another fix to do with constraining anyway

@gehan-hc
Copy link
Author

Ok if I revert my commit:
76f5a0e

Then the issue I described goes - it moves ok again. However now sometimes when I pan then the image gets 'stuck' and the inertia/animation stops working...!

@iangilman
Copy link
Member

I see! Good point... your patch removes the immediacy of the mobile panning. I think it probably would be good to revert (and yes, I remember it was my suggestion in the first place!).

Any thoughts on how to repro the sticking? I can't get it to happen (with a test page with that change reverted, and constrainDuringPan: 1, visibilityRatio: 1). And the inertia stops working when you get it stuck? Can you get it unstuck afterwards?

That page is amazing by the way! Is it still a work in progress, or is it ready to share?

@iangilman iangilman reopened this Jul 19, 2017
@gehan
Copy link
Contributor

gehan commented Jul 19, 2017 via email

@iangilman
Copy link
Member

Excellent... shared:

https://twitter.com/openseadragon/status/888105136760868865

It looks like the version that's deployed there doesn't have the sticking problem, but instead has the jitter, is that correct? Is it using 2.2.1?

Would it be possible to deploy somewhere else a copy with your modified 2.3.0 with the sticking problem so we can get to the bottom of it?

@iangilman iangilman added this to the 2.3.1 milestone Jul 26, 2017
@gehan
Copy link
Contributor

gehan commented Jul 28, 2017

I'll see if we can put it on a staging server or something.

@iangilman
Copy link
Member

@gehan Sounds good :)

@gehan-hc
Copy link
Author

gehan-hc commented Aug 1, 2017

I've asked if we can put it somewhere!

@gehan-hc
Copy link
Author

gehan-hc commented Aug 9, 2017

@iangilman I got someone to put it on QA - http://di-qa-ceip.hugoandcat.co.uk/features/game-of-thrones/

That's the same as production except using v2.3.0 of OSD

I'm only here for a another 2 weeks but @ticktockreed will be interested if you can improve things!

@iangilman
Copy link
Member

@gehan-hc Thank you! I'll take a look. And bravo again on a beautiful project :)

@iangilman
Copy link
Member

@gehan-hc Just so I'm clear, the QA version is running vanilla 2.3.0 and the live version is running 2.2.1? We don't have a version with the sticking problem with your modified 2.3.0?

At any rate, I think the sticking issue has to do with this code:

https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L2699-L2705

Specifically the fact that when it finds the new bounds are different than the constrained bounds, it just zeroes the delta. Instead it should limit the delta just enough to honor the constrained bounds.

For starters I'd recommend cleaning up the code a little so it's easier to work with the delta:

gestureSettings = this.gestureSettingsByDeviceType( event.pointerType );
var delta = this.viewport.deltaPointsFromPixels( event.delta.negate() );

if( !this.panHorizontal ){
    delta.x = 0;
}

if( !this.panVertical ){
    delta.y = 0;
}

if( this.constrainDuringPan ){
    this.viewport.centerSpringX.target.value += delta.x;
    this.viewport.centerSpringY.target.value += delta.y;

    var bounds = this.viewport.getBounds();
    var constrainedBounds = this.viewport.getConstrainedBounds();

    this.viewport.centerSpringX.target.value -= delta.x;
    this.viewport.centerSpringY.target.value -= delta.y;

    if (bounds.x != constrainedBounds.x) {
        delta.x = 0;
    }

    if (bounds.y != constrainedBounds.y) {
        delta.y = 0;
    }
}

this.viewport.panBy(delta, gestureSettings.flickEnabled && !this.constrainDuringPan);

...and then the delta.x = 0 bit should become something more like this:

    if (bounds.x != constrainedBounds.x) {
        delta.x += (constrainedBounds.x - bounds.x);
    }

...and same for delta.y, of course.

It's interesting to me that we're just comparing the x and y of the bounds and not the width and height; it's possible width and height need to be factored in as well, or maybe not since it's just panning.

Anyway, @gehan-hc I know you're moving on shortly, so maybe you don't have time to look at this. @ticktockreed might you be interested in giving this a try?

@ticktockreed
Copy link

@iangilman thanks for coming back to us on this. I'm definitely interested in giving it a try. Will aim to place in the next release

@iangilman
Copy link
Member

@ticktockreed Great, and pleased to meet you! Let me know if you need additional information from me.

@iangilman
Copy link
Member

I want to release a 2.3.1 soonish. If we don't resolve this entirely before then, I'm thinking it might still be worth taking out the !this.constrainDuringPan. What do you think, @ticktockreed ?

@iangilman iangilman linked a pull request Aug 29, 2017 that will close this issue
@iangilman
Copy link
Member

Okay, I've made that one change in #1298.

That doesn't fix the sticking that @gehan mentioned. That still needs the more nuanced delta.x/delta.y handling I mentioned above.

@gehan-hc
Copy link
Author

I have today and tomorrow left here - I'll try to have a go at lunch with #1298 and also the delta stuff above...! @ticktockreed is on holiday at the moment

@iangilman
Copy link
Member

@gehan-hc Sounds good... thank you for taking a look!

@gehan
Copy link
Contributor

gehan commented Aug 31, 2017

I tried the new constrain code but it has the same problem, if we do the below then there is no pan during constrain and so the image doesn't move enough and so feels sluggish:

this.viewport.panBy(delta, gestureSettings.flickEnabled && !this.constrainDuringPan);

If we pan during constrain and only change the deltas as required then it seems to move better but for some reason the panning stops dead a lot of the time on my phone.

this.viewport.panBy(delta, gestureSettings.flickEnabled);

I have don't really know how the springs work but it must be something to do with that as after altering the deltas the code just tells the springs to pan to that point immediately. Either way something kills the inertia - however only on my phone. Works great on chrome devtools as a phone!

@iangilman
Copy link
Member

@gehan Thank you for trying this out!

The post-drag intertia is handled here:

https://github.com/openseadragon/openseadragon/blob/master/src/viewer.js#L2715

You might try looking at the event.speed to see if it's a reasonable value.

It seems odd to me that changing the panBy line you mention is enough to change the behavior of the post-drag inertia. I wonder what the relationship is.

@iangilman
Copy link
Member

Here's a thought: it doesn't look like the post-drag inertia is handling constrainDuringPan. Perhaps what's happening is the inertia pushes it outside the constraints and the applyConstraints is somehow stopping it dead in its tracks.

@gehan-hc
Copy link
Author

gehan-hc commented Sep 6, 2017

Yep it must be something weird like that... but only on a physical mobile, which seems weird, but that is what is happening. So something to do with how touch/gestures/etc works on a mobile perhaps. So the way the delta adjustment is calculated needs to be done in a different way for a mobile then I guess.

@iangilman
Copy link
Member

Well, the momentum is only on mobile because of the gesture settings. You can make it happen in desktop like so:

viewer.gestureSettingsMouse.flickEnabled = true;

That might help with debugging; sorry I didn't mention it earlier!

And yes, it might have to do with the delta adjustment, but it might have to do with the post-drag inertia instead.

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.

4 participants