port labeling, symbol sorting, and reparsing overscaled tiles #1666

ansis · 2015-05-29T21:22:53Z

this ports a bunch of stuff from -js:

sort overlapping symbols in y-direction sort overlapping symbols in the y direction mapbox-gl-js#1184
better and faster labeling better and faster labeling mapbox-gl-js#1079
reparse overscaled tiles reparse overscaled tiles mapbox-gl-js#1005

This is finally close to merging. Sorry about the amount of changes in one pull request.

TODO:

fix resource loader test failure https://travis-ci.org/mapbox/mapbox-gl-native/jobs/64636907#L1660
testing on an actual ios device. anyone have time to do this?

I haven't noticed any stability problems but a lot has changed so this will probably introduce new bugs.

Collision prevention is temporarily disabled.

-js doesn't have this here because it subtracts the buffer earlier. https://github.com/mapbox/mapbox-gl-js/blob/617e590c833040407508606b9dbdae2abc3a305d/js/symbol/glyph_source.js#L75 We should match -js eventually.

line-height or padding can make the height of a shaped object <= 0. In these cases either create no boxes, or create boxes with a minimum height to avoid creating too many of them. -js: 9e78c02d8b22b60a08fdb28fd29eef1642f62d94

Conflicts: src/mbgl/renderer/symbol_bucket.cpp src/mbgl/renderer/symbol_bucket.hpp src/mbgl/text/collision.cpp src/mbgl/text/glyph_store.cpp src/mbgl/text/placement.cpp src/mbgl/text/rotation_range.cpp

-js: e99ed95aaba5e79c322733b32e03518b12203a79

-js: 37a498a7aa2c37d6b94611b614b4efe134e6dd59

Conflicts: src/mbgl/map/tile_parser.cpp src/mbgl/map/tile_parser.hpp src/mbgl/renderer/painter.hpp src/mbgl/renderer/painter_symbol.cpp src/mbgl/renderer/symbol_bucket.cpp src/mbgl/renderer/symbol_bucket.hpp src/mbgl/text/collision.cpp src/mbgl/text/collision.hpp src/mbgl/text/placement.cpp

so that layout property functions are applied correctly and so that label placement is redone js: mapbox/mapbox-gl-js#1005 and mapbox/mapbox-gl-js@440bc02

jfirebaugh · 2015-06-02T21:17:42Z

@ansis This doesn't work very well on device at the moment. It seems to stop loading tiles after a few gestures.

ansis · 2015-06-02T21:21:17Z

@jfirebaugh I just pushed 1f5fb1d which I think should fix that

jfirebaugh · 2015-06-02T21:30:49Z

Yep, feels good now!

bleege · 2015-06-02T21:32:10Z

Same here. 👍 @ansis

ansis · 2015-06-02T23:57:40Z

A couple things should be reviewed:

redoPlacement uses workers safely. Overview:
Source::update calls VectorTileData::redoPlacement in the map thread.
VectorTileData::redoPlacement checks if placement needs to be redone and that there is no worker working on the tile currently. If there isn't, it calls VectorTileData::workerRedoPlacement in a worker which calls VectorTileData::endRedoPlacement in the main thread when it is done.
Source::tileLoadingCompleteCallback calls VectorTileData::redoPlacement in the map thread because the angle may have changed while the tile was loading.
To avoid unsafely overwriting existing buffers while they are being used, SymbolBucket stores the new buffers separately in renderDataInProgress. renderDataInProgress gets moved to renderData later from the main thread.
1e044a1 which implements reparsing tiles for high zoom levels changes TileID(z, x, y) to TileID(z, x, y, sourceZ). The first z is used to differentiate overparsed tiles that share the same actual vectortile. The second is used to download the data. For example if tile 15/200/300 needs to be overscaled for z17, the TileID would be (17, 200, 300, 15). Does this seem ok?
@ansis should double check that out-of-bounds accesses aren't possible in a couple spots where vertices are accessed by index

mb12 · 2015-06-03T01:22:56Z

1.) Is the following scenario handled?

a. worker starts doing replacement.
b. Tile becomes obsolete before worker thread doing replacement finishes.
c. worker should not call endRedoPlacement or somehow VectorTileData pointer should not be invalid when endRedoPlacement is called.

This will work in most cases because TileCache will hold a shared_ptr to VectorTileData. However this needs to work even when TileCache size is zero.

2.) Also does it make sense to do some testing with TileCache size as zero? Tile Cache may hide some bugs because it holds a shared_ptr to TileData.

ansis · 2015-06-03T02:00:48Z

Yes, the TileData destructor calls TileData::cancel, which resets workRequest, which calls WorkTask::cancel. WorkTask::cancel clears the callback (endRedoPlacement) so that it doesn't get called when WorkTask::runAfter gets called later (it's definitely later, because it gets called on the same thread).
Good idea. I tested it for a bit with cache size 0 and will keep that in mind for the future

thanks for the suggestions @mb12

1ec5 · 2015-06-03T17:38:52Z

So far so good with visual inspection.

jfirebaugh · 2015-06-03T19:13:57Z

redoPlacement uses workers safely

I don't see any glaring issues, but it's impossible to say much about thread safety conclusively, given the current structure. The TileData class hierarchy and worker code needs an overhaul to cleanly separate data on the worker thread from data on the map thread.

kkaefer · 2015-06-03T21:36:37Z

src/mbgl/map/live_tile_data.hpp

                 Style&,
                 GlyphAtlas&,
                 GlyphStore&,
                 SpriteAtlas&,
                 util::ptr<Sprite>,
-                 const SourceInfo&);
+                 const SourceInfo&,
+                 float,


Can we name parameters when it's not clear from their type?

and name parameters when it's not clear from their type

Conflicts: src/mbgl/renderer/painter_fill.cpp

mb12 · 2015-06-07T15:58:31Z

