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

"geometry-type" to identify Multi- features #519

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zstadler
Copy link
Contributor

@zstadler zstadler commented Feb 2, 2024

What does this PR do

The style spec for ["geometry-type"] says it "Gets the feature's geometry type: Point, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon." However the implementation for tile-based geometries was identical to that of $type and the possible values were Point, LineString, Polygon. This also applied to GeoJSON sources in Maplibre-GL-JS because they are converted and stored internally as tile-based geometries.

The PR adds aligns the implementation of ["geometry-type"] with its spec and adds the distinction between Point and MultiPoint, between LineString and MultiLineString, and between Polygon and MultiPolygon for tile-based geometries.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

- Add mapping from geometry-type to $type
- Remove caching to geometry analysis results
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (2009ed9) 92.22% compared to head (e3b7448) 92.21%.
Report is 21 commits behind head on main.

Files Patch % Lines
src/expression/evaluation_context.ts 90.47% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
- Coverage   92.22%   92.21%   -0.02%     
==========================================
  Files         100      100              
  Lines        4195     4241      +46     
  Branches     1202     1216      +14     
==========================================
+ Hits         3869     3911      +42     
- Misses        326      330       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zstadler zstadler changed the title "geometry-type" to identify Milti- features "geometry-type" to identify Multi- features Feb 9, 2024
@zstadler zstadler marked this pull request as ready for review February 9, 2024 22:18
@zstadler
Copy link
Contributor Author

Resolves maplibre/maplibre-gl-js#3516

@wipfli
Copy link
Member

wipfli commented Mar 25, 2024

The result from the poll in #536 is that 8 out of 10 people voted for:

"geometry-type" will be implemented accord to the spec, this will happen only in a breaking change version (both native and web)"

while only 2 out of 10 voted for:

Keep the current behavior (duplicate operators), update the spec accordingly, and introduce a new operator to get "multi"s

I am in favor of the second option with the simple reason that we do not have a good way of informing our users that there has been a breaking style spec change. Up until today, when I as a map designer make a style sheet there have been two options:

  • either the style will render identically in MapLibre GL JS and MapLibre Native and it does not matter which version I use
  • or the version of GL JS or Native will not support a feature I am using and therefore the who rendering will crash.

If we start introducing breaking changes in the style specification we will have to add a third point to this list of options which is:

  • how your map will look like depends on the version of MapLibre you are using

There are plenty of bad things in the MapLibre Style Specification and I would love to through away entirely things like "{name}" and replace it everywhere with "["get", "name"]". But as ugly as the style spec is in many places, it is also an incredible achievement of this project and all the people who worked on it to get a spec that has been without breaking changes over the last 8 years or so. There is a lot of value in stability and I think we should keep following this path.

@HarelM
Copy link
Member

HarelM commented Mar 25, 2024

I generally agree, except that this case is different - $type is used in most cases and there's no good way to get a multi geometry type. I believe that this case makes sense. The community had voted so.
The way to tell users, much like other breaking changes, is in the release notes of the relevant breaking changes version (5.x of malibre-gl in this case probably).
We had other changes that changed how the map looked due to bug fixes, I think this case is very similar in that aspect that it feels more like a bug fix and not an actual spec change...

@wipfli
Copy link
Member

wipfli commented Mar 25, 2024

Thanks for the feedback, @HarelM. I doubt that we actually have a good way of knowing if people use $type more often than geometry-type. But even if it is less often used, it does not mean that changing its behavior is not important.

I remember we did have some other breaking change in the style spec where we fixed something with colors, do you remember what the other breaking changes were?

Fixing bugs in the style spec sounds attractive, but keep in mind how old this toolkit now is and how many people rely on it. It is like with any other old library. People know the bugs and the expect them to be there. If you fix the bugs, they will say why did you change the behavior - the old way was fine for us and now we have extra work because you made the code look better.

Maybe I am too conservative and we can start to be more dynamic with how we handle the spec, I don't know, but I feel like it would be much better for our users to freeze the behavior and leave bugs and bad aesthetics where they are...

@HarelM
Copy link
Member

HarelM commented Mar 25, 2024

They can still do that by not upgrading to the latest and greatest. Color was one thing and text along a line was another. Also collision boxes, which are still a bit broken, will be improved in the coming releases.
I don't think this decision should be taken lightly, and I believe we have made the relevant steps to ensure this is not a whim.

@zstadler
Copy link
Contributor Author

None of the styles available in Maputnik uses ["geometry-type"]. I was looking for styles on maplibre.org home page, and found none.

zstadler added a commit to zstadler/openmaptiles that referenced this pull request Mar 25, 2024
Related to maplibre/maplibre-style-spec#519 and maplibre/maplibre-gl-js#3516

According to the Style Spec, `["geometry-type"]` is expected to distinguish between LineString and MultiLineString, while `"$type"` does not. For the transportation layer, the distinction is harmful.

All of OMT style layers, except these two, use `"$type"`.
TomPohys pushed a commit to openmaptiles/openmaptiles that referenced this pull request Mar 28, 2024
Related to maplibre/maplibre-style-spec#519 and maplibre/maplibre-gl-js#3516

According to the Style Spec, `["geometry-type"]` is expected to distinguish between LineString and MultiLineString, while `"$type"` does not. For the transportation layer, the distinction is harmful.

All of OMT style layers, except these two, use `"$type"`.
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

Successfully merging this pull request may close these issues.

None yet

4 participants