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.

Sign up for GitHub

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

Jump to bottom

limit animation duration on flyTo with maxDuration option #5349

Merged
merged 6 commits into from
Sep 29, 2017
Merged

limit animation duration on flyTo with maxDuration option #5349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a0facc
limit animation duration on flyTo with maxDuration option
stepankuzmin Sep 25, 2017
3886d82
add maxDuration description
stepankuzmin Sep 25, 2017
c019780
set the duration to 0 if flyTo duration exceeds maxDuration
stepankuzmin Sep 25, 2017
4100e87
clarify flyTo maxDuration option description
stepankuzmin Sep 26, 2017
f941090
add flyTo maxDuration option test
stepankuzmin Sep 27, 2017
ed8fdc4
decrease flyTo maxDuration option test precision
stepankuzmin Sep 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/ui/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,8 @@ class Camera extends Evented {
* It does not correspond to a fixed physical distance, but varies by zoom level.
* @param {number} [options.screenSpeed] The average speed of the animation measured in screenfuls
* per second, assuming a linear timing curve. If `options.speed` is specified, this option is ignored.
* @param {number} [options.maxDuration] The animation's maximum duration, measured in milliseconds.
Copy link
Member

Choose a reason for hiding this comment

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

Let's clarify what happens if duration exceeds maxDuration in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I've updated the PR.

* If duration exceeds maximum duration, it resets to 0.
* @param eventData Additional properties to be added to event objects of events triggered by this method.
* @fires movestart
* @fires zoomstart
Expand Down Expand Up @@ -803,6 +805,10 @@ class Camera extends Evented {
options.duration = 1000 * S / V;
}

if (options.maxDuration && options.duration > options.maxDuration) {
options.duration = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to set options.duration = options.maxDuration in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, @mollymerp! According to the last @mourner's comment in #3904, it was decided to reset duration to 0. So should I change it, or not?

Copy link
Member

Choose a reason for hiding this comment

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

@mollymerp yeah, that was intentional — consider a use case where you need to switch to a view from another part of the world, but you don't want it to take forever to animate. Setting to maxDuration would make the animation too fast, since it's a very long fly path — instead, just switching to the new view in a moment is usually a better experience IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So everything is fine, then? Should I write tests and add a new example?

Copy link
Member

Choose a reason for hiding this comment

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

@stepankuzmin definitely worth adding a test! I don't think a small option like this needs a separate example though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mourner alright fair enough! I would think if a user had a case with a very long "flight path" they would rather explicitly set duration to 0 but I can definitely see both as valid 🤷‍♀️

Copy link
Member

@mourner mourner Sep 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would think if a user had a case with a very long "flight path" they would rather explicitly set duration to 0

@mollymerp the user doesn't know beforehand how long the flight will take between two given positions (e.g. say you have a list of locations around the world and you click to switch between them in arbitrary order) — this is the primary reason for introducing maxDuration in the first place, and is similar to how other libs like Leaflet and Google Maps do it, disabling animation automatically if the flight path would be too long.

}

this.zooming = true;
this.rotating = (startBearing !== bearing);
this.pitching = (pitch !== startPitch);
Expand Down
16 changes: 16 additions & 0 deletions test/unit/ui/camera.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1201,6 +1201,22 @@ test('camera', (t) => {
camera.flyTo(flyOptions);
});

t.test('resets duration to 0 if it exceeds maxDuration', (t) => {
let startTime, endTime, timeDiff;
const camera = createCamera({ center: [37.63454, 55.75868], zoom: 18});

camera
.on('movestart', () => { startTime = new Date(); })
.on('moveend', () => {
endTime = new Date();
timeDiff = endTime - startTime;
t.equalWithPrecision(timeDiff, 0, 1e+1);
t.end();
});

camera.flyTo({ center: [-122.3998631, 37.7884307], maxDuration: 100 });
});

t.end();
});

Expand Down