@ansis Does redoPlacement need to be called at higher zoom levels? Specifically when (map's current zoom > SourceInfo.maxzoom + 1). Or is this handled in some other way to ensure good symbol density at higher zoom levels? (CollisionTileData.maxScale is const 2.0)

ansis · 2015-06-08T14:57:17Z

@mb12 this is handled in some other way. mostly by 1e044a1. For zoom levels > maxzoom, tiles from the maximum zoom level are reparsed completely. Layout properties can be functions and in order for these functions to work it needs to reparse those layers at every zoom level. It reparses entire tiles instead of only layers that need it because that was easier to implement.

ansis and others added 30 commits March 30, 2015 17:48

remove in-shader label flipping

19ddaae

port some of the text shaping changes

7b29c5b

split generating symbol quads from collision code

9309f59

Collision prevention is temporarily disabled.

start porting symbol bucket changes

128ed1f

start porting CollisionTile and CollisionFeature

8fd42f3

port collision box debug rendering

dda1b77

port the remained of CollisionTile

fd526ae

port SymbolBucket::placeFeatures

e1059e7

remove old collision and rotation range code

dea7b32

don't add upside-down glyphs to buffers

c200053

port bboxifyLabel from -js

1d50485

port getAnchors from -js

c49e1ac

port checkMaxAngle from -js

2767a9b

port clipLines from -js

07b8504

remove old symbol collision types

7b02800

fix quad offsets by subtracting buffer

02d2dde

-js doesn't have this here because it subtracts the buffer earlier. https://github.com/mapbox/mapbox-gl-js/blob/617e590c833040407508606b9dbdae2abc3a305d/js/symbol/glyph_source.js#L75 We should match -js eventually.

remove tests for deleted rotation_range

1dcd1cf

fix android and Release builds

68ae21d

Merge branch 'master' into new-labelling

148294d

Conflicts: src/mbgl/renderer/symbol_bucket.cpp src/mbgl/renderer/symbol_bucket.hpp src/mbgl/text/collision.cpp src/mbgl/text/glyph_store.cpp src/mbgl/text/placement.cpp src/mbgl/text/rotation_range.cpp

fix bboxify where first box should be placed after anchor

34a5bf6

-js: e99ed95aaba5e79c322733b32e03518b12203a79

port line icon angle fix

7ac0166

-js: 37a498a7aa2c37d6b94611b614b4efe134e6dd59

Merge master @ 353e566

4d63162

redo placement when map rotates

d41bfa2

place symbols with correct angle on initial load

0bd39ae

make collision debug boxes toggleable

8962442

Reparse tiles when zoom level > source maxzoom

1e044a1

so that layout property functions are applied correctly and so that label placement is redone js: mapbox/mapbox-gl-js#1005 and mapbox/mapbox-gl-js@440bc02

fix clipping lines for labeling

4cd95a9

fix shaped icon box sizes

fd19eb7

This was referenced Jun 3, 2015

Adjust labeling density to match new mapbox-gl-native labeling work mapbox/mapbox-gl-styles#149

Closed

Emerald label render problems #1646

Closed

1ec5 mentioned this pull request Jun 3, 2015

Glyph bitmap overflow, disappearing labels on rotation #1681

Closed

kkaefer reviewed Jun 3, 2015
View reviewed changes

ansis added 2 commits June 3, 2015 19:17

fix collision box clipping

2bd7e45

and name parameters when it's not clear from their type

Merge remote-tracking branch 'origin/master' into HEAD

9c03b39

Conflicts: src/mbgl/renderer/painter_fill.cpp

ansis force-pushed the new-labelling branch from 8c2dbce to 9c03b39 Compare June 3, 2015 23:21

ansis merged commit 9c03b39 into master Jun 3, 2015

ansis removed the in progress label Jun 3, 2015

ansis deleted the new-labelling branch June 3, 2015 23:35

This was referenced Jun 3, 2015

Overlapping labels #1594

Closed

Labels pop in and out when you rotate #633

Closed

1ec5 mentioned this pull request Jun 4, 2015

Rotation causes some labels to collide #1689

Closed

friedbunny mentioned this pull request Jun 10, 2015

How to set z-ordering for annotations #1700

Closed

1ec5 mentioned this pull request Jun 19, 2015

update changelog for 0.4.0 #1756

Closed

This was referenced Jan 15, 2016

Parsing error in Mapbox Streets in San Jose, California #1454

Closed

Highway shield overlap #1409

Closed

ansis mentioned this pull request Jan 27, 2016

add setters, getters for map's minZoom and maxZoom #3712

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port labeling, symbol sorting, and reparsing overscaled tiles #1666

port labeling, symbol sorting, and reparsing overscaled tiles #1666

ansis commented May 29, 2015

jfirebaugh commented Jun 2, 2015

ansis commented Jun 2, 2015

jfirebaugh commented Jun 2, 2015

bleege commented Jun 2, 2015

ansis commented Jun 2, 2015

mb12 commented Jun 3, 2015

ansis commented Jun 3, 2015

1ec5 commented Jun 3, 2015

jfirebaugh commented Jun 3, 2015

kkaefer Jun 3, 2015

mb12 commented Jun 7, 2015

ansis commented Jun 8, 2015

port labeling, symbol sorting, and reparsing overscaled tiles #1666

port labeling, symbol sorting, and reparsing overscaled tiles #1666

Conversation

ansis commented May 29, 2015

jfirebaugh commented Jun 2, 2015

ansis commented Jun 2, 2015

jfirebaugh commented Jun 2, 2015

bleege commented Jun 2, 2015

ansis commented Jun 2, 2015

mb12 commented Jun 3, 2015

ansis commented Jun 3, 2015

1ec5 commented Jun 3, 2015

jfirebaugh commented Jun 3, 2015

kkaefer Jun 3, 2015

Choose a reason for hiding this comment

mb12 commented Jun 7, 2015

ansis commented Jun 8, 2015