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

[core] fix deduping rings in querying, #11357 #11984

Merged
merged 1 commit into from
Jan 4, 2019
Merged

[core] fix deduping rings in querying, #11357 #11984

merged 1 commit into from
Jan 4, 2019

Conversation

ansis
Copy link
Contributor

@ansis ansis commented May 22, 2018

fix #11357

Different rings for the same feature were being assigned different indices. This assigns the same index to all features. -js already has the correct behaviour.

cc @mb12 @ChrisLoer

@ansis ansis requested a review from ChrisLoer May 22, 2018 20:10
@mb12
Copy link

mb12 commented May 22, 2018

@ansis Can you please kindly clarify the following regarding polygons and the spatial index constructed on polygon geometries?

Is there any advantage to inserting bounding box for all the internal rings of a polygon? Wouldn't it be efficient to just insert the bounding box of only the outer polygon (or if its a multipolygon just the outer ring of each polygon comprising the multipolygon)? The results of the index query have to be run against the actual polygon to rule out false positives. Adding internal rings to spatial index does not seem to have any advantage.

Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

👍

@mb12 That's a good point that inner rings may not be necessary for the coarse lookup step. There might be a useful optimization there (maybe for tiles with dense building geometries?), but I think we'd track that separately (and we'd want to be careful not to introduce bugs by accidentally making assumptions about the input geometry).

@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Jul 31, 2018
@stale
Copy link

stale bot commented Oct 24, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the archived Archived because of inactivity label Oct 24, 2018
@ChrisLoer
Copy link
Contributor

Whoa, we just forgot about this, right @ansis? I think we should still merge this.

@stale stale bot removed the archived Archived because of inactivity label Oct 24, 2018
@kkaefer
Copy link
Member

kkaefer commented Nov 5, 2018

bump

@stale stale bot added the archived Archived because of inactivity label Jan 4, 2019
@stale
Copy link

stale bot commented Jan 4, 2019

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.

@stale stale bot closed this Jan 4, 2019
@ChrisLoer
Copy link
Contributor

Back off stale-bot, we just need to merge this!

@ChrisLoer ChrisLoer reopened this Jan 4, 2019
@stale stale bot removed the archived Archived because of inactivity label Jan 4, 2019
@ChrisLoer ChrisLoer merged commit 750734d into master Jan 4, 2019
@ChrisLoer ChrisLoer deleted the fix-11357 branch January 4, 2019 23:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using sortIndex to skip duplicates in FeatureIndex::query is incorrect(dead code).
4 participants