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

Float zooms #15

Merged
merged 1 commit into from Jul 26, 2018

Conversation

@TeaSeaLancs
Copy link
Contributor

commented Jan 8, 2018

This (optionally) removes the requirement for integer zooms to be outputted from viewport.

This also updates one test which appears to be failing on my box, even on master. I wonder whether it’s due to node updates?

@timiyay

This comment has been minimized.

Copy link
Contributor

commented May 26, 2018

@TeaSeaLancs The failing test was triggered for me when developing with NodeJS 8.

I originally wrote that test, and the float precision was unbounded. I guess NodeJS 8+ handles floats differently?

At any rate, I fixed this in #18, by rounding the coordinates in the test to 8 decimal places, which roughly equates to 1mm accuracy in much of the world.

If you rebase your branch off the latest master, that test will pass.

@MichielDeMey

This comment has been minimized.

Copy link

commented Jul 4, 2018

Hi! Currently using @TeaSeaLancs's branch for float support for the zoom level.
Care to rebase to master to fix the failing tests and get some official support on this?
Otherwise, I can take a stab at the rebase and open another PR based on this one.

@TeaSeaLancs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

Hi @MichielDeMey, sorry, this completely slipped off my radar.

I'll try and get it all rebased tomorrow.

@TeaSeaLancs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

So turns out this work was already done by @timiyay in another branch which was merged. So my job is done here ;-) @MichielDeMey upgrade and you should be fine.

@TeaSeaLancs TeaSeaLancs closed this Jul 5, 2018

@MichielDeMey

This comment has been minimized.

Copy link

commented Jul 5, 2018

Are you referring to #19?
As far as I can tell this just adds support to the bounds function, but not the viewport function.
Or am I missing something? 🤔

@TeaSeaLancs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

Well, looks like I don't know how to read. One sec.

@TeaSeaLancs TeaSeaLancs reopened this Jul 5, 2018

@TeaSeaLancs TeaSeaLancs force-pushed the Allinthedata:float-zooms branch from bd034e5 to 7524fdd Jul 5, 2018

@TeaSeaLancs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

Rebased, ready for continued QC.

@MichielDeMey

This comment has been minimized.

Copy link

commented Jul 16, 2018

LGTM, function signature is backwards compatible which is nice.
@timiyay, @tmcw any outstanding issues? This provides support for map rendering engines supporting floating zoom levels (e.g. Mapbox GL)

@chrusart

This comment has been minimized.

Copy link

commented Jul 25, 2018

Please merge and release :)

@timiyay

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@MichielDeMey I think @tmcw is no longer working on this lib.

@mapsam helped me out last time, so he might know if any Mapbox people want to review before merge.

The code LGTM. It's hard to make readable tests for this maths stuff, but if the past test suite is still passing, and your new stuff is handling float zooms, then it should be sweet.

@MichielDeMey

This comment has been minimized.

Copy link

commented Jul 26, 2018

Yeah, I noticed this repo was a bit stale.

Meanwhile, we've been using this additional code for the past month or so in production. (Mapbox GL)
We haven't encountered an issue yet.

@mapsam

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

@TeaSeaLancs hi there! Apologies for the lack of communication here. This looks really great and I think should be merged in. I'll make a commit to master once this is merged updating docs & changelog and cut a 0.4.0 release.

Cheers!

@mapsam mapsam merged commit 72205f1 into mapbox:master Jul 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mapsam

This comment has been minimized.

Copy link
Member

commented Jul 26, 2018

This has been published!

npm install @mapbox/geo-viewport@0.4.0
@MichielDeMey

This comment has been minimized.

Copy link

commented Jul 26, 2018

Thanks @mapsam!

@TeaSeaLancs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

@mapsam Hey, absolutely no worries :) I did this work as part of a project and as it turns out I actually went another direction with it anyway, but glad to see the work's made it in. Yay contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.