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

GeoJSON layers not behaving as expected when using ["geometry-type"] #5103

Closed
dschep opened this issue Nov 24, 2024 · 7 comments · Fixed by #5132
Closed

GeoJSON layers not behaving as expected when using ["geometry-type"] #5103

dschep opened this issue Nov 24, 2024 · 7 comments · Fixed by #5132
Labels
need more info Further information is requested

Comments

@dschep
Copy link
Contributor

dschep commented Nov 24, 2024

When looking into the impact of maplibre/maplibre-style-spec#519 on Ultra which makes heavy use of GeoJSON sources, I discovered that geojson-vt transforms all single geometry types into multis. This manifests as a bug in MapLibre GL JS.

maplibre-gl-js version: v5 & v4, probably all?

browser: All

Steps to Trigger Behavior

  1. Make a GeoJSON source containing MultiPoints and Points (applicable to [Multi]LineString and [Multi]Polygon geometries too)
  2. Add a layer using ["geometry-type"] to style Points and MultiPoints differently
  3. In v4 all [Multi]Points are rendered as points and in v5 all re rendered as MultiPoints

Link to Demonstration

Expected Behavior

Image

Actual Behavior

v4

Image

v5

Image

@HarelM
Copy link
Collaborator

HarelM commented Nov 24, 2024

I've checked this locally and it looks good.
Note that it looks like geojson-vt is flattening the multi stuff in case they are not needed (probably to improve performance).
If your multi point is more than one point, it should work, I don't know how to categorize this honestly in terms of what should be the right behavior if you supply a single point for multipoint, single line for multiline etc...
See here:
https://jsbin.com/vuguyuvili/3/edit?html,output

Image

@HarelM HarelM added the need more info Further information is requested label Nov 24, 2024
@dschep
Copy link
Contributor Author

dschep commented Nov 25, 2024

OK.. Updated my example to also add labels with the value of ["geometry-type"]. It prints out Point for both the Point and MultiPoint. IMO this makes it even more of a bug! ["geometry-type"] doesn't even behave consistently for the same feature type (when filtering it behaves as if `["geometry-type"] returns MultiPoint for both MultiPoint & Point, but both are labeled with Point).

Image

@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

Your example still has a multipoint with one point.
This is what I think causes your confusion (I fully understand why this can be confusing).
If you change the content of the geometry of multipoint to be more than one point you'll see that it behaves as expected.
As I said above, I guess that the geojson conversion is optimizing to look at the content of the geometry.

@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

I see your point now, the filter is not acting as it should be.
The text field is working as expected though as far as I can tell...
Here's the updated jsbin:
https://jsbin.com/nuvuvovusi/2/edit?html,output

@HarelM
Copy link
Collaborator

HarelM commented Nov 25, 2024

I think the bug is here:
https://github.com/maplibre/maplibre-style-spec/blob/40f6fe71dcb0b24527ad45e88c2648d3624c8d4e/src/expression/compound_expression.ts#L464C27-L464C47

Filters are converted to special use case for some reason (probably an historic one... :-/)

@HarelM
Copy link
Collaborator

HarelM commented Nov 28, 2024

@dschep if you have a way to test my fix it would be great to see if I didn't miss anything (this is in the style spec repo right now).
See here:

@dschep
Copy link
Contributor Author

dschep commented Nov 28, 2024

I'll test as soon as I can, but that probably won't be today 🦃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need more info Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants