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

[core] Mark allow-overlap symbols visible even outside of collision grid #12698

Merged
merged 1 commit into from
Sep 6, 2018

Conversation

ChrisLoer
Copy link
Contributor

Fixes issue #12683.

I tested the case this change was meant to address manually in a sample app that laid out a grid of allow-overlap: true symbols across the map. We don't have a good mechanism for automated testing of symbol fading on the native side, although when I do a port to GL JS I plan to write a test that will exercise the JS version of this logic.

cc @ansis @mkrussel77

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

This looks like this produces the right behaviour, but I'm wondering if it's possible to implement this without duplicating the text-optional logic.

Would making collisionIndex.placeFeature(....) always return placed: true for features with -allow-overlap: true achieve the same result? An extra condition to avoid adding offscreen features to the collisionindex might be necessary in this case.

@ChrisLoer
Copy link
Contributor Author

Would making collisionIndex.placeFeature(....) always return placed: true for features with -allow-overlap: true achieve the same result? An extra condition to avoid adding offscreen features to the collisionindex might be necessary in this case.

I tried going down something like that path. The problem is that you have to apply the "optional" logic in parallel to both the "placed" result and the "outside the grid so don't insert into the collision index" result (whatever you call it). Although we're OK with having these allow-overlap symbols at the edge of the grid show without being inserted into the grid, in general the logic gets a lot harder to think about as soon as what's in the grid follows subtly different rules from what's showing. I may just not be thinking about it the right way, but my attempt ended up looking like significantly more complicated.

I was attracted to the per-bucket "always show" logic because at least the extra calculation only has to happen once per bucket.

Copy link
Contributor

@ansis ansis left a comment

Choose a reason for hiding this comment

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

I tried going down something like that path. The problem is that you have to apply the "optional" logic in parallel to both the "placed" result and the "outside the grid so don't insert into the collision index" result (whatever you call it). Although we're OK with having these allow-overlap symbols at the edge of the grid show without being inserted into the grid, in general the logic gets a lot harder to think about as soon as what's in the grid follows subtly different rules from what's showing. I may just not be thinking about it the right way, but my attempt ended up looking like significantly more complicated.

I was attracted to the per-bucket "always show" logic because at least the extra calculation only has to happen once per bucket.

Sounds good then!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants