search

keplergl / kepler.gl

Kepler.gl is a powerful open source geospatial analysis tool for large-scale data sets.
http://kepler.gl
MIT License
10.11k stars 1.71k forks source link

[Bug] Fix custom map style input #2564

Open akre54 opened 2 months ago

akre54 commented 2 months ago

Fix for #2560 and #2458: custom map style urls aren't being loaded.

Quoting from #2560:

Looks like the issue (or one issue) is that the confirmButton in the AddMapStyle modal is always disabled. It's checking for mapStyle.inputStyle.style which is always null since the input onChange listener only sets a url and not a style property. Also addCustomMapStyleUpdater is checking for state.inputStyle.id which is also null. Setting any id from the onChange handler seems to fix the issue.

The icon is also broken. The getStyleImageIcon called from inputMapStyleUpdater is returning a Mapbox url even for non-mapbox basemaps. For instance, inputting the example url "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json" gives me "https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl-style/style.json/static/-122.3391,37.7922,9,0,0/400x300?access_token={snip}&logo=false&attribution=false". I'm not sure what the best fix for that would be.

netlify[bot] commented 2 months ago

Deploy Preview for keplergl2 failed.

Name Link
Latest commit ff892b3851ebc4978a9b4542c624b43d82539052
Latest deploy log https://app.netlify.com/sites/keplergl2/deploys/663303ff5a5e8a00087f1ce5
ibgreen commented 2 months ago

The icon is also broken. The getStyleImageIcon called from inputMapStyleUpdater is returning a Mapbox url even for non-mapbox basemaps. For instance, inputting the example url "https://basemaps.cartocdn.com/gl/positron-gl-style/style.json" gives me "https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl-style/style.json/static/-122.3391,37.7922,9,0,0/400x300?access_token={snip}&logo=false&attribution=false". I'm not sure what the best fix for that would be.

Looking at your URL it seems the style is expected to be a name not a URL (see the double https in https://api.mapbox.com/styles/v1/https://basemaps.cartocdn.com/gl/positron-gl).

So while this PR may unbreak some thing, it is probably not a complete solution.

@ilyabo @macrigiuseppe Any thoughts?

akre54 commented 2 months ago

I can confirm that the main issue is still broken with just a mapbox url scheme:

https://github.com/keplergl/kepler.gl/assets/931368/23aa7a69-2186-40f8-a31a-37af3aac941f

akre54 commented 2 months ago

With this fix, the mapbox:// url scheme still doesn't work. Looks like the only two places that check for a mapbox url scheme are isStyleUsingMapboxTiles (called to determine attribution in map-container.tsx) and getStyleDownloadUrl (called from getLoadMapStyleTasks)

https://github.com/keplergl/kepler.gl/assets/931368/faf578e9-c335-4c82-ac4e-c9b093d16178

ilyabo commented 2 months ago

Maplibre dropped the support for mapbox:// style URLs. To work around this we would probably have to load the basemap library dynamically. Maybe we can add a switch in this dialog to choose from Maplibre and Mapbox. Or revert to Mapbox. @ibgreen what do you think?

ibgreen commented 2 months ago

I don't think that reverting to mapbox is what we want to do. Supporting both would be an option, though that is a feature that someone needs to implement.

One option is to remove the map style support and the dialog.

How important is this feature? Are we breaking a important workflows here?

akre54 commented 2 months ago

Would it be possible to simply rewrite the urls? Either in Kepler or in Maplibre? This feels like a pretty useful feature

Or at the very least include a warning message if you try to use a mapbox:// url scheme? This feels unexpected

ilyabo commented 2 months ago

I suspect that newer mapbox styles would not work correctly with maplibre. I have experienced issues when trying to use newer styles with mapbox@1.

akre54 commented 2 months ago

I can look into the icon import. It feels odd that the importer would assume that all urls come from mapbox, but it's not clear to me that Carto, for instance, returns a unique icon for all of its styles, or that every other style provider is required to return one as well. It seems to me like this would be better if it just used a generic icon, or black square.

I do think it's fine for mapbox:// urls to be broken so long as basic style.jsons still work. What do you think?

chrisgervang commented 2 months ago

It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?

akre54 commented 2 months ago

Maybe not the url protocol but it's the most commonly used map style format. What's the alternative?

And how have they diverged? The functionality to load mapbox V1 styles works after a bugfix. I don't feel like having a preview icon is enough reason to break this workflow. What do you think?

ibgreen commented 2 months ago

Apologies that this is taking time, the basemap support in kepler clearly lacks a decisive owner right now. Together we all have parts of the answer, but the risk is that this thread could go on for a long time unless we decide on something. Following up in the issue.

akre54 commented 2 months ago

Yeah totally agree. I don't want to bike shed this any more than necessary. I agree there needs to be a better default for the icon. If I come up a patch to make this black or a basic color or something would that be acceptable?

ibgreen commented 2 months ago

come up a patch to make this black or a basic color or something would that be acceptable?

Yes let's land something reasonable.

birkskyum commented 1 month ago

It doesn't really make sense to continue supporting Mapbox styles (or their URLs) unless the actual Mapbox library is added as an alternative option, right?

I think this is right. The reason mapbox:// was removed from maplibre was not only a matter of compatibility, but also a license compliance concern. This change in the mapbox style spec license saying:

The software and files in this repository (collectively, "Software") are licensed under the Mapbox TOS for use only with the relevant Mapbox product(s) listed at www.mapbox.com/pricing.

Looking at the www.mapbox.com/pricing etc. it seemed risky to interpret that in any other way than "mapbox styles coming from the mapbox api will be under latest version of this license, and are thus only allowed to be rendered with a mapbox renderer".

akre54 commented 1 month ago

I added a temporary fix using the NO_BASEMAP_ICON for custom map styles and removing the logic for isValidStyleUrl which checked for mapbox styles. I'm sure this can be changed in the future but for now this fixes the major issues.

birkskyum commented 1 month ago

seems like ci is failing on the "should set the inputStyle" test

akre54 commented 2 weeks ago

@birkskyum oddly tests are passing for me locally. Not sure what's happening on CI?

Screenshot 2024-06-12 at 8 22 27 PM
birkskyum commented 2 weeks ago

Weird,. Maybe the ci has different node version, or OS. What's your environment like?

akre54 commented 2 weeks ago

I'm on Mac node v20.9.0. That test is a unit test that isn't using a Chrome instance, so I'm not sure why it would be massively different. Are you able to run it locally?

birkskyum commented 2 weeks ago

No, I see it fail locally too, with yarn test-node-debug and node 18 (it's the version used in the CI)

izi-manny commented 2 days ago

Wasn't aware of this PR before creating a new bug report. Just wanted to let you all know that Kepler.gl has been indispensable for our hobby project for geospatial analysis. Adding a custom map style helped in topographic migration analysis, and it's a bummer it didn't work. Would absolutely love to see the fix merged!