make symbol placement information transferrable between workers #2624

mollymerp · 2016-05-25T16:45:44Z

first step in the quest to make cross-source label placement a reality! re: #2516

next actions:

code review @lucaswoj @ansis
~~tests! I have no idea how to do this, so I will spend some time poking around the existing tests and try to crib from those to make sure the correct data is getting transferred.~~ this is basically a refactor, no new functionality to test
create a process to transfer the symbol_buffer data to a new worker where it could be processed together with symbol_buffers from other sources using placeFeatures() at that time.

also:

does it make sense to pass -1 as the index for the symbolquads that do not exist in a tile?

return this.symbolInstancesBuffer.emplaceBack(
     startGlyphIndex || -1,
     endGlyphIndex || -1,
     iconQuadIndex || -1,
     symbolInstance.x,
     symbolInstance.y,
     symbolInstance.index);

similarly, I was getting symbol buckets that had neither glyphs or icons... why would that be happening? or am I missing something?

lucaswoj · 2016-05-25T18:52:04Z

js/data/bucket/symbol_bucket.js

@@ -29,6 +29,8 @@ function SymbolBucket(options) {
    this.showCollisionBoxes = options.showCollisionBoxes;
    this.overscaling = options.overscaling;
    this.collisionBoxArray = options.collisionBoxArray;
+    this.symbolQuadsBuffer = options.symbolQuadsBuffer;
+    this.symbolInstancesBuffer = options.symbolInstancesBuffer;


Can we call these Arrays instead of Buffers? We generally use the term Buffer exclusively in the context of a WebGL buffer (though I'd like to phase that out too)

lucaswoj · 2016-05-25T19:05:39Z

This is looking great! The code is tight and idiomatic. I made a couple superficial comments and am feeling 👍 about the direction here.

mollymerp · 2016-05-25T21:37:25Z

getting some interesting rendering test failures 💭

lucaswoj · 2016-05-25T21:51:04Z

Try re-installing mapbox-gl-test-suite. I've seen that failure when using v1 tiles with earcut.

rm -rf node_modules/mapbox-gl-test-suite && npm install

mollymerp · 2016-05-25T22:42:36Z

Ah yep, that was it @lucaswoj!

ansis · 2016-05-31T23:04:13Z

Looks good

does it make sense to pass -1 as the index for the symbolquads that do not exist in a tile?

It might be nice to not need a special value for the empty case. Sync the startGlyphIndex and endGlyphIndex represent a range, there are no elements if both values are the same. For example, if the start index is 52 and the end index is 52 then there are zero things between 52 and 52.

similarly, I was getting symbol buckets that had neither glyphs or icons... why would that be happening? or am I missing something?

It could be that the tile doesn't have any features matching the bucket's filter.

A good next step would be to get rid of the original symbolInstances array completely and use the new struct array version in placeFeatures

mollymerp · 2016-06-03T02:40:48Z

Update:
As @ansis suggested, I'm refactoring to allow the total removal of the symbolInstances array to remove data duplication.

Seems like the sort function I implemented for the StructArray for placement of overlapping symbols isn't working. Also the render tests make me think that tile boundary clipping isn't working as expected.

Also, I think I should treat the texture varables in the symbolQuadArray as floats because the hamburgers look funny and I think it has to do with imprecise texture coordinates? 💭

mollymerp · 2016-06-03T04:00:20Z

I fixed the sort issue, and all but 2 of the remaining render tests pass if I set all the anchor point offset variables (tl, tr, bl, br) in the structArray to Float32 so I can either update the render tests to match this loss of precision, or keep the floats.

The tests that still fail are symbol-placement point and symbol-visibility visible

lucaswoj · 2016-06-03T17:38:04Z

What is the downside to setting the anchor point offset variables to Float32?

mollymerp · 2016-06-03T18:29:55Z

@lucaswoj I'm not sure what the downside is, but I would guess it has to do with memory usage and/or speed of transfer from worker --> main thread. I ran all the bench marking tests and didn't get any warnings. Looks like it got significantly faster actually.
Buffer bench on master: 1,132 ms
Buffer bench on this branch: 830ms

The last remaining piece of this puzzle is figuring out why cross-tile collision detection is not working

more art:
master

this branch

I have a feeling it has to do with the fact that we're not passing a true CollisionFeature to the functions CollisionTile.placeCollisionFeature and CollisionTile.insertCollisionFeature but I haven't figured out how :(

mollymerp · 2016-06-04T00:04:19Z

Seems to be an issue with how the CollisionTile.grid is being populated. The fully populated grids on this branch have fewer keys and bboxes than those on master. When I console collisionTile.grid in the done () function in WorkerTile I get very different resultant grids:

master

this branch

😭

lucaswoj · 2016-06-06T22:04:46Z

debug/index.html

+        }
+    });
+
+    map.addLayer({
        "id": "route",
        "type": "line",
        "source": "geojson",


Please revert your changes to this file

…Instances and SymbolQuads create SymbolInstancesArray StructArrayType add symbolinstances and quads to struct arrays populate symbolInstanceBuffer and symbolQuadsBuffer transfer symbol buffers from worker --> main thread add symbolInstances and symbolQuads to tile.js

…ransferred back to the worker transferrable-symbol-buffer: refactor placeFeatures to use the structArrays add tex getter to symbol_quads StructArrayType add sort method to StructArray in order to sort overlapping symbols during label placement store structArray info for placeFeatures use structArray to render labels -- collision detection not working fix typo in collision boxes move sorting function, handle overlaps fix symbolQuad sort remove all this.symbolInstances array completely, change offset coordinates to Float32 fix formatting

…e the structArrays only to place symbol features on the map revert debug page rearrange methods for converting a StructArray into an array of StructTypes

mollymerp added feature 🍏 in progress labels May 25, 2016

lucaswoj reviewed May 25, 2016
View reviewed changes

mollymerp force-pushed the transferrable-symbol-buffer branch from 387a23a to c68e579 Compare May 25, 2016 19:09

mollymerp force-pushed the transferrable-symbol-buffer branch from b58061c to 3f4d3a2 Compare June 3, 2016 01:32

mollymerp force-pushed the transferrable-symbol-buffer branch from f5b4a64 to 75032c5 Compare June 3, 2016 02:44

mollymerp force-pushed the transferrable-symbol-buffer branch from 343d93a to 6496115 Compare June 3, 2016 18:18

mollymerp force-pushed the transferrable-symbol-buffer branch from 37ba3c7 to 6d3923f Compare June 6, 2016 22:01

lucaswoj reviewed Jun 6, 2016
View reviewed changes

Molly Lloyd added 3 commits June 6, 2016 15:53

[transferrable symbol buffer] remove old symbolInstances array and us…

e268120

…e the structArrays only to place symbol features on the map revert debug page rearrange methods for converting a StructArray into an array of StructTypes

mollymerp force-pushed the transferrable-symbol-buffer branch from 5a51853 to e268120 Compare June 6, 2016 22:56

mollymerp removed the do not merge label Jun 6, 2016

mollymerp changed the title ~~[wip] transferrable symbol buffer~~ make symbol placement information transferrable between workers Jun 6, 2016

mollymerp merged commit e268120 into master Jun 6, 2016

mollymerp deleted the transferrable-symbol-buffer branch June 6, 2016 23:31

mollymerp mentioned this pull request Jun 9, 2016

Implement cross-source symbol placement #2703

Closed

jfirebaugh mentioned this pull request Oct 28, 2016

Simplify Bucket #3465

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make symbol placement information transferrable between workers #2624

make symbol placement information transferrable between workers #2624

mollymerp commented May 25, 2016 •

edited

Loading

lucaswoj May 25, 2016

lucaswoj commented May 25, 2016

mollymerp commented May 25, 2016

lucaswoj commented May 25, 2016

mollymerp commented May 25, 2016

ansis commented May 31, 2016

mollymerp commented Jun 3, 2016 •

edited

Loading

mollymerp commented Jun 3, 2016 •

edited

Loading

lucaswoj commented Jun 3, 2016

mollymerp commented Jun 3, 2016 •

edited

Loading

mollymerp commented Jun 4, 2016

lucaswoj Jun 6, 2016

make symbol placement information transferrable between workers #2624

make symbol placement information transferrable between workers #2624

Conversation

mollymerp commented May 25, 2016 • edited Loading

lucaswoj May 25, 2016

Choose a reason for hiding this comment

lucaswoj commented May 25, 2016

mollymerp commented May 25, 2016

lucaswoj commented May 25, 2016

mollymerp commented May 25, 2016

ansis commented May 31, 2016

mollymerp commented Jun 3, 2016 • edited Loading

mollymerp commented Jun 3, 2016 • edited Loading

lucaswoj commented Jun 3, 2016

mollymerp commented Jun 3, 2016 • edited Loading

mollymerp commented Jun 4, 2016

lucaswoj Jun 6, 2016

Choose a reason for hiding this comment

mollymerp commented May 25, 2016 •

edited

Loading

mollymerp commented Jun 3, 2016 •

edited

Loading

mollymerp commented Jun 3, 2016 •

edited

Loading

mollymerp commented Jun 3, 2016 •

edited

Loading