Add "Map#addImage" / "Map#removeImage" #4404

lucaswoj · 2017-03-10T22:17:40Z

This PR adds the Map#addImage and Map#removeImage methods which give users the ability to modify their sprite at runtime.

Fixes #2059

Launch Checklist

lucaswoj · 2017-03-14T22:41:52Z

benchmark	master `787b3a2`	addimage `e069dd0`
map-load	115 ms	120 ms
style-load	101 ms	99 ms
buffer	1,011 ms	1,000 ms
fps	60 fps	60 fps
frame-duration	4.2 ms, 0% > 16ms	4.1 ms, 0% > 16ms
query-point	1.21 ms	0.88 ms
query-box	66.14 ms	63.76 ms
geojson-setdata-small	7 ms	3 ms
geojson-setdata-large	89 ms	162 ms

anandthakker

🎉 This is exciting! Couple questions/comments, mostly around documentation.

anandthakker · 2017-03-15T11:31:36Z

src/symbol/sprite_atlas.js

+
+        const rect = this.allocateImage(width, height);
+        if (!rect) {
+            this.fire('error', {error: new Error('There is not enough space to add this image.')});


Should this return early?

anandthakker · 2017-03-15T11:33:47Z

src/symbol/sprite_atlas.js

+        delete this.images[name];
+
+        if (!image) {
+            this.fire('error', {error: new Error('No image with this name exists.')});


Needs return

anandthakker · 2017-03-15T11:55:36Z

src/symbol/sprite_atlas.js

+            rect,
+            {pixelRatio: pixelRatio, x: 0, y: 0, width: image.width * pixelRatio, height: image.width * pixelRatio},
+            false
+        );


Is it necessary to overwrite the existing image data with an empty image? If so, then it might be good to leave a one-line comment here noting why.

I added this code out of fear of edge cases with semitransparent images & linear interpolation. Testing suggests these concerns are unwarranted. Removing it for now.

anandthakker · 2017-03-15T12:08:20Z

src/symbol/sprite_atlas.js

@@ -14,9 +16,11 @@ class AtlasImage {
    }
 }

