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

Cheapo WMS, Part II #2612

Closed
wants to merge 4 commits into from
Closed

Cheapo WMS, Part II #2612

wants to merge 4 commits into from

Conversation

bhousel
Copy link
Contributor

@bhousel bhousel commented May 23, 2016

Like #2409 but uses whoots-js library to calculate the bounding box with more correct math.
Also added a wms.html debug page.

cc @tmcw @lucaswoj @mourner

@lucaswoj
Copy link
Contributor

The CI failure should be fixed by #2610. Can you rebase and re-push? Thanks!

@lucaswoj
Copy link
Contributor

lucaswoj commented May 24, 2016

I'm 👍 on merging this pending:

@lucaswoj
Copy link
Contributor

What do you think about namespacing the bbox token behind a wms TileJSON scheme, like our tms scheme? #2565

@bhousel
Copy link
Contributor Author

bhousel commented May 25, 2016

What do you think about namespacing the bbox token behind a wms TileJSON scheme, like our tms scheme?

I thought about that, but worried that's misleading because we are only supporting a very specific subset of wms servers. We'd likely see people opening issues because they try to add wms://something as a source and it doesn't do what they expect.

@tmcw tmcw mentioned this pull request May 26, 2016
@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 2, 2016

My understanding is that {bbox-epsg-3857} only makes sense within the context of wms.

Putting the wms-specific tokens to a wms namespace will increase the self-documenting-ness of the feature and give us maneuvering room to improve wms support in the future.

Our likelihood of getting weird wms support requests is unrelated to the existence of a wms scheme.

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 2, 2016

cc @yhahn @tmcw @jfirebaugh: As the lead TileJSON contributors, what's your feeling on this?

@tmcw
Copy link
Contributor

tmcw commented Jun 2, 2016

I love this PR!

  • I don't think we should add this to TileJSON. WMS already has its own metadata stuff and allowing this to be configured WMS-style is an unnecessary can of worms. Also, WMS is not quite a tiling 'scheme', since it isn't tiles, we're just requiring it as tiles.
  • {bbox-epsg-3857} seems fine to me.

I see why we might want to scope this bigger, but don't feel like we need to or should. I think what we have here is pretty great.

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 2, 2016

Ok! Let's 🚢!

@lucaswoj
Copy link
Contributor

lucaswoj commented Jun 2, 2016

... as soon as I finish the hotfix release for #2657

@bhousel
Copy link
Contributor Author

bhousel commented Jun 3, 2016

Just pushed a few commits to bump to the v2 classless library, and added a test.
I'll merge later today when CircleCI goes green..

We should think about how we want to communicate this feature. Should I document it in style-spec?

@bhousel
Copy link
Contributor Author

bhousel commented Jun 3, 2016

merged as
09c0c96 Add test for {bbox-epsg-3857} token replacement
e793682 Bump to WhooTS v2
2ebe717 Add wms.html debug page
4e9a523 Use whoots-js to calculate EPSG:3857 bounding box

@1ec5
Copy link
Contributor

1ec5 commented Jul 10, 2016

This is being ported to the native libraries in mapbox/mapbox-gl-native#5628.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants