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

map#isMoving #3068

Closed
wants to merge 8 commits into from
Closed

map#isMoving #3068

wants to merge 8 commits into from

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Aug 25, 2016

Adds a method Map#isMoving that checks for the states of this.zooming, this.rotating, this.pitching, map.dragPan._active, and map.dragRotate._active. If any of them is true, the map is considered "moving" and the method returns true.

mapbox-gl-jsismoving

refs: #2792

TODO

  • add tests, @lucaswoj I was thinking a test that looks something like this:
test('Map#isMoving returns expected results', function (t) {
    var map = createMap();
    var moving = false;
    var notMoving = false;

    function check () {
        if (map.isMoving()) moving = true;
        if (!map.isMoving()) notMoving = true;
    }

    map.on('load', function() {
        map.flyTo({center: [0, 0], zoom: 9});
    });

    map.on('render', check);

    map.on('moveend', function() {
        t.ok(moving, 'map was considered moving at one point');
        t.ok(notMoving, 'map was considered not moving at one point');
        t.end();
    });
});

But not having any luck with it so far.

cc @mollymerp @jfirebaugh

this.rotating ||
this.pitching ||
this.dragPan._active ||
this.dragRotate._active) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that we need to mix the "interaction handler" layer of abstraction the "camera" layer of abstraction to compute Map#isMoving. Should Map#isMoving return true if the user starts a "drag rotate" interaction but doesn't actually rotate the map? Or if the user starts a "drag pan" interaction but doesn't pan the map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good questions! I'm not familiar enough with these interactions enough to know the differences. The only way I was able to capture "pan" and "rotate" actions was through those dragPan and dragRotate parameters, though. Are there other places to look for them?

Should Map#isMoving return true if the user starts a "drag rotate" interaction but doesn't actually rotate the map?

I'm not sure - any chance you could provide a case where this happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only way I was able to capture "pan" and "rotate" actions was through those dragPan and dragRotate parameters, though. Are there other places to look for them?

We may need to create the place to look for this state by

  • creating map.panning property
  • ensuring that map.rotating and map.rotating works with the dragRotate handler

I'm not sure - any chance you could provide a case where this happens?

Imagine I click and drag the map by 5 pixels, initiating a "drag pan" gesture, put a rock on my mouse button, and go for lunch. Map#isMoving will be true the whole time I'm gone but the map won't be moving. Is that the behaviour we want? I'm not sure. It deserves some discussion.

@lucaswoj
Copy link
Contributor

Using Map#flyTo in tests is tricky!

The default duration of Map#flyTo is rather long relative to our unit test execution time. In #2259 we sped up our unit tests from 19 seconds to 5 seconds by shortening the duration of a Map#flyTo test 😬

Perhaps the render event isn't getting fired as expected in our testing environment. Have you tried listening to move events instead?

@mapsam
Copy link
Contributor Author

mapsam commented Sep 6, 2016

@lucaswoj just added map.easing as a property, which is enacted within map._easeTo and cancelled within map._easeEnd

Since zooming, rotating, and pitching are all within the ease functions, they're probably not 100% required any longer, but I haven't removed them just in case.

I also moved the map.dragRotate and map.dragPan checks to use their respective isActive() methods, rather than checking a private property.

Next up, tests!

@lucaswoj
Copy link
Contributor

lucaswoj commented Sep 6, 2016

Let's define a "moving" map as a map whose center coordinate, bearing, zoom, or pitch is expected to be different in the next frame. Whether or not we "expect" these parameters to be different on the next frame during an interaction is an open question.

The word "easing" feels overloaded:

  • the dictionary definition is to "move carefully, gradually, or gently"
  • it can refer to the Map#easeTo operation
  • it can refer to the internal Camera#_ease operation, used by both Map#easeTo and Map#flyTo

Let's think carefully about how we're using these words and how we can refactor to make their use more clear :-) #2801


To answer your original question:

wondering if instead of the map.easing property, can I just use map.isEasing()

Looks like this would work 👍

@mapsam
Copy link
Contributor Author

mapsam commented Sep 6, 2016

Sure thing @lucaswoj. Since we already have the isEasing() method there's no need to start adding more properties/language like map.easing.

Since all of the potential "movements" have their own isSomething() methods except for zooming (i.e. there is no isZooming() method), it seems like we don't need to add anything else.

@mapsam
Copy link
Contributor Author

mapsam commented Sep 6, 2016

@lucaswoj I've added a test that checks if the map is moving before, during, and after a flyTo animation. Let me know what you think!

@mapsam
Copy link
Contributor Author

mapsam commented Nov 11, 2016

Closing this. I've got a better grasp on the camera movement, and think this can be better implemented once #3583 lands.

@mapsam mapsam closed this Nov 11, 2016
@jfirebaugh jfirebaugh deleted the isMoving branch February 3, 2017 18:02
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