Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbols: fix translation, variable-anchors and collision boxes #4071

Merged
merged 36 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
4fbf013
Import mercator symbol render tests from globe branch
kubapelc May 3, 2024
ef9a8f8
Import src changes to symbols from globe branch
kubapelc May 3, 2024
b408ef3
Add forgotten file change from globe branch
kubapelc May 3, 2024
62fe605
Use a reduced version of projection, adapt changes for main branch
kubapelc May 3, 2024
06f2f3e
Update build size
kubapelc May 3, 2024
effb706
Fix failing unit test
kubapelc May 3, 2024
00aff6d
Minor refactor
kubapelc May 3, 2024
f64af3f
Rename ProjectionArgs to SymbolProjectionContext
kubapelc May 13, 2024
02c409e
Refactor posMatrix in draw_collision_debug
kubapelc May 13, 2024
6cc4e1f
Add "_" to projectTruncatedLineSegment function name
kubapelc May 13, 2024
505ce4a
Refactor collision box passing into a function
kubapelc May 13, 2024
ffbde56
Remove unused code from placement.ts
kubapelc May 13, 2024
1a82f9d
collision_index: use else-if
kubapelc May 13, 2024
6706491
collision_index: avoid creating copies of points
kubapelc May 13, 2024
5c3dd5f
collision_index: refactor AABB projection using map() calls
kubapelc May 13, 2024
123e6b2
collision_index: remove unused code branch
kubapelc May 13, 2024
ad54cb7
Merge remote-tracking branch 'upstream/main' into kubapelc/symbols
kubapelc May 13, 2024
d73c7aa
Fix merge
kubapelc May 13, 2024
d084cf7
Change changelog formulation
kubapelc May 13, 2024
7f07bd1
Merge remote-tracking branch 'upstream/main' into kubapelc/symbols
kubapelc May 14, 2024
2587e77
Symbols: fix pitchWithMap texts sometimes having a wrongly shifted co…
kubapelc May 14, 2024
3e6d206
Symbols: fix pitchWithMap symbol collision boxes sometimes becoming o…
kubapelc May 14, 2024
4640469
Symbols: update render tests
kubapelc May 14, 2024
f21ecdc
Revert oversized collisionbox fix
kubapelc May 14, 2024
6ddea2c
Symbols: more render tests with collisions enabled
kubapelc May 14, 2024
9b1aba6
Remove unused arguments from drawCollisionDebug
kubapelc May 15, 2024
13a52f3
Refactor bounding box projection
kubapelc May 15, 2024
fc16852
Use explicit type for projection result in symbol projection
kubapelc May 15, 2024
b375b47
Add render test for distant pitch-aligned text collision bugfix
kubapelc May 15, 2024
e083e0c
Add render test variants without bounding box corners
kubapelc May 15, 2024
12ef88b
Merge remote-tracking branch 'upstream/main' into kubapelc/symbols
kubapelc May 15, 2024
efa54fe
Fix missing docs
kubapelc May 15, 2024
a516943
Add symbol box collisions benchmark
kubapelc May 15, 2024
0e8ceb6
Only measure collision box handling, not grid performance
kubapelc May 15, 2024
c7250cc
Minor refactor
kubapelc May 15, 2024
dc4b7a4
Fix benchmark name being inconsistent with file name
kubapelc May 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
- Sprites include optional textFitHeight and textFitWidth values ([#4019](https://github.com/maplibre/maplibre-gl-js/pull/4019))

### 🐞 Bug fixes

- Fixed symbol collision debug view (`showCollisionBoxes`) not showing the actual bounding boxes used for collision and click areas. The displayed boxes now match actual collision boxes exactly.
- Fixed symbol collisions using inaccurate and sometimes entirely wrong collision boxes when the map is pitched or rotated. ([#210](https://github.com/maplibre/maplibre-gl-js/issues/210))
- Fixed symbol collision boxes not being accurate for variable-anchor symbols.
- Fixed icon collision boxes using `text-translate` property for translation instead of the correct `icon-translate`.
- Fixed `text-translate` and `icon-translate` behaving weirdly and inconsistently with other `-translate` properties. ([#3456](https://github.com/maplibre/maplibre-gl-js/issues/3456))
- _...Add new stuff here..._

## 4.1.3
Expand Down
3 changes: 2 additions & 1 deletion src/data/bucket/symbol_attributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ export const placementOpacityAttributes = createLayout([

export const collisionVertexAttributes = createLayout([
{name: 'a_placed', components: 2, type: 'Uint8'},
{name: 'a_shift', components: 2, type: 'Float32'}
{name: 'a_shift', components: 2, type: 'Float32'},
{name: 'a_box_real', components: 2, type: 'Int16'},
]);

export const collisionBox = createLayout([
Expand Down
3 changes: 2 additions & 1 deletion src/data/bucket/symbol_bucket.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {IndexedFeature, PopulateParameters} from '../bucket';
import {StyleImage} from '../../style/style_image';
import glyphs from '../../../test/unit/assets/fontstack-glyphs.json' assert {type: 'json'};
import {StyleGlyph} from '../../style/style_glyph';
import {createProjection} from '../../geo/projection/projection';

// Load a point feature from fixture tile.
const vt = new VectorTile(new Protobuf(fs.readFileSync(path.resolve(__dirname, '../../../test/unit/assets/mbsv5-6-18-23.vector.pbf'))));
Expand Down Expand Up @@ -64,7 +65,7 @@ describe('SymbolBucket', () => {
const bucketA = bucketSetup() as any as SymbolBucket;
const bucketB = bucketSetup() as any as SymbolBucket;
const options = {iconDependencies: {}, glyphDependencies: {}} as PopulateParameters;
const placement = new Placement(transform, undefined as any, 0, true);
const placement = new Placement(transform, createProjection(), undefined as any, 0, true);
const tileID = new OverscaledTileID(0, 0, 0, 0, 0);
const crossTileSymbolIndex = new CrossTileSymbolIndex();

Expand Down
76 changes: 76 additions & 0 deletions src/geo/projection/projection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import type Point from '@mapbox/point-geometry';
import type {Tile} from '../../source/tile';
import {pixelsToTileUnits} from '../../source/pixels_to_tile_units';

/**
* A greatly reduced version of the `Projection` interface from the globe branch,
HarelM marked this conversation as resolved.
Show resolved Hide resolved
* used to port symbol bugfixes over to the main branch. Will be replaced with
* the proper interface once globe is merged.
*/
export type Projection = {
useSpecialProjectionForSymbols: boolean;
isOccluded(_x, _y, _t): boolean;
projectTileCoordinates(_x, _y, _t, _ele): {
point: Point;
signedDistanceFromCamera: number;
isOccluded: boolean;
};
getPitchedTextCorrection(_transform, _anchor, _tile): number;
translatePosition(transform: { angle: number; zoom: number }, tile: Tile, translate: [number, number], translateAnchor: 'map' | 'viewport'): [number, number];
getCircleRadiusCorrection(tr: any): number;
};

export function createProjection(): Projection {
return {
isOccluded(_x: any, _y: any, _t: any) {
HarelM marked this conversation as resolved.
Show resolved Hide resolved
return false;
},
getPitchedTextCorrection(_transform: any, _anchor: any, _tile: any) {
return 1.0;
},
get useSpecialProjectionForSymbols() { return false; },
projectTileCoordinates(_x, _y, _t, _ele) {
HarelM marked this conversation as resolved.
Show resolved Hide resolved
// This function should only be used when useSpecialProjectionForSymbols is set to true.
throw new Error('Not implemented.');
},
translatePosition(transform, tile, translate, translateAnchor) {
return translatePosition(transform, tile, translate, translateAnchor);
},
getCircleRadiusCorrection(_: any) {
return 1.0;
}
};
}

/**
* Returns a translation in tile units that correctly incorporates the view angle and the *-translate and *-translate-anchor properties.
* @param inViewportPixelUnitsUnits - True when the units accepted by the matrix are in viewport pixels instead of tile units.
*
* Temporarily imported from globe branch.
*/
function translatePosition(
transform: { angle: number; zoom: number },
tile: Tile,
translate: [number, number],
translateAnchor: 'map' | 'viewport',
inViewportPixelUnitsUnits: boolean = false
): [number, number] {
if (!translate[0] && !translate[1]) return [0, 0];

const angle = inViewportPixelUnitsUnits ?
(translateAnchor === 'map' ? transform.angle : 0) :
(translateAnchor === 'viewport' ? -transform.angle : 0);

if (angle) {
const sinA = Math.sin(angle);
const cosA = Math.cos(angle);
translate = [
translate[0] * cosA - translate[1] * sinA,
translate[0] * sinA + translate[1] * cosA
];
}

return [
inViewportPixelUnitsUnits ? translate[0] : pixelsToTileUnits(tile, translate[0], transform.zoom),
inViewportPixelUnitsUnits ? translate[1] : pixelsToTileUnits(tile, translate[1], transform.zoom)];
}
20 changes: 9 additions & 11 deletions src/render/draw_collision_debug.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {DepthMode} from '../gl/depth_mode';
import {StencilMode} from '../gl/stencil_mode';
import {CullFaceMode} from '../gl/cull_face_mode';
import {collisionUniformValues, collisionCircleUniformValues} from './program/collision_program';

import {QuadTriangleArray, CollisionCircleLayoutArray} from '../data/array_types.g';
import {collisionCircleLayout} from '../data/bucket/symbol_attributes';
import {SegmentVector} from '../data/segment';
Expand Down Expand Up @@ -38,10 +37,7 @@ export function drawCollisionDebug(painter: Painter, sourceCache: SourceCache, l
const tile = sourceCache.getTile(coord);
const bucket: SymbolBucket = (tile.getBucket(layer) as any);
if (!bucket) continue;
let posMatrix = coord.posMatrix;
if (translate[0] !== 0 || translate[1] !== 0) {
posMatrix = painter.translatePosMatrix(coord.posMatrix, tile, translate, translateAnchor);
}
const posMatrix = coord.posMatrix;
HarelM marked this conversation as resolved.
Show resolved Hide resolved
const buffers = isText ? bucket.textCollisionBox : bucket.iconCollisionBox;
// Get collision circle data of this bucket
const circleArray: Array<number> = bucket.collisionCircleArray;
Expand All @@ -50,7 +46,7 @@ export function drawCollisionDebug(painter: Painter, sourceCache: SourceCache, l
// This might vary between buckets as the symbol placement is a continuous process. This matrix is
// required for transforming points from previous screen space to the current one
const invTransform = mat4.create();
const transform = posMatrix;
const transform = posMatrix; // Ignore translation

mat4.mul(invTransform, bucket.placementInvProjMatrix, painter.transform.glCoordMatrix);
mat4.mul(invTransform, invTransform, bucket.placementViewportMatrix);
Expand All @@ -66,15 +62,17 @@ export function drawCollisionDebug(painter: Painter, sourceCache: SourceCache, l
circleCount += circleArray.length / 4; // 4 values per circle
circleOffset = circleCount;
}
if (!buffers) continue;

// Draw collision boxes
if (!buffers) {
continue;
}

program.draw(context, gl.LINES,
DepthMode.disabled, StencilMode.disabled,
painter.colorModeForRenderPass(),
CullFaceMode.disabled,
collisionUniformValues(
posMatrix,
painter.transform,
tile),
collisionUniformValues(painter.transform, coord.posMatrix),
painter.style.map.terrain && painter.style.map.terrain.getTerrainData(coord),
layer.id, buffers.layoutVertexBuffer, buffers.indexBuffer,
buffers.segments, null, painter.transform.zoom, null, null,
Expand Down
2 changes: 1 addition & 1 deletion src/render/draw_symbol.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ describe('drawSymbol', () => {
const spy = jest.spyOn(symbolProjection, 'updateLineLabels');
drawSymbols(painterMock, sourceCacheMock, layer, [tileId], null);

expect(spy.mock.calls[0][9]).toBeFalsy(); // rotateToLine === false
expect(spy.mock.calls[0][8]).toBeFalsy(); // rotateToLine === false
});

test('transparent tile optimization should prevent program.draw from being called', () => {
Expand Down
Loading