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

Adjust geometry-type expression to return Point, LineString or Polygon? #965

Closed
louwers opened this issue Jan 9, 2025 · 12 comments
Closed

Comments

@louwers
Copy link
Collaborator

louwers commented Jan 9, 2025

Historically, in both MapLibre GL JS up to v4 and MapLibre Native, contrary to what the style specification specifies, the geometry-type expression returned Point, LineString or Polygon. This was reported in maplibre/maplibre-gl-js#3516 as a bug, fixed in #519 and released as a breaking change in MapLibre GL JS 5.0.0.

We had a discussion about this topic on Slack and during today's TSC Meeting. I am creating this issue, because I feel we haven't reached a consensus on this topic yet. To add some more context to the discussion I tried the styles of some MapLibre sponsors. Most have issues.

Jawg Streets

AWS Open

image image

Radar Maps

image image

Facebook Light

Image Image

Obligatory XKCD

image
@louwers
Copy link
Collaborator Author

louwers commented Jan 9, 2025

In the TSC Meeting of February 2024 this change was discussed. We thought that most styles used $type anyway, so it would not be such a big deal to change it to align it with the spec.

In retrospect, with styles from all MapLibre sponsors I tried breaking in subtle (or not so subtle) ways, that does not seem to be the case.

I would be in favor of reverting this change, cutting another release, coming up with another solution for 5.1.0.

@hyperknot
Copy link
Contributor

hyperknot commented Jan 9, 2025

First of all, I'm sorry for entering the room just before celebration of the amazing release of 5.0.0. Adding Globe view is indeed a huge accomplishment, and I didn't want to ruin the mood!

I'm just feeling that this voting back then probably didn't represent the belief of the wider MapLibre ecosystem and community. Actually, it probably didn't represent even the core team's view, had it been clear that AWS maps, Facebook maps, etc. would all will be broken with one option.

I propose one solution:

  • Revert the geom-type changes in 5.0.1 and let the world celebrate the globe view with backwards compatibility.
  • Take our time and implement simple-geom-type or similar in 5.1.0.

I made a PR for the fix, by doing an interactive rebase: #966
I tested that PR on AWS maps and it renders it correctly.

@hyperknot
Copy link
Contributor

We thought that most styles used $type anyway, so it would not be such a big deal to change it to align it with the spec.

About $type, I looked into it. What we need to know about $type is that no one was using it (who uses linting), as gl-style-migrate removes it.

Version <=20 turns it into single types:

Image

21, 22 turns it into double types.

Image

In conclusion, anyone who uses the linting tools is 100% sure not to have $type in their stylesheet.

@hyperknot
Copy link
Contributor

I tested Maputnik, it also doesn't allow using $type in some expressions.

Image

@HarelM
Copy link
Collaborator

HarelM commented Jan 9, 2025

Versions 21 and 22 were released late October 2024 in order to support the geometry expression change of maplibre v5.
$type can't be used with "newer" expression syntax.

@hyperknot
Copy link
Contributor

This is what I wrote: "gl-style-migrate removes it"

What is wrong about it? It removes the "$type" from the styles, both old and new versions. So no one can have "$type" in a stylesheet if they used gl-style-migrate.

@philipko100
Copy link

Hi MapLibre team, I am coming from the Amazon Location Service team. We have had multiple customers raising this rendering issue with AWS Maps using MapLibre v5.x. Is there a ETA on the fix? Could we rollback the backward incompatible change?

@hy9be
Copy link

hy9be commented Jan 9, 2025

Hi MapLibre team, I am coming from the Amazon Location Service team. We have had multiple customers raising this rendering issue with AWS Maps using MapLibre v5.x. Is there a ETA on the fix? Could we rollback the backward incompatible change?

@philipko100 Just to clarify, the breaking change was a conscious decision instead of a bug. So technically there is no "fix". From semantic versioning perspective, v5.x is a major version bump. Including incompatible API in a new major version is a common practice.

On the other hand, I am sorry to hear that this change has impacted your customers. To help the community understand the impact and decide the next steps, could you please provide more information without sharing any sensitive or confidential data? It will be also great if you guys can make a suggestion/pull request from your perspective.

@philipko100
Copy link

Hi @hy9be, thanks for the context. I understand that a rollback is not the preferred way to proceed as this is a major version bump. The issue was that our style descriptors that customers are using no longer became compatible with MapLibre GL v5 due to the geometry-type polygon change. For now, we are exploring the option to update our style descriptors to become comptaible with gl v5. I assume that updating our style descriptors to be compatible with geometry-type polgon of gl v5 will still keep the compatibility of v4 and v3. Could you kindly confirm this is the case?

@1ec5
Copy link
Contributor

1ec5 commented Jan 9, 2025

Is the style specification covered by the semantic versioning guarantee of MapLibre GL JS or the maplibre-gl-style-spec package? Where does that leave MapLibre Native?

Since there’s been some speculation: the original Mapbox GL team decided in 2018 to freeze the style specification at v8 and avoid any breaking changes to the specification for the foreseeable future. A lot of good ideas for breaking changes were collected in mapbox/mapbox-gl-js#6584 for a breaking v9 release someday in the distant, unimaginable future. In the meantime, new layer properties would be purely additive, and any obsolete properties or combinations would simply be ignored rather than explicitly deprecated or removed. The closest thing to a breaking change has been the removal of the undocumented ref syntax, or perhaps that the {token} syntax is mutually exclusive of expressions on some platforms. Some platforms started marking some APIs as “experimental”, but this approach was never adopted by the style specification itself: mapbox/mapbox-gl-js#4171.

Before the fork, Mapbox also noticed that geometry-type wasn’t returning multi-types from vector sources, only GeoJSON sources: mapbox/mapbox-gl-js#7699. Their solution, post-fork, was to document this behavior and save any change for style specification v9. They rejected the idea of changing the behavior in the context of v8 styles, even in a major version bump of GL JS.

There were many reasons for these decisions, but the one that’s probably most relevant to MapLibre is compatibility with older styles. By analogy, the major version of a Web browser or HTML rendering library has little bearing on whether the browser can still view HTML 3.2 authored many years ago in FrontPage. gl-style-migrate is just one way to upgrade a style from legacy functions and filters, and of course there’s no guarantee it has been upgraded anyways.

/ref #536 (reply in thread) #214 (comment)

@prusswan
Copy link

I understand @hyperknot's predicament, even though I don't use the lint tools and pretty much relied on existing styles and only modified them as needed. Regardless of the outcome, we need some clarity on future usage of $type, e.g.

  1. it does not care if the geometry is single or multi, so Point/MultiPoint means the same thing
  2. it is a special/legacy keyword that cannot be used in certain context/expressions, so use with caution

[Disclaimer: these points are what I understood from the slack comments and may not be entirely correct]

@louwers
Copy link
Collaborator Author

louwers commented Jan 12, 2025

To avoid further disruption, keep backward compatibility with styles that use this expression and give people a smoother transition path to MapLibre GL JS v5, we decided to reverted this change to match the old behavior. I hope that this is an acceptable outcome for everyone, especially to those that contributed to the past discussion and development of #519. Let's open a new issue so we can tackle the aims of that PR (which was reverted) in a backward-compatible way.

I shared an idea to create end-to-end tests that use real styles in the MapLibre GL JS repo, which I think will be helpful to gauge the impact of changes on real styles in the future.

@louwers louwers closed this as completed Jan 12, 2025
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

No branches or pull requests

7 participants