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] Custom Map Style loading failure #2560

Open CalypsoR opened 2 months ago

CalypsoR commented 2 months ago

Describe the bug I am not able to load my Mapbox map style in Kepler.

To Reproduce Steps to reproduce the behavior:

  1. Go to 'Kepler.gl/demo'
  2. Select 'Base Map' tab
  3. Click on 'Add Map style'
  4. Paste style URL 'mapbox://styles/leon-sildano/clrq8rugo001g01qvbzpudmn4'
  5. Add the name 'Locala map'

Expected behavior Add style button should be selectable.

Screenshots

Capture d’écran 2024-04-11 à 15 03 54

Desktop (please complete the following information):

akre54 commented 2 months ago

Confirmed this is reproducible. Here's the stack trace with this url:

       Failed to load resource: net::ERR_FAILED
bundle.js:82 Error: Failed to fetch
    at bundle.js:19:128310
_onEvent @ bundle.js:82
bundle.js:419 Fetch API cannot load mapbox://mapbox.mapbox-streets-v7,examples.0fr72zt8. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
bundle.js:419 Fetch API cannot load mapbox://sprites/examples/cjikt35x83t1z2rnxpdmjs7y7@2x.json. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
bundle.js:419 Fetch API cannot load mapbox://sprites/examples/cjikt35x83t1z2rnxpdmjs7y7@2x.png. URL scheme "mapbox" is not supported.
(anonymous) @ bundle.js:419
3bundle.js:82 Error: Failed to fetch
    at bundle.js:19:128310
_onEvent @ bundle.js:82
ibgreen commented 2 months ago

@ilyabo @birkskyum Some leftovers from the maplibre transition?

birkskyum commented 2 months ago

The bug with the "Add style" button not being clickable actually predates the work on maplibre transition.

This bug was i.e. reported here Nov 29, 2023:

The maplibre transition PR merged and published Dec 19-21, 2023

That said, MapLibre iirc also requires the style to be a normal http(s):// url instead of the mapbox:// - I think we can look up somewhere what the actual url to prepend is if the mapbox api license allow for it.

akre54 commented 2 months ago

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.

Making these changes seems to help:

diff --git a/src/components/src/modal-container.tsx b/src/components/src/modal-container.tsx
index a26e7579..77152824 100644
--- a/src/components/src/modal-container.tsx
+++ b/src/components/src/modal-container.tsx
@@ -434,7 +434,7 @@ export default function ModalContainerFactory(
               onConfirm: this._onAddCustomMapStyle,
               confirmButton: {
                 large: true,
-                disabled: !mapStyle.inputStyle.style,
+                disabled: !mapStyle.inputStyle.url,
                 children: 'modal.button.addStyle'
               }
             };
diff --git a/src/components/src/modals/add-map-style-modal.tsx b/src/components/src/modals/add-map-style-modal.tsx
index f5e27343..72703488 100644
--- a/src/components/src/modals/add-map-style-modal.tsx
+++ b/src/components/src/modals/add-map-style-modal.tsx
@@ -194,7 +196,7 @@ function AddMapStyleModalFactory() {
                 <InputLight
                   type="text"
                   value={inputStyle.url || ''}
-                  onChange={({target: {value}}) => this.props.inputMapStyle({url: value})}
+                  onChange={({target: {value}}) => this.props.inputMapStyle({url: value, id: 'Custom Style
' })}
                   placeholder="e.g. https://basemaps.cartocdn.com/gl/positron-gl-style/style.json"
                 />
               </StyledModalSection>

Of course, the icon is broken, but this appears to be the basic fix.

akre54 commented 2 months ago

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"

ibgreen commented 2 months ago

It doesn't really make sense to continue supporting Mapbox URLs unless the actual Mapbox library is added as an alternative option

Let's decide that we are going to remove the mapbox URL protocol. However, to avoid future confusion, we should do it right.

There are a bunch of references to in docs and code to mapbox:// url protocol. There could also be some screenshots. If we no longer support it, that should be at least superficially cleaned up, in a quick PR.

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?

If we can keep support for V1 styles that would be preferable, but I am still confused as to what is supported in maplibre and what is not.

birkskyum commented 2 months ago

Any mapbox v1 style will still be supported by maplibre, albeit when passed as http://, not mapbox://, because there has yet to be introduced a breaking change in the style spec. The style spec is in general quite stable and changes mainly by addition.

akre54 commented 2 months ago

So then it seems reasonable to not support mapbox:// for base urls, and instead prefer http resources.

I've seen a few cases where assets within map style jsons are loaded via mapbox://, which might necessitate rewriting the urls or dropping support, but I think fixing the loading is probably enough for now. My preference would be for this to happen in maplibre but I understand if / why that's less than desirable. We can revisit if there's enough interest. Hows that sound?