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

increases max zoom levels for raster uploads #139

Merged
merged 6 commits into from
Apr 25, 2016
Merged

increases max zoom levels for raster uploads #139

merged 6 commits into from
Apr 25, 2016

Conversation

isiyu
Copy link
Contributor

@isiyu isiyu commented Apr 14, 2016

refs https://github.com/mapbox/unpacker/issues/852

@dnomadb does this need more SPATIAL RESOLUTIONS tests for the new zoom levels?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling da07395 on moremaxzoom into 805d115 on master.

@dnomadb
Copy link
Contributor

dnomadb commented Apr 19, 2016

does this need more SPATIAL RESOLUTIONS tests for the new zoom levels?

@isiyu y.

@@ -410,9 +410,9 @@ tape('[SPATIAL RESOLUTIONS] Get spatial resolutions / valid spatial resolutions'
assert.end();
});

tape('[SPATIAL RESOLUTIONS] Get spatial resolutions / valid spatial resolutions with weight 0.5', function(assert) {
tape('[SPATIAL RESOLUTIONS] Get spatial resolutions / valid spatial resolutions with weight 0.25', function(assert) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious @isiyu - why did this test change?

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 changed the description - it said it was with weight 0.5 but the call below on line 418 was using weight 0.25

Copy link
Contributor

Choose a reason for hiding this comment

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

💭 ... 👍

tests for if pixel resolution is over and under getValidSpatialResolutions threshold
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling 88cd7d7 on moremaxzoom into 805d115 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling fc48241 on moremaxzoom into 805d115 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling f19539c on moremaxzoom into 805d115 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling 3b7b6ee on moremaxzoom into 805d115 on master.

@@ -87,7 +87,7 @@ module.exports.getUnitType = function(srs) {

module.exports.getSpatialResolutions = function() {
var circumference = 40075000;
var zoomLevels = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19];
var zoomLevels = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not bump to z21?

Copy link
Contributor

Choose a reason for hiding this comment

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

@GretaCB @isiyu maxzoom is zoomLevels.length (https://github.com/mapbox/mapnik-omnivore/blob/moremaxzoom/lib/raster.js#L179), which means that this is 21. That said, previous maxzoom was 20 (not 19, as I think we've assumed / verified on master and on branch setup from modularization a0c4cda), so I think bumping it to 21 (maxzoom of 22 - 2 more zooms) would be 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.095% when pulling 02009b0 on moremaxzoom into 805d115 on master.

@isiyu isiyu changed the title [not ready] increases max zoom levels for raster uploads increases max zoom levels for raster uploads Apr 25, 2016
@isiyu isiyu merged commit c94b7c9 into master Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants