Use getFeaturesInExtent

[ahocevar]

Fixes #13194.

[github-actions]

📦 Preview the examples and docs from this branch here: https://deploy-preview-13195--ol-site.netlify.app/.

[mike-000]

forEachFeatureIntersectingExtent() is not the same as forEachFeatureInExtent(). If you select a region of empty North Atlantic between Canada and France you do not expect to select Russia.

[ahocevar]

The difference is not rocket science either, so I added another commit with a filter for actual geometry intersection.

[EvertEt]

2 small questions from porting this to TS:

1. Should we guard for getGeometry returning undefined? When can this happen?
2. The filter added in the second commit uses intersectsExtent which again TS warns about not being able to work with VectorTile as it does not exist on RenderFeature.

Thanks already!

[ahocevar]

Should we guard for getGeometry returning undefined? When can this happen?

In this case you won't get features without a geometry, because those would not be returned as result of getFeaturesInExtent(). But in general, features without a geometry are valid.

The filter added in the second commit uses intersectsExtent which again TS warns about not being able to work with VectorTile as it does not exist on RenderFeature.

In this case you'd have to cast to ol/Feature.

[EvertEt]

In this case you won't get features without a geometry, because those would not be returned as result of getFeaturesInExtent(). But in general, features without a geometry are valid.

👍

The filter added in the second commit uses intersectsExtent which again TS warns about not being able to work with VectorTile as it does not exist on RenderFeature.

In this case you'd have to cast to ol/Feature.

If we are allowed to cast, there will never be RenderFeatures returned?
I expected the OlFeature to be from VectorLayer and OlRenderFeature from VectorTileLayer? edit: I was thinking of a TS cast - ignore.

I do notice another problem with the box select and VectorTileLayer: duplicate selections. Maybe per tile?
Should we try to make a separate example for this or use both types of layers in the example possibly?

[mike-000]

As with the VT selection example there should to be an idProperty to identify features split across tiles and zoom levels https://codesandbox.io/s/vector-tile-selection-forked-c5jvn?file=/main.js You will only get the type of feature specified by featureClass in the MVT options. As getFeaturesInExtent allows either to be returned casting may be unavoidable.

[EvertEt]

In this case you'd have to cast to ol/Feature.

Do you mean a TS cast or using featureClass of MVT?

[ahocevar]

@EvertEt a TS cast in this case won't help, because ol/render/Feature does not implement intersectsExtent(). So yes, the easiest way to fix this is configure the MVT format with featureClass: Feature.

[EvertEt]

Thanks you for answering The decrease in render performance is something we'd like to avoid.
Are there docs explaining which cases would be wrong without the filter or in the oblique case? Would there be a difference for Points or only LineString and Polygon?

[mike-000]

Without the filter you are simply testing if the extents intersect - for example the box intersects the extents of the geometries of USA and Mexico, but not the geometries:

[ahocevar]

@EvertEt For points, the result would be the same. For other cases, I think it could make sense to have a fromRenderFeature function on ol/Feature, to create a Feature from a render feature.

[EvertEt]

Would this fromRenderFeature then be able to be used after the selection happened to construct ol/Feature so we can check the intersects exactly?
If so, would you be able to point me to how this function would work, I could take a stab at it.

Combining the above with an idProperty could then give the same behaviour for both Vector and vectorTile, correct? With minimal performance hit since only the selected features are converted and not even displayed?

Thank you both

[ahocevar]

Exactly. The fromRenderFeature() function could look like this:

function fromRenderFeature(renderFeature) {
  const geometryType = renderFeature.getType();
  let geometry;
  switch (geometryType) {
    case GeometryType.POINT:
      geometry = new Point(renderFeature.getFlatCoordinates());
      break;
    // ...
    case GeometryType.POLYGON:
      geometry = new Polygon(renderFeature.getFlatCoordinates(), GeometryLayout.XY, renderFeature.getEnds());
      break;
    // ...
    default:
  }
  const feature = new Feature({
    ...renderFeature.getProperties(),
    geometry
  });
  feature.setId(renderFeature.getId());
  return feature;
}

You would have to handle the cases for all supported geometry types.

[EvertEt]

Interesting, I will have a look at this, thank you.
If both Feature and RenderFeature are able to be converted from each other, would it make sense to merge them?
Or would the Feature generated by fromRenderFeature be with a simplified geometry or missing holes or something alike?

[ahocevar]

@EvertEt ol/render/Feature does not have events, and does not have the overhead of Geometry objects and Object properties. It is simply optimised for fast creation and low memory footprint. The geometry of an ol/render/Feature uses a flat coordinates representation, just like internally in Geometry objects, and supports all geometry types that the MVT format supports (including polygons with multiple rings). As you saw yourself, this is good for performance, and that was the reason why we created ol/render/Feature.

In future major versions, we might be getting rid of both ol/Feature and ol/render/Feature, in favor of a pure GeoJSON representation.

[EvertEt]

I appreciate your explanation @ahocevar , thanks!
I am however having a difficult time understanding the internal OL representations with the flatCoordinates and such to create a fromRenderFeature supporting all GeometryTypes. If someone else feels more confident to do this, I would gladly help review and test it.

Another idea that popped up, for the usecase of using the dragBox on VectorTile, only the Feature is not needed as only the Geometry is enough. We could also make geometry.fromRenderFeature to make it a bit more efficient? (Or implement both)

Lastly a comment correcting my previous post: MVT idProperty does not filter out duplicate selections, it just makes it easier to do on feature.id instead of feature[idProperty]. Correct?

[mike-000]

The idProperty ensures all parts of a feature are shown as selected if the polygon is spread over multiple tiles (for example Spain at zoom level 3 in https://openlayers.org/en/latest/examples/canvas-tiles-tms.html) and that is remains selected if the zoom level is changed and new tiles are loaded. In the examples it makes feature.getId() equivalent to feature.get('iso_a3').

[MoonE]

@EvertEt: Please have a look at #13297