-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Convert GeometryTileWorker to "one-phase" loading #10457
Comments
@ChrisLoer Can you please confirm if there would be a change in behavior or not with one phase loading? With two phase loading, areas and lines appear first on the map even if it takes longer to prepare symbol buckets. This could happen for a couple of reasons like the glyph source timing out or poorly designed / large vector tiles. With 2 phase loading, the user gets a feedback in the form of areas and lines appearing on the screen. Would the same behavior be preserved( or not )with one phase loading? |
@mb12 From the rendering point of view, two-phase loading is already (mostly) gone -- we removed it with the global collision detection changes. At the time, we weighed the benefits of being able to load partial tiles (w/o symbols) against the increasing costs in complexity for maintaining the functionality. We already had trouble keeping bugs out of the two-phase logic, and global collision detection was set to make the problem more challenging, so we decided to remove the functionality. This issue is mainly about removing vestigial under-the-hood loading logic. The only case I know where it would affect rendering is when already loaded tiles are updated in place (in which case there was still some left-over "two phase" behavior which will disappear). |
Fixed with #11575. |
GeometryTileWorker
currently maintains a rather complicated state machine to handle "two phase" tile loading logic, in which non-symbol buckets can be rendered before symbol buckets are displayed, and in which a queue of asynchronous requests to redo symbol placement can be interleaved with asynchronous requests that may change tile layout.PR #10436 removes these requirements: symbol placement is now done on the foreground, and in line with GL JS, tiles no longer render before labels are ready to display.
This means that most of the state transition code in
GeometryTileWorker
is vestigial, and can be replaced with a much simpler state machine that:/cc @jfirebaugh @ansis @kkaefer
The text was updated successfully, but these errors were encountered: