search

openlayers / ol-mapbox-style

Use Mapbox Style objects with OpenLayers
https://unpkg.com/ol-mapbox-style/dist/examples/index.html
BSD 2-Clause "Simplified" License
344 stars 121 forks source link

Patched Mapbox GL Spec #10

Open lukasmartinelli opened 7 years ago

lukasmartinelli commented 7 years ago

@ahocevar Would it make sense to have a patched Mapbox GL spec that only contains the properties and layer types that are supported by ol-mapbox-style?

I would take care of it. It could be in this repo or in another one.

For https://github.com/maputnik/editor this would allow me to only display properties for a OL3 style that can be supported. And in general it would help to specify and document the subset of the Mapbox GL spec that is supported by ol-mapbox-style.

As always - I love this plugin.

ahocevar commented 7 years ago

Are you talking about e.g. https://github.com/mapbox/mapbox-gl-style-spec/blob/mb-pages/reference/v8.json? In general, I like the idea. I'm just wondering what would be the best way to implement it. Ideally, unsupported features would just be commented out, but JSON does not allow comments. On the other hand, if unsupported features are removed from the spec, it will be harder to add them back later. Do you have any thoughts on this?

lukasmartinelli commented 7 years ago

Are you talking about e.g. https://github.com/mapbox/mapbox-gl-style-spec/blob/mb-pages/reference/v8.json?

Yes.

In general, I like the idea. I'm just wondering what would be the best way to implement it. Ideally, unsupported features would just be commented out, but JSON does not allow comments. On the other hand, if unsupported features are removed from the spec, it will be harder to add them back later. Do you have any thoughts on this?

Perhaps fork and add ol-mapbox-style the sdk-support field. This way we only annotate properties that are supported and do not delete/add anything to the spec.

          "sdk-support": {
            "basic functionality": {
              "js": "0.10.0",
              "android": "2.0.1",
              "ios": "2.0.0",
              "macos": "0.1.0"
            }
ahocevar commented 7 years ago

So you mean something like

      "sdk-support": {
        "basic functionality": {
          "js": "0.10.0",
          "android": "2.0.1",
          "ios": "2.0.0",
          "macos": "0.1.0",
          "ol-mapbox-style: "1.0.0"
        }
      }

I agree that doing this is the cleanest way. If you agree, before creating a fork, I'd create an upstream ticket with the suggestion - maybe it can even be added there.

lukasmartinelli commented 7 years ago

I agree that doing this is the cleanest way. If you agree, before creating a fork, I'd create an upstream ticket with the suggestion - maybe it can even be added there.

Good idea, try that. I think the chances are not so big but upstream would be the best way.

ahocevar commented 7 years ago

@lukasmartinelli I think you can create a pull request against https://github.com/mapbox/mapbox-gl-style-spec. I'll take care of the formal requirements once you have submitted it. Just reference the pull request here.

orangemug commented 6 years ago

Hey @ahocevar did you get anywhere with this? I noticed in https://github.com/mapbox/mapbox-gl-js/issues/4170#issuecomment-297946609 you were planning to work on it.

Also congrats on the improvements to ol-mapbox-style I've been updating the dependency in https://github.com/maputnik/editor over the months and the map styles are starting to look really good.

ahocevar commented 6 years ago

@orangemug I still didn't find time to do this, but it's still high on my list.

orangemug commented 6 years ago

Thanks for the response @ahocevar. I really want to get OpenLayers properly supported in Maputnik so anything I can do to help just shout. We've still got a little bit of work to do Maputnik side also, see https://github.com/maputnik/editor/issues?q=is%3Aissue+is%3Aopen+label%3Aopenlayers

Note: I had a look into this over the weekend, I was thinking visual regression type tests might be quite nice. Basically some GeoJSON + styling for each style rule that's in the Mapbox GL style spec.

orangemug commented 10 months ago

Note: I had a look into this over the weekend, I was thinking visual regression type tests might be quite nice. Basically some GeoJSON + styling for each style rule that's in the Mapbox GL style spec.

@ahocevar I'm going to give this a go, see where I can get to. I've started over at https://maparatus.github.io/ol-mapbox-style-spec/ I'm going to build a bunch of test styles to see the visual differences between ol-mapbox-style/maplibre-gl later on maybe we could build this into some automated visual regression tests.

orangemug commented 10 months ago

There are some interesting quirks, for example here I'd argue that ol/ol-mapbox-style does a better job (OpenLayers is on the right)

Screenshot from 2023-12-11 16-59-37

I'm currently unsure if the style-spec come with a caveat for OpenLayers

orangemug commented 10 months ago

I've update https://maparatus.github.io/ol-mapbox-style-spec/ with a bunch more examples. Some notable differences

orangemug commented 10 months ago

Here is how the maplibre compatibility support is going so far, results are in the ol-mapbox-style-spec repo and the spec is at ./src/spec. That page also contains PR and issue links to this repo for various features missing/buggy.

Any feedback welcome (and encouraged)

Key for the results:

Results: