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

Filter can't be applied to type: MultiPolygon #3516

Open
dreadedhamish opened this issue Jan 1, 2024 · 25 comments
Open

Filter can't be applied to type: MultiPolygon #3516

dreadedhamish opened this issue Jan 1, 2024 · 25 comments
Labels
bug Something isn't working good first issue Good for newcomers PR is more than welcomed Extra attention is needed

Comments

@dreadedhamish
Copy link

Perhaps this is a feature request, but it seems like an existing function is incomplete.

The scenario is "Add multiple geometries from one GeoJSON source" (https://maplibre.org/maplibre-gl-js/docs/examples/multiple-geometries/) but where a Feature is a MultiPolygon.

When applying styling using a filter to a Feature that is of type = MultiPolygon:

map.addLayer({ 'id': 'park-boundary', 'type': 'fill', 'source': 'national-park', 'paint': { 'fill-color': '#888888', 'fill-opacity': 0.8 }, 'filter': ['==', '$type', 'MultiPolygon'] });

an error is thrown:

Error: layers.park-boundary.filter[2]: expected one of [Point, LineString, Polygon], "MultiPolygon" found

MultiPolygons are styled correctly when not using the filter.

@HarelM
Copy link
Member

HarelM commented Jan 1, 2024

Have you tried geometry-type?
https://maplibre.org/maplibre-style-spec/expressions/#geometry-type
When in doubt, I recommend going through the above site, it is very elaborate.

@HarelM HarelM added the need more info Further information is requested label Jan 1, 2024
@dreadedhamish
Copy link
Author

dreadedhamish commented Jan 2, 2024

Have you tried geometry-type? https://maplibre.org/maplibre-style-spec/expressions/#geometry-type When in doubt, I recommend going through the above site, it is very elaborate.

Thanks for your help.

Could you help me with expression? I'm a little out of my depth.
I tried:
'filter': ['==', '$type', 'MultiPolygon']
'filter': ["==", ["geometry-type"], "MultiPolygon"]
'filter': ["match", ["geometry-type"], "MultiPolygon", true, false]
None throw and error, and none work.

The geoJSON is loaded like the example in my original post:
map.on('load', () => { map.addSource('national-park', { 'type': 'geojson', 'data': { 'type': 'FeatureCollection', 'features': [ { 'type': 'Feature', 'geometry': {"type": "MultiPolygon", "coordinates":

Temporarily here is the page where I've deployed the code: https://stage2.postcodesandplaces.com/au/boroondara-2/

@HarelM
Copy link
Member

HarelM commented Jan 3, 2024

Hmm...
Seems like geometry-type is also returning a Polygon instead of a MultiPlygon... :-/
https://jsbin.com/cuhikisela/edit?html,output

This looks like a bug as it looks like geojson-vt support multipplygon:
mapbox/geojson-vt#5

The following is the only test that checks geometry-type, which is very limited.

[{}, {"geometry": {"type": "LineString", "coordinates": [[0, 0], [10, 0]]}}]

Feel free to try and tackle this, I hope it won't be complicated...

@HarelM HarelM added bug Something isn't working good first issue Good for newcomers PR is more than welcomed Extra attention is needed and removed need more info Further information is requested labels Jan 3, 2024
@wipfli
Copy link
Member

wipfli commented Jan 10, 2024

@HarelM
Copy link
Member

HarelM commented Jan 10, 2024

It is related, but it seems like a reverse behavior to what's reported here.
Same "area" of the bug but possible a different bug.

@acalcutt
Copy link
Contributor

Do you need to distinguish between MultiPolygon and Polygon? In the past when I was testing with buildings, the filter had to be "Polygon" even though the object was a MultiPolygon $type when i viewed in inspect. I just assumed it was matching without the "Multi", since it does't consider those valid geometry types (like shown here in slack https://github.com/maplibre/maplibre-style-spec/blob/main/src/expression/evaluation_context.ts#L6 )

@zstadler
Copy link
Contributor

zstadler commented Jan 12, 2024

The style spec contains two means to access the geometry type of an element, each has a different set of possible velues:

  1. ["geometry-type"] Gets the feature's geometry type: Point, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon.
  2. "$type": the feature type. This key may be used with the "==","!=", "in", and "!in" operators. Possible values are "Point", "LineString", and "Polygon".

The values returned by "$type" are closely related to the basic encoding of geometries in the MVT format.

On the other hand, the MVT format spec also says that a geometry MUST interpreted as a multi* geometry when the number of MoveTo commands is greater that 1. Apparently, this is not part of the ["geometry-type"] implementation and therefore it never returns any of the Multi* values.

Note that once the ["geometry-type"] implementation does return Multi* values, the implementation of the "==","!=", "in", and "!in" operators that use $type should also be updated accordingly. For example, ["==", $type, "LineString"] should be implemented as ["in", ["geometry-type"], "LineString", "MultiLineString"] rather than ["==", ["geometry-type"], "LineString"] as currently done by gl-style-migrate.

@sbachinin
Copy link
Collaborator

I've done some research into the problem.

  1. It's really complicated. A complete solution will be complex and risky and hardly worthwhile.
  2. I see a simple fix that will greatly reduce the scope of the problem.

When filter is applied to features, it's applied to individual polygons; Multipolygon itself doesn't live to this moment. That's because of how geojson-vt works.

Geojson-vt supports Multipolygon (MP) but in a very limited way. Basically it just flattens MP to individual Polygons (Ps).

When filter is applied to features, they are retrieved from tiles already generated by geojson-vt. (geojson_worker_source:73). It returns only bare Ps. Then filter compares them to desired geometry_type ("MP") and it results in false.

Actually geojson-vt gives a clue about an original MP. Some solution can be built around this knowledge. But passing it down to filter is really painful.
Main obstacle is that we are tied to some types from geojson-vt and MVT and these types do not include MP as a possible "type" of a Feature. (geojson-vt: FeatureTypes, mapbox-vector-tile: VectorTileFeature.type).

Other walkarounds for a problem are also imaginable but none of them look simple and safe.
To build a compex walkaround seems unproductive because (as far as I understand) the entire problem is not that big.

I'd suggest a simple (but partial) fix: replace "MP" filter with "P" filter in the very beginning. It will not solve the problem completely but will solve it in most cases I think.

The good thing is that: there is no error, and all Ps within MPs will be drawn.
Bad: if there are MPs and also independent Ps in one source, there will be no way to say: "draw only those Ps within MPs but hide the independent Ps". This looks like a really tiny problem. (And such filtering is impossible anyway).

I'm not sure where exactly to make this swap. The most convenient place is here, before the validation of a layer (but is it ok to pollute style.ts with such stuff?).
if ('filter' in layerObject && layerObject.filter[2] === 'MultiPolygon') { layerObject.filter[2] = 'Polygon'; }

@HarelM
Copy link
Member

HarelM commented Jan 15, 2024

What's more interesting for me, is to understand if this happens to MPs in MVT.
If this is only an issue with geojson source, then the fix should be in the geojson-vt library and not here.
But if there's a wider issue that also applies to mvt, then it needs to be solved.

@zstadler
Copy link
Contributor

The problem, as far as I understand, is unrelated to GeoJSON. Here is an example where the vector tiles were created from OSM data by OpenMapTiles.

Here is an inner member of an OSM nature reserve boundary relation where ["element-type"] == "Polygon" is shown on the member's geometry, rather than "MultiPolygon "

image

@sbachinin
Copy link
Collaborator

@HarelM, apparently it's a wider issue that also applies to mvt.

Type "MP" gets lost with VT sources too. It gets lost not within Maplibre but supposedly when tiles are generated.

I tried a couple of ways to generate tiles (tippecanoe from geojson, maptiler from osm data) and in both cases MP was stored as P, i.e. the "type" of a feature was renamed from "MP" to "P".
("The POLYGON geometry type encodes a polygon or multipolygon geometry", mvt spec says).

Though the structure of an original MP is preserved. The resulting "P" (ex-MP) feature still contains multiple "lines" that correspond to original constituent Ps. (But not necessarily multiple. Depends on tiles' structure).

On the frontend nothing intersting happens. @mapbox/vector-tile basically just decodes the fetched PBF and makes no assumptions about feature types. I.e., it just takes the hardcoded feature type ("3") from the appropriate index in PBF.

I hope this answers your question. (#3516 (comment)).

Does it mean that this problem should be solved in maplibre?
I think it might be possible (though very complicated) with geoJSON sources.
And I doubt that it's possible for VT sources because it seems that VTs doesn't give enough information to deduce the original feature type.
Anyway to build a real solution would mean working against the overall framework of mvt. Or perhaps there is no framework really, but MPs are just under-supported.

@zstadler
Copy link
Contributor

@sbachinin, @HarelM,

The MVT Spec also says (my emphasis on MUST):

If the command sequence for a POLYGON geometry type includes only a single exterior ring then the geometry MUST be interpreted as a single polygon; otherwise the geometry MUST be interpreted as a multipolygon geometry, wherein each exterior ring signals the beginning of a new polygon. If a polygon has interior rings they MUST be encoded directly after the exterior ring of the polygon to which they belong.

It looks like the interpretation required by the MVT Spec, using a count of external rings, is not performed at any point during the processing of the vector tiles. The numeric values provided are converted to their literal string names.

On the other hand, VectorTileFeature.prototype.toGeoJSON() performs that check by classifying the rings and counting the number of outer rings. Similar treatment is done for MultiPoint and MultiLineString features.

@zstadler
Copy link
Contributor

Here is a proposal to sort this out:

  • ["geometry-type"] will always give the detailed geometry, including the classification of MVT features as "Multi"* geometries.
  • "$type" will always give the simplified geometry by removing any "Multi" prefix of a geometry type.

With this proposal,

  • The two type operators of the style spec will provide their expected values regardless of the feature source - MVT or GeoJSON
  • No changes are required to the style spec
  • maplibre-gl-js will comply with the interpretation required by MVT Spec

Implementation thoughts

  • The evaluation of ["geometry-type"] can be lazy in the sense that the classification of MVT features will be done only once, and the string result of the classification will replace the numeric .type. That will avoid the need for subsequent classifications of the same feature.
  • The evaluation of "$type" will continue to handle numeric .type values and will not trigger their classification. It will only remove any "Multi" prefix from string .type values.

@HarelM
Copy link
Member

HarelM commented Jan 22, 2024

The above proposal seems right to me as it aligns the implantation to the spec.
@louwers can you check what is happening in native for the two filters discussed here?

@zstadler
Copy link
Contributor

zstadler commented Jan 25, 2024

EDIT: Changed the code below to make the map view more informative

I think we have been discussing this issue under an assumption that ["geometry-type"] differentiates between Polygon and MultiPolygon. Now, I'm not sure that ["geometry-type"] will ever produce a "MultiPolygon" value.

@dreadedhamish,
Could you please add the following code as the last layers in a style you're using and see what ["geometry-type"] and "$type" values are placed on the GeoJGON's MultiPolygons of your national-park layer?

,
{
  "id": "geometry-type",
  "type": "symbol",
  "source": "national-park",
  "layout": {
    "text-field": [
      "concat",
      "[\"geometry-type\"]: ",
      ["geometry-type"]
    ],
    "text-anchor": "bottom",
    "text-font": ["Open Sans Regular"],
    "text-max-width": 60,
    "text-allow-overlap": true
  }
},
{
  "id": "polygon-dollar-type",
  "type": "symbol",
  "source": "national-park",
  "filter": ["==", "$type", "Polygon"],
  "layout": {
    "text-field": "[\"==\", \"$type\", \"Polygon\"]",
    "text-anchor": "top",
    "text-font": ["Open Sans Regular"],
    "text-max-width": 60,
    "text-allow-overlap": true
  }
},
{
  "id": "not-polygon-dollar-type",
  "type": "symbol",
  "source": "national-park",
  "filter": ["!=", "$type", "Polygon"],
  "layout": {
    "text-field": "[\"!=\", \"$type\", \"Polygon\"]",
    "text-anchor": "top",
    "text-font": ["Open Sans Regular"],
    "text-max-width": 60,
    "text-allow-overlap": true
  }
}

So far, I was unable to produce an example that shows a MultiPolygon value of ["geometry-type"], even when inlining GeoJSON in the style itself.
https://codepen.io/zstadler/pen/LYajQPx?editors=0010

image

@zstadler
Copy link
Contributor

zstadler commented Feb 1, 2024

I've modified the "Maptiler Basic" style as described above.

When opening this style in Maplibre Maputnik, the Inspect view says the $type of the area is MultyPolygon:

image

On the other hand, the Map view says that both ["geometry-type"] and $type" are Polygon:

image

@zstadler
Copy link
Contributor

zstadler commented Feb 13, 2024

Summary of the approach taken in PR maplibre/maplibre-style-spec#519

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.

@louwers
Copy link
Collaborator

louwers commented Feb 13, 2024

@HarelM The behavior of MapLibre Native is the same. Only Point, LineString and Polygon are returned. A MultiLineString is considered a LineString et cetera. It would be easy to change though, but it would be a breaking change.

@HarelM
Copy link
Member

HarelM commented Feb 14, 2024

@louwers in both cases native returns only non multi types? or does it behave differently for geometry-type and $type?

@HarelM
Copy link
Member

HarelM commented Feb 14, 2024

I don't know if solving a bug that aligns the product to the spec is a major or minor version change, in general, we just released version 4 of maplibre, so if this is considered a breaking change, we'll need to wait with it to version 5...
I'll bring it up in the monthly meeting tonight and see what people think.

@louwers
Copy link
Collaborator

louwers commented Feb 14, 2024

It is the same for both.

enum class FeatureType : uint8_t {
    Unknown = 0,
    Point = 1,
    LineString = 2,
    Polygon = 3
};

struct ToFeatureType {
    FeatureType operator()(const EmptyGeometry &) const { return FeatureType::Unknown; }
    template <class T>
    FeatureType operator()(const Point<T> &) const {
        return FeatureType::Point;
    }
    template <class T>
    FeatureType operator()(const MultiPoint<T> &) const {
        return FeatureType::Point;
    }
    template <class T>
    FeatureType operator()(const LineString<T> &) const {
        return FeatureType::LineString;
    }
    template <class T>
    FeatureType operator()(const MultiLineString<T> &) const {
        return FeatureType::LineString;
    }
    template <class T>
    FeatureType operator()(const Polygon<T> &) const {
        return FeatureType::Polygon;
    }
    template <class T>
    FeatureType operator()(const MultiPolygon<T> &) const {
        return FeatureType::Polygon;
    }
    template <class T>
    FeatureType operator()(const mapbox::geometry::geometry_collection<T> &) const {
        return FeatureType::Unknown;
    }
};

@zstadler
Copy link
Contributor

It looks like the current Maplibre Native implementation above may also be different than the current maplibre-style-spec implementation.

geometryType() {
    return this.feature ? typeof this.feature.type === 'number' ? geometryTypes[this.feature.type] : this.feature.type : null;
}

If this.feature.type is not a number, maplibre-style-spec returns it as-is, without converting a MultiLineString to a LineString et cetera.

@HarelM
Copy link
Member

HarelM commented Feb 15, 2024

Monthly meeting conversation summary:
Since this is a breaking change in terms of how maps might look there are two options to proceed:

  1. Introduce this in a breaking change version (both in native and web)
  2. Keep the current behavior, change the spec accordingly to align with current implementation and propose a new spec function to facilitate for this requirement (multi type filtering).

There is a strong bias towards not breaking the way styles will look, and changing the geometry-type implementation will cause this.
Having said that, it was agreed that $type is probably what is used in most styles and changing geometry-type to align it to the spec is OK.

I have created the following poll to solicitate more feedback:
maplibre/maplibre-style-spec#536

Feel free to participate there.

@wipfli
Copy link
Member

wipfli commented Mar 20, 2024

@HarelM I lost a bit track of this conversation. Maybe you can help me with an example:

Say I have this filter:

[
  "in",
  [
    "geometry-type"
  ],
  [
    "literal",
    [
        "Polygon",
        "MultiPolygon"
    ]
  ]
]

What is the current behavior, what would be the proposed new behavior?

@zstadler
Copy link
Contributor

For this filter, there will be no change in behavior if the "breaking" change would be implemented.

Currently, ["geometry-type"] never returns a "MultiPolygon" value, even when the geometry is a true MultiPolygon - one with multiple outer rings.

zstadler added a commit to zstadler/openmaptiles that referenced this issue 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 issue 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
bug Something isn't working good first issue Good for newcomers PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants