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

don't reset bearing in fitbounds #1340

Closed
wants to merge 16 commits into from
Closed

don't reset bearing in fitbounds #1340

Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2bfe756
dont reset bearing on box zoom
kelvinabrokwa Jun 26, 2015
ffe1090
add visual bounds
kelvinabrokwa Jun 26, 2015
f90fc9c
account for rotation in zoom calculation
kelvinabrokwa Jun 26, 2015
40f0ecd
fix test
kelvinabrokwa Jun 26, 2015
90da31c
simplify min/max calls
kelvinabrokwa Jun 26, 2015
ca6a439
min for zoom
kelvinabrokwa Jun 26, 2015
10f2149
merge master
kelvinabrokwa Jun 29, 2015
7d70066
restart fix
kelvinabrokwa Jun 29, 2015
47fbf44
remove bearing from method call
kelvinabrokwa Jun 29, 2015
bc3d085
differentiate between coords and pixels
kelvinabrokwa Jun 29, 2015
b4c496d
inf var and use point-geometry lib for ops
kelvinabrokwa Jun 29, 2015
4d94596
remove padding from center calculation
kelvinabrokwa Jun 29, 2015
c6222f9
- -> + on sw
kelvinabrokwa Jun 29, 2015
f6b0745
remove console log
kelvinabrokwa Jun 29, 2015
f34ed88
project in the loop
kelvinabrokwa Jun 29, 2015
fdb5085
revert to original fix and hang head in shame
kelvinabrokwa Jul 1, 2015
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
71 changes: 58 additions & 13 deletions js/ui/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{
* @returns {Map} `this`
*/
fitBounds: function(bounds, options) {

options = util.extend({
padding: 0,
offset: [0, 0],
Expand All @@ -352,21 +351,67 @@ util.extend(Camera.prototype, /** @lends Map.prototype */{

bounds = LatLngBounds.convert(bounds);

var offset = Point.convert(options.offset),
tr = this.transform,
nw = tr.project(bounds.getNorthWest()),
var offset = Point.convert(options.offset);

var tr = this.transform;

var nw = tr.project(bounds.getNorthWest()),
se = tr.project(bounds.getSouthEast()),
size = se.sub(nw),
scaleX = (tr.width - options.padding * 2 - Math.abs(offset.x) * 2) / size.x,
scaleY = (tr.height - options.padding * 2 - Math.abs(offset.y) * 2) / size.y;
ne = tr.project(bounds.getNorthEast()),
sw = tr.project(bounds.getSouthWest());
Copy link
Contributor

Choose a reason for hiding this comment

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

The calls to project() need to take place inside the for loop below. As it is, you’re assuming that the northwest point remains the northwest point after rotation.


var segment = [ nw, sw, se, ne ];
Copy link
Contributor

Choose a reason for hiding this comment

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

The “segment” terminology in gl-native was unfortunate; I’m going to rename it to “points” or somesuch.


var nePixel = {
x: -Infinity,
y: -Infinity
};
var swPixel = {
x: Infinity,
y: Infinity
};

for (var i in segment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to use forEach() or a plain ol’ for loop in this case. In C++, the for…in syntax was cleaner than the alternative, but there isn’t much to be gained here.

Copy link
Member

Choose a reason for hiding this comment

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

Always iterate over arrays with a plain for loop unless there's a strong reason not to.

var pixel = segment[i];
sw.x = Math.min(swPixel.x, pixel.x);
ne.x = Math.max(nePixel.x, pixel.x);
sw.y = Math.min(swPixel.y, pixel.y);
ne.y = Math.max(nePixel.y, pixel.y);
}

var size = [
ne.x - sw.x,
ne.y - sw.y
];

// zoom
var scaleX = (tr.width - options.padding * 2 - Math.abs(offset.x) * 2) / size.x;
Copy link
Member

Choose a reason for hiding this comment

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

You create size as array but then use size.x... Doesn't look right.

var scaleY = (tr.height - options.padding * 2 - Math.abs(offset.y) * 2) / size.y;
var minScale = Math.min(scaleX, scaleY);
var zoom = Math.min(tr.scaleZoom(tr.scale * minScale), options.maxZoom);

// center
var paddedNEPixel = {
x: ne.x + options.padding / minScale,
y: ne.y + options.padding / minScale
};

var paddedSWPixel = {
x: sw.x - options.padding / minScale,
y: sw.y - options.padding / minScale
};

var centerPixel = {
x: (paddedNEPixel.x + paddedSWPixel.x) / 2,
y: (paddedNEPixel.y + paddedSWPixel.y) / 2
};
Copy link
Member

Choose a reason for hiding this comment

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

We use point-geometry module for operations like this, e.g.:

var centerPixel = paddedNEPixel.add(paddedSWPixel).div(2);

var center = tr.unproject(centerPixel);

options.zoom = zoom;
options.center = center;

options.center = tr.unproject(nw.add(se).div(2));
options.zoom = Math.min(tr.scaleZoom(tr.scale * Math.min(scaleX, scaleY)), options.maxZoom);
options.bearing = 0;
return options.linear ? this.easeTo(options) : this.flyTo(options);

return options.linear ?
this.easeTo(options) :
this.flyTo(options);
},

/**
Expand Down