Skip to content

Commit

Permalink
Fix issue #6919: bring gl-js collision handling closer to gl-native
Browse files Browse the repository at this point in the history
- Don't place features that _have_ text but don't have collision boxes
- Create a single collision box even when the length of a label is less than half the box size

Test updates:
- Add debug collision circles to line-center to verify collision behavior
- Add regression test that exercises case where a line label is almost exactly the same length as its line
  • Loading branch information
ChrisLoer committed Jul 11, 2018
1 parent bfc793f commit 5dd68bb
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/symbol/collision_feature.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class CollisionFeature {
bucketIndex: number,
overscaling: number) {
const step = boxSize / 2;
const nBoxes = Math.floor(labelLength / step);
const nBoxes = Math.floor(labelLength / step) || 1;
// We calculate line collision circles out to 300% of what would normally be our
// max size, to allow collision detection to work on labels that expand as
// they move into the distance
Expand Down
11 changes: 7 additions & 4 deletions src/symbol/placement.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,16 @@ export class Placement {

const partiallyEvaluatedTextSize = symbolSize.evaluateSizeForZoom(bucket.textSizeData, this.transform.zoom, symbolLayoutProperties.properties['text-size']);

const iconWithoutText = !bucket.hasTextData() || layout.get('text-optional');
const textWithoutIcon = !bucket.hasIconData() || layout.get('icon-optional');
const textOptional = layout.get('text-optional');
const iconOptional = layout.get('icon-optional');

const collisionGroup = this.collisionGroups.get(bucket.sourceID);

for (const symbolInstance of bucket.symbolInstances) {
if (!seenCrossTileIDs[symbolInstance.crossTileID]) {

let placeText = symbolInstance.feature.text !== undefined;
let placeIcon = symbolInstance.feature.icon !== undefined;
let placeText = false;
let placeIcon = false;
let offscreen = true;

let placedGlyphBoxes = null;
Expand Down Expand Up @@ -256,6 +256,9 @@ export class Placement {
offscreen = offscreen && placedIconBoxes.offscreen;
}

const iconWithoutText = textOptional || (symbolInstance.numGlyphVertices === 0 && symbolInstance.numVerticalGlyphVertices === 0);
const textWithoutIcon = iconOptional || symbolInstance.numIconVertices === 0;

// Combine the scales for icons and text.
if (!iconWithoutText && !textWithoutIcon) {
placeIcon = placeText = placeIcon && placeText;
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
{
"version": 8,
"metadata": {
"test": {
"debug": true,
"collisionDebug": true,
"description": "'Carribean Sea' crosses a tile boundary, but we don't draw the tile boundary in the test because JS and Native render tile boundaries differently.",
"height": 256,
"width": 1024
}
},
"center": [
-73,
15
],
"zoom": 4.5,
"sources": {
"mapbox": {
"type": "vector",
"maxzoom": 14,
"tiles": [
"local://tiles/mapbox.mapbox-streets-v7/{z}-{x}-{y}.mvt"
]
}
},
"glyphs": "local://glyphs/{fontstack}/{range}.pbf",
"layers": [
{
"id": "background",
"type": "background",
"paint": {
"background-color": "white"
}
},
{
"id": "line-center",
"type": "symbol",
"source": "mapbox",
"source-layer": "marine_label",
"layout": {
"text-field": "{name_en}",
"symbol-placement": "line-center",
"text-allow-overlap": true,
"text-size": 50,
"text-letter-spacing": 0.5,
"text-font": [
"Open Sans Semibold",
"Arial Unicode MS Bold"
]
}
},
{
"id": "line",
"type": "line",
"source": "mapbox",
"source-layer": "marine_label",
"paint": {
"line-width": 1
}
}
]
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"text-field": "{name_en}",
"symbol-placement": "line-center",
"text-allow-overlap": true,
"text-size": 50,
"text-size": 38,
"text-letter-spacing": 0.5,
"text-font": [
"Open Sans Semibold",
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"version": 8,
"metadata": {
"test": {
"collisionDebug": true,
"height": 512
}
},
Expand Down

0 comments on commit 5dd68bb

Please sign in to comment.