mapbox / node-s2

bindings for s2 in node
88 stars 38 forks source link

Looks like min, max, max_cells only working on getCoverSync and not getCover #89

Closed rhagigi closed 7 years ago

rhagigi commented 7 years ago

When calling:

s2.getCover(s2Cap, {
      type: 'cap',
      min: 16,
      max: 16,
      result_type: 'cellId'
    }, (err, coveringCellIds) => {
     console.log(coveringCellIds.map((cell) => {
    return {
      level: cell.level(),
      string: cell.toString(),
      token: cell.toToken(),
      id: cell.id()
    };
  }));

I get back cells ranging in level from 5 to 16

but when I use

s2.getCoverSync(s2Cap, {
      type: 'cap',
      min: 16,
      max: 16,
      result_type: 'cellId',
      ...options
    })

I get back what I expect, cells all in level 16.

I looked through the code and it seems like when generating a synchronous cover, it directly calls the coverers set_min_level with the value passed in on the options object. For the async cover, it sets the values on an object and then passes that through to the Cover Worker. I know that the values are being passed through because on the OK callback (in the worker), result_type value is used correctly (you can set token as a the result_type and it does change the result type to tokens). So the issue seems to be on the wrapping and unwrapping.

If I had to guess, somehow

    if (coverConfiguration->min_level != std::numeric_limits<int>::min()) {

isn't passing. But honestly unsure.

rhagigi commented 7 years ago

oh -- I think I found the bug.

https://github.com/mapbox/node-s2/blob/master/src/s2.cc#L67

if (coverConfiguration->min_level != std::numeric_limits<int>::min()) {
  coverer.set_min_level(coverConfiguration->level_mod);
}