-class SpriteAtlas {
+class SpriteAtlas extends Evented {


This isn't a comment about the changes in this PR, but just noting while I'm here: having never worked in this code before, I found it confusing, at first, to make sense of how/why SpriteAtlas works. E.g., "what's with the this.copy()?", "what's a rect?", "why does AtlasImage not have a reference to the underlying data?"

Reading more widely through the codebase, I think I've satisfied my questions, but this might be a nice time to add a brief, high-level description of what SpriteAtlas is/does. OTOH ticketing that separately is also totally fair 😄

All fair points!

I lost about a week trying to refactor, simplify, and document our whole style image subsystem and eventually decided that the minimal patch to add this feature was worth executing.

I'll push another commit with some targeted renames & comments to better explain SpriteAtlas until we get around to a bigger cleanup.

anandthakker · 2017-03-15T12:14:20Z

src/ui/map.js

+     * @param {number} options.width The pixel width of the `ArrayBufferView` image
+     * @param {number} options.height The pixel height of the `ArrayBufferView` image
+     * @param {Object} options.pixelRatio The ratio of pixels in the `ArrayBufferView` image to physical pixels on the screen
+     */


Since this could result in an error when there isn't enough space for a new image, should we document that possibility?

jfirebaugh

LGTM other than a few minor documentation adjustments.

jfirebaugh · 2017-03-15T17:21:51Z

src/ui/map.js

@@ -818,6 +818,30 @@ class Map extends Camera {
    }

    /**
+     * Add an image to the style. This image can be used in `icon-image`,
+     * `background-pattern`, `fill-patern`, and `line-pattern`.


fill-patern ⇢ fill-pattern

jfirebaugh · 2017-03-15T17:23:51Z

src/ui/map.js

+     * @param {Object} [options] Only required if passing an `ArrayBufferView`
+     * @param {number} options.width The pixel width of the `ArrayBufferView` image
+     * @param {number} options.height The pixel height of the `ArrayBufferView` image
+     * @param {Object} options.pixelRatio The ratio of pixels in the `ArrayBufferView` image to physical pixels on the screen


{Object} ⇢ {number}. Also, should this be marked as optional? It looks like the implementation will default to screen pixelRatio if it's not provided.

lucaswoj · 2017-03-15T19:55:56Z

I think this is ready to go. How does it look @anandthakker?

mourner · 2017-03-15T22:30:35Z

src/symbol/sprite_atlas.js

        this.width = width;
        this.height = height;

-        this.atlas = new ShelfPack(width, height);
+        this.shelfPack = new ShelfPack(width, height);


I'm not sure about this rename. ShelfPack is more of an algorithm name than a name that describes an instance. atlas is better IMO, and consistent with the class name (SpriteAtlas) since the latter is a wrapper around a generic atlas for image sprites.

I see your point and agree that the semantics of the name are questionable. My intent was to reduce the size of the vocabulary and amount of indirection in this class.

SpriteAtlas has a shelfPack

SpriteAtlas#shelfPack is an instance of ShelfPack

SpriteAtlas#shelfPack has bins

SpriteAtlas has a atlas

SpriteAtlas#atlas is an instance of ShelfPack

SpriteAtlas#atlas has bins

a wrapper around a generic atlas for image sprites.

If SpriteAtlas#atlas were a generic texture atlas, I'd be keen on calling it such. As it stands, this member only provides the "shelf pack" algorithm and not the rest of the texture atlas functionality.

anandthakker · 2017-03-16T13:08:57Z

LGTM @lucaswoj

lucaswoj · 2017-03-16T17:25:38Z

Merging and working on an example / demo as a follow-up 😄

pwilczynski · 2017-03-16T19:01:07Z

@lucaswoj - thank you so much, this looks awesome, and is so exciting for us. No more flickering maps for us!

* Update yarn.lock files * Add "Map#addImage" * Make `SpriteAtlas` fire `error` and `data` events * Add "Map#removeImage" * Relax requirement that styles using "icon-image" must have a "sprite" * Add "pixelRatio" parameter to "Map#addImage" * Enable "runtime-styling/image-add" integration test * Add "runtime-styling/image-remove" integration test * Add "runtime-styling/image-add-pattern" integration test * Add docs for `Map#addImage` and `Map#removeImage` * Add explanatory comment to `SpriteAtlas` * Rename SpriteAtlas#atlas to SpriteAtlas#shelfPack * Eliminate `AtlasImage` * Moar comments * Ensure `SpriteAtlas#addImage` returns early after an error * Misc docs improvements * Remove logic that clears texture when image is removed * fixup! Eliminate `AtlasImage` * Remove addImage debug page

ChrisLoer · 2017-04-03T19:07:10Z

I'm way late to this party, but how do we handle stale icons in SymbolBucket after a call to Map#removeImage? It seems like if you remove an image that's currently displayed on a tile, add another one (thus overwriting the old one's position in the SpriteAtlas), and then call redoPlacement, you'll get symbols referring to a garbage location in the SpriteAtlas. It looks like the expected behavior is if you call Map#removeImage while there are tiles displaying the image, the image should just disappear from the tiles the next time layout happens.

lucaswoj added the under development label Mar 10, 2017

lucaswoj force-pushed the addimage branch 3 times, most recently from 4473cd3 to c1b0972 Compare March 10, 2017 23:25

Lucas Wojciechowski added 3 commits March 13, 2017 11:16

Update yarn.lock files

3f91699

Add "Map#addImage"

b0969a6

Make SpriteAtlas fire error and data events

565c9da

lucaswoj force-pushed the addimage branch 2 times, most recently from 3c3df32 to e555657 Compare March 13, 2017 18:29

Lucas Wojciechowski added 5 commits March 13, 2017 12:39

Add "Map#removeImage"

bbaee18

Relax requirement that styles using "icon-image" must have a "sprite"

ed585d8

Add "pixelRatio" parameter to "Map#addImage"

58e7c1c

Enable "runtime-styling/image-add" integration test

d2dae85

Add "runtime-styling/image-remove" integration test

a298f4a

lucaswoj force-pushed the addimage branch from e555657 to a298f4a Compare March 13, 2017 19:39

Add "runtime-styling/image-add-pattern" integration test

82d470f

lucaswoj removed the under development label Mar 14, 2017

Add docs for Map#addImage and Map#removeImage

e069dd0

anandthakker suggested changes Mar 15, 2017

View reviewed changes

jfirebaugh approved these changes Mar 15, 2017

View reviewed changes

Lucas Wojciechowski added 9 commits March 15, 2017 11:44

Add explanatory comment to SpriteAtlas

a2eb682

Rename SpriteAtlas#atlas to SpriteAtlas#shelfPack

b5bb412

Eliminate AtlasImage

d1cf37f

Moar comments

af8a4cb

Ensure SpriteAtlas#addImage returns early after an error

2fa6b8a

Misc docs improvements

4533c5e

Remove logic that clears texture when image is removed

9eea591

fixup! Eliminate AtlasImage

d685f20

Remove addImage debug page

97665c2

mourner reviewed Mar 15, 2017

View reviewed changes

anandthakker approved these changes Mar 16, 2017

View reviewed changes

lucaswoj merged commit f50a304 into master Mar 16, 2017

lucaswoj deleted the addimage branch March 16, 2017 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Map#addImage" / "Map#removeImage" #4404

Add "Map#addImage" / "Map#removeImage" #4404

lucaswoj commented Mar 10, 2017 •

edited

Loading

lucaswoj commented Mar 14, 2017

anandthakker left a comment

anandthakker Mar 15, 2017

lucaswoj Mar 15, 2017

anandthakker Mar 15, 2017

lucaswoj Mar 15, 2017

anandthakker Mar 15, 2017

lucaswoj Mar 15, 2017

anandthakker Mar 15, 2017

lucaswoj Mar 15, 2017

anandthakker Mar 15, 2017

jfirebaugh left a comment

jfirebaugh Mar 15, 2017

lucaswoj Mar 15, 2017

jfirebaugh Mar 15, 2017

lucaswoj Mar 15, 2017

lucaswoj commented Mar 15, 2017

mourner Mar 15, 2017

lucaswoj Mar 16, 2017

lucaswoj Mar 16, 2017

anandthakker commented Mar 16, 2017

lucaswoj commented Mar 16, 2017

pwilczynski commented Mar 16, 2017

ChrisLoer commented Apr 3, 2017

Add "Map#addImage" / "Map#removeImage" #4404

Add "Map#addImage" / "Map#removeImage" #4404

Conversation

lucaswoj commented Mar 10, 2017 • edited Loading

Launch Checklist

lucaswoj commented Mar 14, 2017

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker commented Mar 16, 2017

lucaswoj commented Mar 16, 2017

pwilczynski commented Mar 16, 2017

ChrisLoer commented Apr 3, 2017

lucaswoj commented Mar 10, 2017 •

edited

Loading