search

mapbox / mapnik-omnivore

Node module that returns metadata about spatial files.
45 stars 19 forks source link

fix the bug of raster zoom calculate #171

Closed JrontEnd closed 6 years ago

mapsam commented 7 years ago

Hey @JrontEnd thanks for your PR! Do you mind providing an explanation of your changes?

JrontEnd commented 7 years ago

When I upload a raster tile set to server, I got a wrong zoom range of the tile set.This pr is to fix this bug.

mapsam commented 7 years ago

@GretaCB or @flippmoke could you give this an 👀 ? I'm not very familiar with this section of the code.

GretaCB commented 7 years ago

Hey @JrontEnd 👋 , thanks for the PR submission.

I took a peek at the PR locally, but I wasn't unable to replicate the zoom behaviour you described. Any chance you can provide a test raster file that replicates the behaviour you're seeing? Or better yet, add a unit test to this PR that verifies the bug/fix?

springmeyer commented 7 years ago

It looks odd to me that this.details.pixelSize = pixelSize is being set at all within Raster.prototype.getZooms since it is set when the Raster object is created: https://github.com/mapbox/mapnik-omnivore/blob/7cfb58eb4f689e6a6330a05e01e7087f5cfb6cb9/lib/raster.js#L57. So it seems like currently if getZooms is called it overwrites that property. I can't see why that should happen as I assume getZooms is only returning metadata and not modifying anything. So, maybe the change should be to remove the this.details.pixelSize = pixelSize. Agree however that ideally we'd need a testcase that this changes behavior for before proceeding.

mapsam commented 6 years ago

Hi @JrontEnd any luck creating a fixture here?

flippmoke commented 6 years ago

Closing for now as no test case to reproduce, will revisit or open if test case is found.