limit animation duration on flyTo with maxDuration option #5349
Conversation
| @@ -665,6 +665,7 @@ 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. | |||
There was a problem hiding this comment.
Let's clarify what happens if duration exceeds maxDuration in the docs.
There was a problem hiding this comment.
Thanks for the review! I've updated the PR.
There was a problem hiding this comment.
Thank you for the contribution @stepankuzmin! I just requested one small change + the resultant documentation change.
| @@ -803,6 +805,10 @@ class Camera extends Evented { | |||
| options.duration = 1000 * S / V; | |||
| } | |||
|
|
|||
| if (options.maxDuration && options.duration > options.maxDuration) { | |||
| options.duration = 0; | |||
There was a problem hiding this comment.
I think it would make more sense to set options.duration = options.maxDuration in this case.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
So everything is fine, then? Should I write tests and add a new example?
There was a problem hiding this comment.
@stepankuzmin definitely worth adding a test! I don't think a small option like this needs a separate example though.
There was a problem hiding this comment.
@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 🤷♀️
There was a problem hiding this comment.
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.
|
Thanks for the review! I've added the test. |
Hi there! I've added the
maxDurationoption to limit animation duration onflyToaccording to #3904.If this is the right way to do it, I'll add tests and documentation.
Launch Checklist