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

don't stop animation on map resize #6636

Merged
merged 4 commits into from
Jun 27, 2018
Merged

don't stop animation on map resize #6636

merged 4 commits into from
Jun 27, 2018

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented May 8, 2018

Launch Checklist

allows animations from flyTo to continue even when the map resizes. You can end up with the point you're flyingTo no longer being inside the map window if the map is resized a lot, but I feel it's no worse than the current behaviour, something than can be fixed in a future PR, and addresses the root cause of mapbox-gl-geocoder not working on Android.

  • write tests for all new functionality

  • check new unit test fails on master without this change

  • n/a document any changes to public APIs

  • n/a post benchmark scores

  • manually test the debug page

  • n/a tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes

@jdaudier
Copy link

jdaudier commented Jun 4, 2018

I can't wait for this to land! I need this fix!

@andrewharvey
Copy link
Collaborator Author

I'm not too familiar with the animation/interaction code, so not sure if there are any implications of this change I've overlooked.

@mollymerp
Copy link
Contributor

hmm yeah – it does temporarily bork the transform state but it does seem to me that this behavior is preferable considering the drawbacks of the current approach. @mourner @ansis are there undesirable side-effects of this that I'm missing?

if (options.interactive) {
map.stop();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related to the resize fix or something separate?
No need to split it out if it's separate, I'm just wondering if I'm missing a relation between this and the resize bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I needed this so that scrolling on the map cancelled an active flyTo.

@ansis
Copy link
Contributor

ansis commented Jun 6, 2018

@mollymerp nah, I think this is fine. If it doesn't finish in the right location we should try to fix that but this seems fine to me.

@andrewharvey
Copy link
Collaborator Author

test-flow is failing with:

Cannot call `this.resize()._update` because property `_update` is missing in `Evented` [1].

   src/ui/map.js:1738:13
   1738|             this.resize()._update();
                     ^^^^^^^^^^^^^^^^^^^^^^^

References:
   src/util/evented.js:47:14
     47| export class Evented {
                      ^^^^^^^ [1]

🤔

@mollymerp
Copy link
Contributor

@andrewharvey I think flow was confused by returning this.fire.... from map.resize() 🤷‍♀️

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ansis ansis merged commit 47be75c into master Jun 27, 2018
@ansis ansis deleted the 4041 branch June 27, 2018 14:25
@jdaudier
Copy link

Any idea when this fix will be released?

andrewharvey added a commit to andrewharvey/road-orientation-map that referenced this pull request Jul 18, 2018
mourner pushed a commit to mourner/road-orientation-map that referenced this pull request Jul 18, 2018
@allthesignals
Copy link
Contributor

This is a little tangential, but what is the behavior called when MapboxGL (or WebGL) pauses animation (or rendering) when the window/tab isn't "active" (isn't visible in the user's desktop)? Having an automation issue, and trying to find the correct language to understand...

@ryanhamley
Copy link
Contributor

ryanhamley commented Oct 3, 2018

AFAIK, we (and WebGL) do not pause rendering on background tabs. Processes in background tabs are throttled as the default behavior of many modern browsers for performance reasons.

@allthesignals
Copy link
Contributor

allthesignals commented Oct 3, 2018

@ryanhamley thank you! Makes sense that it's way lower level than a MapboxGL thing.

EDIT: https://winaero.com/blog/disable-tab-throttling-google-chrome/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants