Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Fix queryRenderedFeatues bug #14884

Merged
merged 2 commits into from
Jun 12, 2019
Merged

[core] Fix queryRenderedFeatues bug #14884

merged 2 commits into from
Jun 12, 2019

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Jun 10, 2019 •
edited
Loading

The bug was caused by an issue with the sortIndex calculation, i.e. after a polygon geometry was added to the feature index, its inner rings happened to have the same sort indexes as the features added at the next calls |=> the features added after the polygon were dropped from the query() result.

Fixes #14722

@pozdnyakov pozdnyakov force-pushed the mikhail_fix_query_bug branch 2 times, most recently from ef5d4ca to 04012e4 Compare June 10, 2019 13:03
@pozdnyakov pozdnyakov added Core The cross-platform C++ core, aka mbgl bug needs review labels Jun 10, 2019
astojilj
astojilj reviewed Jun 10, 2019
View reviewed changes
src/mbgl/geometry/feature_index.cpp Outdated
for (const auto& ring : geometries) {
auto envelope = mapbox::geometry::envelope(ring);
if (envelope.min.x < util::EXTENT &&
envelope.min.y < util::EXTENT &&
envelope.max.x >= 0 &&
envelope.max.y >= 0) {
grid.insert(IndexedSubfeature(index, sourceLayerName, bucketLeaderID, featureSortIndex++),
grid.insert(IndexedSubfeature(index, sourceLayerName, bucketLeaderID, sortIndex++),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it is important to change approach (featureSortIndex -> sortIndex change) for other than polygon features - was the removed code 04012e4#diff-c7acc1b0aee2a0628aac4613e46fa727L74 used only for polygons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the logic for keeping the same sorting index makes sense for polygon features only, @ansis could you confirm on that?

@friedbunny friedbunny added the needs changelog Indicates PR needs a changelog entry prior to merging. label Jun 10, 2019
@pozdnyakov pozdnyakov mentioned this pull request Jun 11, 2019
Closed
@pozdnyakov pozdnyakov requested a review from kkaefer June 12, 2019 13:47
kkaefer
kkaefer reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's a unit test missing here.

@pozdnyakov
Copy link
Contributor Author

pozdnyakov commented Jun 12, 2019 •
edited
Loading

The latest patch is different from the initial version: after offline talk to @ansis decided to follow a safer approach and still add geometries for inner rings to the grid and just keep the same sort id for them.

@pozdnyakov pozdnyakov requested a review from 1ec5 as a code owner June 12, 2019 16:40
@pozdnyakov pozdnyakov requested a review from a team June 12, 2019 16:40
@pozdnyakov pozdnyakov merged commit 6b7d3a1 into master Jun 12, 2019
@pozdnyakov pozdnyakov deleted the mikhail_fix_query_bug branch June 12, 2019 17:39
@friedbunny friedbunny removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Jun 13, 2019
@friedbunny friedbunny added this to the release-p milestone Jun 13, 2019
@friedbunny
Copy link
Contributor

friedbunny commented Jun 13, 2019 •
edited
Loading

Thanks for adding changelog entries, @pozdnyakov! Since this missed the branch cut for release-oolong yesterday, I’ve added it to the forthcoming release-p milestone.

Whoops, this did make release-oolong. In that case, we’ll want to move the changelog entry into the “5.1.0” section. /cc @fabian-guerra

@friedbunny friedbunny modified the milestones: release-p, release-oolong Jun 13, 2019
@friedbunny friedbunny mentioned this pull request Jun 13, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kkaefer kkaefer kkaefer left review comments

@astojilj astojilj astojilj left review comments

@ansis ansis ansis approved these changes

@alexshalamov alexshalamov Awaiting requested review from alexshalamov

@1ec5 1ec5 Awaiting requested review from 1ec5

Assignees
No one assigned
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Milestone
release-oolong
Development

Successfully merging this pull request may close these issues.

QueryRenderedFeatues with LineLayer
5 participants
@pozdnyakov @friedbunny @kkaefer @astojilj @ansis