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

[core] Avoid clipping symbols in continuous mode #6832

Merged
merged 3 commits into from
Nov 18, 2016

Conversation

brunoabinader
Copy link
Member

@brunoabinader brunoabinader commented Oct 26, 2016

Re-enable symbol clipping based on layout property settings, cleaning up workarounds and adds padding to symbol annotation tiles.

Avoid clipping symbols in continuous mode - preserving clipping behavior in still mode. To avoid overdrawing symbols on tile edges, we avoid inserting symbol features with anchors located among the tile extent edges.

Fixes #1673 and #6670.

/cc @jfirebaugh @ansis

@brunoabinader brunoabinader added bug Core The cross-platform C++ core, aka mbgl annotations Annotations on iOS and macOS or markers on Android labels Oct 26, 2016
@mention-bot
Copy link

@brunoabinader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @mikemorris and @ansis to be potential reviewers.

@jfirebaugh
Copy link
Contributor

Can you comment on #6670 (comment) and help fill in the gaps in my understanding?

@brunoabinader
Copy link
Member Author

Can you comment on #6670 (comment) and help fill in the gaps in my understanding?

I've added an extensive introspection as a comment in #6670.

@brunoabinader brunoabinader force-pushed the fix-symbol-clipping branch 2 times, most recently from ccf145e to adcdf2c Compare October 27, 2016 17:18
@@ -298,8 +297,7 @@ void SymbolLayout::addFeature(const GeometryCollection &lines,
// be drawn across tile boundaries. Instead they need to be included in
// the buffers for both tiles and clipped to tile boundaries at draw time.
//
// TODO remove the `&& false` when is #1673 implemented
const bool addToBuffers = (mode == MapMode::Still) || inside || (mayOverlap && false);
const bool addToBuffers = inside || mayOverlap;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how removing mode == MapMode::Still here is safe. In still mode, rendering a single tile, we cannot throw away features in the tile buffer under any circumstances, for the reasons detailed in #6670 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right - for Still mode we need to render all features.

@brunoabinader
Copy link
Member Author

Per #6670 (comment) - it seems we no longer need padding for symbol annotations - we can sort symbol render tiles instead prior to rendering.

We still clip in still mode, but just because we need to update our render test results. I'm deferring this work to a following PR.

@brunoabinader brunoabinader force-pushed the fix-symbol-clipping branch 3 times, most recently from 9ceff83 to 8854e8d Compare October 31, 2016 15:02
@brunoabinader
Copy link
Member Author

Based on @jfirebaugh's comments, I'm going to revert a few comments about clipping symbols for Still mode I've had removed on this PR. Will flag it reviewable again once it is finished.

@brunoabinader brunoabinader added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Nov 11, 2016
@brunoabinader brunoabinader force-pushed the fix-symbol-clipping branch 2 times, most recently from a161f22 to 43ceefd Compare November 11, 2016 23:17
@brunoabinader brunoabinader added ✓ ready for review and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 11, 2016
@brunoabinader
Copy link
Member Author

Ready for review.

@brunoabinader brunoabinader changed the title [core] Fix symbol clipping [core] Avoid clipping symbols in continuous mode Nov 17, 2016
@brunoabinader
Copy link
Member Author

@jfirebaugh @ansis I have cleaned up the patches from this PR, removing stuff that was not related to the proposed changes:

@brunoabinader brunoabinader merged commit 97288ca into master Nov 18, 2016
@brunoabinader brunoabinader deleted the fix-symbol-clipping branch November 18, 2016 08:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants