[core] don’t use stale collision tile in feature queries

[ivovandongen]

fixes #8289

We could make this check more granular since the FeatureIndex is still usable in this case, but since it's an edge case, I don't know if it's worth it.

[mention-bot]

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh and @1ec5 to be potential reviewers.

[ivovandongen]

To be able to stub out the FeatureIndex for testing I had to make FeatureIndex#query virtual

[jfirebaugh]

Instead of returning an empty result set for queries made between layout and placement, I think we could effectively exclude symbol layers from the query by resetting collisionTile in GeometryTile::onLayout. Then checking the collision tile will be skipped, but querying everything else is still possible.

[jfirebaugh]

Instead of adding an assertion, let's change features[i] to features.at(i). The latter has bounds-checking built in, and is what VectorTileLayer::getFeature uses.

[ivovandongen]

Check. I've changed this, but since vector::at throws an exception, which will still crash the app, I'm wondering if we shouldn't catch, log an error and continue in these cases. At least in release builds.

[jfirebaugh]

For logic errors, it's better to crash -- crashes are picked up by reporting tools, we get a bug report, and can fix the issue. In comparison, logs are mostly ignored.

[jfirebaugh]

Here's an alternative way to write the test that uses a real FeatureIndex object. Still requires a lot of setup, but I think using real objects as much as possible is good.

TEST(VectorTile, Issue8289) {
    VectorTileTest test;
    VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.updateParameters, test.tileset);

    auto data = std::make_unique<AnnotationTileData>();
    data->layers.emplace("test", AnnotationTileLayer("test"));
    data->layers.at("test").features.push_back(AnnotationTileFeature(0, FeatureType::Point, GeometryCollection()));

    // Simulate layout and placement of a symbol layer.
    tile.onLayout(GeometryTile::LayoutResult {
        {},
        std::make_unique<FeatureIndex>(),
        std::move(data),
        0
    });

    auto collisionTile = std::make_unique<CollisionTile>(PlacementConfig());

    IndexedSubfeature subfeature { 0, "", "", 0 };
    CollisionFeature feature(GeometryCoordinates(), Anchor(0, 0, 0, 0), -5, 5, -5, 5, 1, 0, style::SymbolPlacementType::Point, subfeature, false);
    collisionTile->insertFeature(feature, 0, true);
    collisionTile->placeFeature(feature, false, false);

    tile.onPlacement(GeometryTile::PlacementResult {
        {},
        std::move(collisionTile),
        0
    });

    // Simulate a second layout with empty data.
    tile.onLayout(GeometryTile::LayoutResult {
        {},
        std::make_unique<FeatureIndex>(),
        std::make_unique<AnnotationTileData>(),
        0
    });

    std::unordered_map<std::string, std::vector<Feature>> result;
    GeometryCoordinates queryGeometry {{ Point<int16_t>(0, 0) }};
    TransformState transformState;
    RenderedQueryOptions options;

    tile.queryRenderedFeatures(result, queryGeometry, transformState, options);

    EXPECT_TRUE(result.empty());
}

I verified that in current master, this fails -- not at the same point as in #8289, but enough to show that a data synchronization issue is present, and either of the proposed fixes will solve it.

[ivovandongen]

Thanks @jfirebaugh. I've made the requested changes, adding the test in a separate file for clarity.