maplibre / maplibre-navigation-ios

MapLibre Navigation SDK for iOS
Other
29 stars 27 forks source link

Removing Mapbox Road Hardcoding #43

Closed hactar closed 1 month ago

hactar commented 1 month ago

During setting up, maplibre-navigation-ios checks if the mapbox street source is part of the map style. If it is not, it adds the source and creates a new layer. This results in the console throwing network errors as "mapbox://mapbox.mapbox-streets-v7" is not a valid URL in a non mapbox app.

This PR removes this hardcoded check - I guess this made sense when this was only used with mapbox services, but now that people use this with their non mapbox styles, this "hotfixing of the mapstyle" makes no sense.

michaelkirk commented 1 month ago

I agree that it doesn't make sense to link to a mapbox url that the app doesn't even know how to handle.

Instead of removing this logic, is there a possibility of a maplibre hosted style? In theory it seems kind of nice that things could "just work" for library users, but maybe the project isn't interested in the amount of hosting that would imply.

michaelkirk commented 1 month ago

Instead of removing this logic, is there a possibility of a maplibre hosted style?

I thought about this more while working on #45, and personally concluded that injecting default styles in the middle of the code base is probably not the right approach, so I take back my comment.

boldtrn commented 1 month ago

Good catch to remove this code @hactar πŸ‘. While I fully agree that we should remove this Mapbox specific logic here. There is similar usage here as well: https://github.com/maplibre/maplibre-navigation-ios/blob/3eda16937ca0f69f3ef42eb79c5de7f49919e918/MapboxNavigation/NavigationMapView.swift#L993

I am currently wondering if we should make this configurable or just completely remove?

hactar commented 1 month ago

Good catch to remove this code @hactar πŸ‘. While I fully agree that we should remove this Mapbox specific logic here. There is similar usage here as well:

https://github.com/maplibre/maplibre-navigation-ios/blob/3eda16937ca0f69f3ef42eb79c5de7f49919e918/MapboxNavigation/NavigationMapView.swift#L993

I am currently wondering if we should make this configurable or just completely remove?

The one that I found should just be plain removed I think, but the one that you found is an interesting one - it tries to modify the street labels to the countries locale they are in so that they match the signs people might see on streets. That could be a useful feature, but we would need to change this so that we can provide maplibre navigation with the layers that contain the road networks, instead of it trying to find the "mapbox road network". And that layer also needs to support "mul" locale for this to work I guess... So that one should be investigated, but I suggest outside this ticket.

hactar commented 1 month ago

Thanks for working on this. Could you add a Changelog entry for this change?

Done.

hactar commented 1 month ago

@boldtrn the conflict has been resolved, I have no write permissions so you or someone else will have to hit the button ☺️