Add addImage, removeImage API #7610

bsudekum · 2017-01-05T23:10:17Z

This exposes the addImage and removeImage functions to platform: node. Example usage:

map.addImage('myImageName', {
  imageBuffer: <Premultiplied image buffer>,
  height: 40,
  width: 40
});

The developer then would add a corresponding layer, myImageName to style the image.

@jfirebaugh I'm not seeing tests for other similar APIs like addLayer. How should this be tested?

mention-bot · 2017-01-05T23:10:18Z

@bsudekum, thanks for your PR! By analyzing this pull request, we identified @tmpsantos, @jfirebaugh and @tmcw to be potential reviewers.

jfirebaugh · 2017-01-06T00:13:57Z

Can you make the following API adjustments:

Add a pixelRatio property to the options, and use it instead of hard-coding 1
Pass the image data as a standalone second argument, before options

So, you'd call it like:

map.addImage("id", data, {width: w, height: h, pixelRatio: r});

jfirebaugh · 2017-01-06T00:17:16Z

platform/node/src/node_map.cpp

+        return Nan::ThrowTypeError("imageBuffer parameter required");
+    }
+
+    double imageHeight = Nan::Get(imageObject, Nan::New("height").ToLocalChecked()).ToLocalChecked()->NumberValue();


imageHeight and imageWidth should be integers -- validate the type using IsUint32 rather than IsNumber and convert using Uint32Value rather than NumberValue, assigning to a uint32_t rather than a double.

jfirebaugh · 2017-01-06T00:19:43Z

platform/node/src/node_map.cpp

+    double imageHeight = Nan::Get(imageObject, Nan::New("height").ToLocalChecked()).ToLocalChecked()->NumberValue();
+    double imageWidth = Nan::Get(imageObject, Nan::New("width").ToLocalChecked()).ToLocalChecked()->NumberValue();
+    auto imageBuffer = Nan::Get(imageObject, Nan::New("imageBuffer").ToLocalChecked()).ToLocalChecked()->ToObject();
+


Add a check here that the expected size is correct:

if (node::Buffer::Length(imageBuffer) != imageHeight * imageWidth * 4) { return Nan::ThrowTypeError("image size does not match buffer size"); }

tmpsantos · 2017-01-06T00:37:38Z

How should this be tested?

