Filter can't be applied to type: MultiPolygon

dreadedhamish commented 9 months ago

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 commented 9 months ago

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.

dreadedhamish commented 9 months ago

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 commented 9 months ago

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: https://github.com/mapbox/geojson-vt/issues/5

The following is the only test that checks geometry-type, which is very limited. https://github.com/maplibre/maplibre-gl-js/blob/39003c93fcef8b6cf06520e0de391042ca1afbb1/test/integration/expression/tests/geometry-type/basic/test.json#L4

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

wipfli commented 9 months ago

HarelM commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

["geometry-type"] Gets the feature's geometry type: Point, MultiPoint, LineString, MultiLineString, Polygon, MultiPolygon.
"$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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 "

sbachinin commented 9 months ago

@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. (https://github.com/maplibre/maplibre-gl-js/issues/3516#issuecomment-1892341938).

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 commented 9 months ago

@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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

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

zstadler commented 8 months ago

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:

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

zstadler commented 8 months ago

Summary of the approach taken in PR https://github.com/maplibre/maplibre-style-spec/pull/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 commented 8 months ago

@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 commented 8 months ago

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

HarelM commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

Introduce this in a breaking change version (both in native and web)
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: https://github.com/maplibre/maplibre-style-spec/discussions/536

Feel free to participate there.

wipfli commented 7 months ago

@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 commented 7 months ago

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.

maplibre / maplibre-gl-js

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

Summary of the approach taken in PR https://github.com/maplibre/maplibre-style-spec/pull/519