Skip to content

Commit

Permalink
Fix issue mapbox#6919: bring gl-js collision handling closer to gl-na…
Browse files Browse the repository at this point in the history
…tive

- 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 authored and pirxpilot committed Sep 10, 2018
1 parent 8024de4 commit 1730666
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 5 deletions.
2 changes: 1 addition & 1 deletion src/symbol/collision_feature.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class CollisionFeature {
bucketIndex,
overscaling) {
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 @@ -153,16 +153,16 @@ 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 @@ -223,6 +223,9 @@ 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
}
}
]
}

0 comments on commit 1730666

Please sign in to comment.