Conversation
polylabel.js
Outdated
|
|
||
| return [bestCell.x, bestCell.y]; | ||
| var poleOfInaccessibility = [bestCell.x, bestCell.y]; | ||
| poleOfInaccessibility.distance = bestCell.d || 0; |
There was a problem hiding this comment.
Note: the || 0 here is intended to replace occurances of -0 with 0. This works because -0 is falsy. Open to suggestions for alternative approaches here - this just seemed the most straightforward and compact, but perhaps a sacrifice of readability.
There was a problem hiding this comment.
we could clarify this earlier in polylabel.js#L110:
return minDistSq && (inside ? 1 : -1) * Math.sqrt(minDistSq);
There was a problem hiding this comment.
Ah I see, maybe something like
return minDistSq ? (inside ? 1 : -1) * Math.sqrt(minDistSq): 0;
There was a problem hiding this comment.
Or maybe more explicit:
return minDistSq !== 0 ? (inside ? 1 : -1) * Math.sqrt(minDistSq) : 0;
There was a problem hiding this comment.
Or this variant might be most readable:
return minDistSq === 0 ? 0 : (inside ? 1 : -1) * Math.sqrt(minDistSq);
polylabel.js
Outdated
|
|
||
| return [bestCell.x, bestCell.y]; | ||
| var poleOfInaccessibility = [bestCell.x, bestCell.y]; | ||
| poleOfInaccessibility.distance = bestCell.d || 0; |
There was a problem hiding this comment.
we could clarify this earlier in polylabel.js#L110:
return minDistSq && (inside ? 1 : -1) * Math.sqrt(minDistSq);
|
Thank you @Fil for reviewing! I've made one more commit to address your idea. Requesting re-review please. |
|
Hooray!!! Thank you @mourner . |
|
@mourner Might it be possible to cut a new NPM release with this change? Thank you. |
|
Published as https://www.npmjs.com/package/@datavis-tech/polylabel for now. Will mark that as deprecated when a new version of this upstream package comes out. |
This was added to library in June 2020 -- see mapbox/polylabel#61
* Adding polylabel 'distance' return field This was added to library in June 2020 -- see mapbox/polylabel#61 * Updating version number as well * Whoops got confused between version number in two different forks of the lib
…Typed#65352) * Adding polylabel 'distance' return field This was added to library in June 2020 -- see mapbox/polylabel#61 * Updating version number as well * Whoops got confused between version number in two different forks of the lib
3 participants
This PR is a proposed solution for #2.
A fresh take on #14.
Summary of changes:
distanceproperty to returned values.0, not-0, for degenerate cases.distance(never getsundefined).var, andObject.assignin tests instead of rest/spread).Kindly requesting review @mourner @Fil . Feedback welcome!
🙏