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

add initial bounds as map constructor option #1970 #5518

Merged
merged 5 commits into from
Oct 8, 2018

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Oct 25, 2017

This PR will add initial bounds as map constructor option according to #1970.

I have a problem implementing the test for this case. Then I manually test new functionality using the debug page, I'm getting the expected result. But the same steps reproduced in test somehow outcomes with different zoom level in the result (1.113 instead of 2.469). What can cause this?

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page

src/ui/map.js Outdated
options.center = tr.unproject(nw.add(se).div(2));
options.zoom = Math.min(tr.scaleZoom(tr.scale * Math.min(scaleX, scaleY)), options.maxZoom);
}

Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse the code from fitBounds somehow and avoid copying it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we can try to use fitBounds(bounds, { duration: 0 }) instead of manually reproducing fitBounds logic, but we have to resize the map before.

this.resize();
this.fitBounds(options.bounds, { duration: 0 });

Unfortunately, the problem with the test is still there.

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 updated the PR using fitBounds


t.end();
});

Copy link
Member

Choose a reason for hiding this comment

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

But the same steps reproduced in test somehow outcomes with different zoom level

The zoom level depends on the size of the map container.

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 took https://github.com/mapbox/mapbox-gl-js/blob/master/test/unit/ui/camera.test.js#L1355 as an example. And it works fine with the zoom level.

@jfirebaugh jfirebaugh force-pushed the 1970 branch 2 times, most recently from 5f83282 to fed794d Compare May 1, 2018 15:43
@mourner
Copy link
Member

mourner commented Jul 25, 2018

@stepankuzmin let's get this shipped. Can you rebase on master and fix the build?

@stepankuzmin
Copy link
Contributor Author

Oh, well, It seems that I mixed up rebasing. Should I create separate PR on top of the current master branch?

@mourner
Copy link
Member

mourner commented Jul 25, 2018

@stepankuzmin I think you can reset the PR branch to the master (git reset --hard origin/master), apply your commit on top and force-push — no need to create a new PR.

@stepankuzmin
Copy link
Contributor Author

Thanks for help, @mourner! It seems that tests are passing now.

pitch: options.pitch
});
if (options.bounds) {
this.resize();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need the resize here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without resize, Transform's pixelMatrixInverse is undefined here:

vec4.transformMat4(coord0, coord0, this.pixelMatrixInverse);

Maybe we can move following resize call before condition?

this.resize();

Copy link
Member

Choose a reason for hiding this comment

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

@stepankuzmin yes, let's do that — seems to be more logical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@mourner
Copy link
Member

mourner commented Jul 25, 2018

Hmm, now the query tests fail for some reason :(

@stepankuzmin
Copy link
Contributor Author

stepankuzmin commented Jul 25, 2018

That's strange, because test-query now fails on upstream/master too. Same with test-render.

@mourner
Copy link
Member

mourner commented Aug 9, 2018

@stepankuzmin maybe let's rebase on top of the current master and revisit this?

@stepankuzmin
Copy link
Contributor Author

I've rebased on top of the current master branch, but the tests are still failing. And the problem is that I can't reproduce it locally — all 14 376 tests are passing.

@mourner
Copy link
Member

mourner commented Aug 9, 2018

@stepankuzmin the unit tests are passing; it's the query and render tests that fail. I can reproduce failures locally, e.g. by running ./build/run-node test/render.test.js 5911. I guess moving resize before the jumpTo somehow messes up map initialization in certain circumstances.

@ryanhamley
Copy link
Contributor

Can we pick this back up? I'd love to close out as many PRs as possible. I was just testing it out and rebasing it against master is still straightforward. The only change necessary to get all the tests passing is moving this.resize() into the conditionals. It's a bit messy to call it twice, but since it needs to be called in a specific order depending on what options are passed, I think that's ok.

            if (options.bounds) {
                this.resize();
                this.fitBounds(options.bounds, { duration: 0 });
            } else {
                this.jumpTo({
                    center: options.center,
                    zoom: options.zoom,
                    bearing: options.bearing,
                    pitch: options.pitch
                });
                this.resize();
            }

I did notice after rebasing that I had to change the expected zoom value in the test from 2.469 to 2.113 to get that test to pass. I'm not totally sure why that would be the case, but changing that value produces a passing test.

@stepankuzmin
Copy link
Contributor Author

Hey @mourner and @ryanhamley! Sorry for the late reply, I've updated the PR according to the last comment — moved resize back to conditional and set expected zoom value in the test to 2.113.

@mourner mourner merged commit 83aebca into mapbox:master Oct 8, 2018
@GeospatialMax
Copy link

@mourner Just stumbled upon this new feature which would be quite useful to us. When is the next release planned for this to be included as it's not in the current latest 0.50.0?

Thank you!

@ryanhamley
Copy link
Contributor

@GeovationMax This will be included in v0.51.0. The beta will be released next week and the final version will be released the following week, around November 7th.

@SoLoGHoST
Copy link

I am using the latest version js and css v0.51.0. Can someone please help me understand how to initialize the map with bounds? Doesn't work for me and I get no errors. It just doesn't seem to work.

@andrewharvey
Copy link
Collaborator

andrewharvey commented Nov 16, 2018

I am using the latest version js and css v0.51.0. Can someone please help me understand how to initialize the map with bounds? Doesn't work for me and I get no errors. It just doesn't seem to work.

See https://www.mapbox.com/mapbox-gl-js/api/#map there is a map option called bounds which accepts a LngLatBoundsLike

var map = new mapboxgl.Map({
    container: 'map',
    style: 'mapbox://styles/mapbox/streets-v9',
    bounds: [left, bottom, right, top]
});

@SoLoGHoST
Copy link

SoLoGHoST commented Nov 16, 2018

So, I'm misunderstood. How can I set a bunch of LngLat markers than have the map show with them centered in there by default? Instead of setting center property to a Lng,Lat.

@andrewharvey
Copy link
Collaborator

@SoLoGHoST Better to ask that at https://stackoverflow.com/tags/mapbox-gl-js

@SoLoGHoST
Copy link

Ok thank you, I have created a question here: https://stackoverflow.com/questions/53330202/set-map-bounds-based-on-multiple-marker-lng-lat

@elyobo
Copy link
Contributor

elyobo commented Dec 6, 2018

This is great, but is missing a way to provide the padding that fitBounds supports; this is quite important if you're fitting bounds to some markers, as without padding greater than the market dimensions they'll always sit partially off the map.

@mourner would you be open to accepting a PR that adds a boundsOptions parameter which would be passed to the fitBounds call here, except that it would always apply a duration of 0 as per the PR?

Added #7681

elyobo added a commit to elyobo/mapbox-gl-js that referenced this pull request Dec 6, 2018
Complements mapbox#5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
elyobo added a commit to elyobo/mapbox-gl-js that referenced this pull request Dec 6, 2018
Complements mapbox#5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
elyobo added a commit to elyobo/mapbox-gl-js that referenced this pull request Dec 6, 2018
Complements mapbox#5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
elyobo added a commit to elyobo/mapbox-gl-js that referenced this pull request Dec 6, 2018
Complements mapbox#5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
elyobo added a commit to elyobo/mapbox-gl-js that referenced this pull request Dec 6, 2018
Complements mapbox#5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
asheemmamoowala pushed a commit that referenced this pull request Dec 18, 2018
Complements #5518 by allowing additional options (notably padding) to be
provided for the initial bounds for a map.
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.

7 participants