One option is to write an unit test like t.test('sets zoom before center', function(t) { ... } that uses pixelmatch for checking the rendering results.

platform/node/test/js/map.test.js:377

jfirebaugh · 2017-01-06T00:57:30Z

It would be better to write a test-suite render test that can be used to test the implementation of the equivalent GL JS feature.

Similar tests of the runtime styling API are in this directory. Take a look at the "operations" key in the style.jsons in that tree to get an idea of how this works.

Since we can't embed literal image data into the style.json, the format I'd propose for the addImage operation is to pass the name of a file in the image directory, and leave it up to suite_implementation.js to load the image data at runtime.

bsudekum · 2017-01-06T19:39:19Z

@jfirebaugh started to go down the route of adding a custom addImage handler to suite_implementation.js. However, I'm not getting an added image to the map. Notice any issues?

jfirebaugh · 2017-01-06T19:45:53Z

addImage doesn't work retroactively on layers that tried to use a previously non-existing image ID. Try waiting to add the symbol layer (using an addLayer operation) until after the image is added.

Also, let's require specifying an ID for the addImage operation, instead of reusing the path. (Using the intrinsic dimensions is fine though.)

bsudekum · 2017-01-06T20:20:24Z

@jfirebaugh this is ready for review

jfirebaugh · 2017-01-06T20:26:29Z

platform/node/test/suite_implementation.js

+                pixelRatio: 1
+            });
+
+            map.addLayer({


This should be test-specific; that is the add-image test should use:

"operations": [ [ "addImage", "marker", "./image/marker.png" ], [ "addLayer", ... ], [ "wait" ] ]

jfirebaugh · 2017-01-06T20:27:40Z

platform/node/src/node_map.cpp

+
+    std::unique_ptr<uint8_t[]> data = std::make_unique<uint8_t[]>(node::Buffer::Length(imageBuffer));
+    std::copy(node::Buffer::Data(imageBuffer), node::Buffer::Data(imageBuffer) + node::Buffer::Length(imageBuffer), data.get());
+    mbgl::PremultipliedImage cPremultipliedImage({ static_cast<uint32_t>(imageWidth), static_cast<uint32_t>(imageHeight) }, std::move(data));


Don't need static_casts here.

jfirebaugh · 2017-01-06T20:29:56Z

platform/node/src/node_map.cpp

+    uint32_t imageHeight = Nan::Get(optionObject, Nan::New("height").ToLocalChecked()).ToLocalChecked()->Uint32Value();
+    uint32_t imageWidth = Nan::Get(optionObject, Nan::New("width").ToLocalChecked()).ToLocalChecked()->Uint32Value();
+
+    if (imageWidth > 1024 && imageHeight > 1024) {


Should be || rather than &&.

jfirebaugh · 2017-01-06T21:35:06Z

platform/node/test/suite_implementation.js

+            });
+
+            applyOperations(operations.slice(1), callback);
+        } else if (operation[0] === 'addLayer') {


addLayer is already handled by the subsequent else block.

jfirebaugh · 2017-01-06T22:48:24Z

Can you open a PR for the mapbox-gl-test-suite additions too?

bsudekum · 2017-01-06T22:56:48Z

@jfirebaugh done mapbox/mapbox-gl-test-suite#201

1ec5 · 2017-01-06T23:34:54Z

platform/node/src/node_map.cpp

+    uint32_t imageHeight = Nan::Get(optionObject, Nan::New("height").ToLocalChecked()).ToLocalChecked()->Uint32Value();
+    uint32_t imageWidth = Nan::Get(optionObject, Nan::New("width").ToLocalChecked()).ToLocalChecked()->Uint32Value();
+
+    if (imageWidth > 1024 || imageHeight > 1024) {


FYI, it’s possible for the image-adding operation to have no effect (and log to stderr) if the size of all the images combined exceeds this 1024-by-1024 square.

bsudekum added Node.js node-mapbox-gl-native ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jan 5, 2017

bsudekum requested a review from jfirebaugh January 5, 2017 23:10

jfirebaugh suggested changes Jan 6, 2017

View reviewed changes

jfirebaugh reviewed Jan 6, 2017

View reviewed changes

bsudekum mentioned this pull request Jan 6, 2017

Add addImage test mapbox/mapbox-gl-test-suite#201

Merged

1ec5 reviewed Jan 6, 2017

View reviewed changes

Add addImage and removeImage API

2df627d

bsudekum force-pushed the bs-addImage branch from c95247d to 2df627d Compare January 7, 2017 00:08

bsudekum removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jan 7, 2017

Update test-suite

971a7cf

jfirebaugh approved these changes Jan 9, 2017

View reviewed changes

bsudekum merged commit 2988ca1 into master Jan 9, 2017

bsudekum deleted the bs-addImage branch January 9, 2017 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add addImage, removeImage API #7610

Add addImage, removeImage API #7610

bsudekum commented Jan 5, 2017

mention-bot commented Jan 5, 2017

jfirebaugh commented Jan 6, 2017

jfirebaugh Jan 6, 2017

jfirebaugh Jan 6, 2017

tmpsantos commented Jan 6, 2017

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

jfirebaugh Jan 6, 2017

jfirebaugh Jan 6, 2017

jfirebaugh Jan 6, 2017

jfirebaugh Jan 6, 2017

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

1ec5 Jan 6, 2017

Add addImage, removeImage API #7610

Add addImage, removeImage API #7610

Conversation

bsudekum commented Jan 5, 2017

mention-bot commented Jan 5, 2017

jfirebaugh commented Jan 6, 2017

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

tmpsantos commented Jan 6, 2017

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

jfirebaugh Jan 6, 2017

Choose a reason for hiding this comment

jfirebaugh commented Jan 6, 2017

bsudekum commented Jan 6, 2017

1ec5 Jan 6, 2017

Choose a reason for hiding this comment