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

style: use requestAnimationFrame for auto pan #71

Merged
merged 9 commits into from
Feb 1, 2018

Conversation

auroranil
Copy link
Contributor

Using requestAnimationFrame provides smoother auto panning than using setInterval with a large interval.

Delta values have been reduced so that the speed of the auto pan is roughly the same as before (but without the large jumps).

This occurs when scrolling with the mouse wheel.
Delta values have been reduced so that the speed of the auto pan is roughly the same.

Using requestAnimationFrame provides smoother auto panning than using setInterval with a large interval.
@chrvadala
Copy link
Owner

Thanks for your PR. It seems a really good improvement. I gonna make some tests then I'll merge.

@auroranil
Copy link
Contributor Author

Also resolves #31, thanks to @brentoneill

@chrvadala
Copy link
Owner

chrvadala commented Jan 28, 2018

Hi @auroranil, I've just tested your work (sorry for the delay). It works well. The auto pan feature is absolutely better.
About the zoom limits feature, I think that doesn't contain the mobile support. We need to add it to guarantee a consistent behaviour also on smartphones and tablets.

I think that to do it, we can just change interactions-touch.js:13 to use the zoom features available in zoom.js:15. Thought?

@auroranil
Copy link
Contributor Author

I have manually tested this, and it works on both my web browser (Chrome) as well as an iPad (Safari).

I refactored the codebase, so that scaleFactorMin and scaleFactorMax are both set into the value variable, rather than passing the prop variable around in several functions.

For zoom, fitSelection and onMultiTouch functions, there are two zoom limit checks. The first one prevents zoom from occurring, and subsequently the unwanted translations with no changes in zoom levels. The second one enforces the zoom level limits, by clamping a and d values so that it lies in the bound [scaleFactorMin, scaleFactorMax].

If scaleFactorMin or scaleFactorMax are unspecified, then it allows infinite zoom out or in, respectively.

@chrvadala
Copy link
Owner

Great job man! I'm going to merge and publish!

@chrvadala chrvadala merged commit 7c2b47b into chrvadala:master Feb 1, 2018
@chrvadala chrvadala removed the WIP label Feb 1, 2018
@chrvadala
Copy link
Owner

Released with v2.15.0

@@ -19,7 +20,12 @@ function onMultiTouch(event, ViewerDOM, tool, value, props) {
const pinchPointDistance = Math.sqrt(Math.pow(x2 - x1, 2) + Math.pow(y2 - y1, 2));
const previousPointDistance = hasPinchPointDistance(value) ? value.pinchPointDistance : pinchPointDistance;
const svgPoint = getSVGPoint(value, (x1 + x2) / 2, (y1 + y2) / 2);
const distanceFactor = pinchPointDistance/previousPointDistance;
let distanceFactor = pinchPointDistance/previousPointDistance;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed this to let as I was modifying this variable at some point in development, but now there is no reason for this change. Although it's no big deal, I should be using const.

@@ -29,7 +29,7 @@ export function onMouseDown(event, ViewerDOM, tool, value, props, coords = null)
switch (tool) {
case TOOL_ZOOM_OUT:
let SVGPoint = getSVGPoint(value, x, y);
nextValue = zoom(value, SVGPoint.x, SVGPoint.y, 1 / props.scaleFactor);
nextValue = zoom(value, SVGPoint.x, SVGPoint.y, 1 / props.scaleFactor, props);
Copy link
Contributor Author

@auroranil auroranil Feb 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was passing props around for scaleFactorMin and scaleFactorMax values, but now they are being passed through the value argument.

I forgot to remove these props arguments in zoom and stopZooming functions. That should not be there.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point about this change is that I'm not sure that set scaleFactorMin and scaleFactorMax in the value object is the right way to handle this two props. These are two statics props and probably they don't change in the component lifecycle. Talking about refactor I think that the previous way to pass them was better.

}
}, 200);
this.autoPanIsRunning = true;
requestAnimationFrame(this.autoPanLoop);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs the auto pan loop when component mounts, whether or not the user is actually auto panning. We should refactor the code so that auto pan loop only runs when the user hovers on one or more of the auto pan regions.

I have not thoroughly check the performance metrics, so we should consider optimising the code if the performance drop is too large.

Copy link
Owner

@chrvadala chrvadala Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... to handle this we can use the mouseEnter and mouseLeave callbacks

onMouseEnter={ event => {
if (detectTouch()) return;
let nextValue = onMouseEnterOrLeave(event, this.ViewerDOM, this.getTool(), this.getValue(), this.props);
if (this.getValue() !== nextValue) this.setValue(nextValue);
}}
onMouseLeave={ event => {
let nextValue = onMouseEnterOrLeave(event, this.ViewerDOM, this.getTool(), this.getValue(), this.props);
if (this.getValue() !== nextValue) this.setValue(nextValue);
}}
but I think that it isn't really necessary. I suppose that usually there's at most one running instance of it (2, 3 in some complex situations). It should not cause a performance drop.

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

Successfully merging this pull request may close these issues.

2 participants