Add a pixelmap feature #637

manthey · 2016-10-27T15:05:20Z

This takes an image where the color values are treated as indices into the data data. This is used to generate a new color for each index. The resultant image is rendered as a quad. The feature is available in the canvas renderer.

The pixelmap image can be specified as a url or given as a Image element. The pixelmap feature will be a deferred jquery object if the image needs to be loaded.

This depends on PRs #633, #634, #635, and #636.

I've left the history of this branch as it has code that shows how this could be extended to non-canvas renderers (though it is slower).

The example uses a pixelmap of the United States and some political data that is probably already out of date.

This takes an image where the color values are treated as indices into the data data. This is used to generate a new color for each index. The resultant image is rendered as a quad. The feature is available of the d3, canvas, and vgl renderers.

Only gl features honored visibility, and then only if they did not consist of secondary features. Visibility now works on all renderers and a concept of dependentFeatures has been added to group visibility for certain features. Some features still probably don't support visibility properly (such as the graphFeature), but that should be left for a different fix.

The features() function replaced features but without properly ensure dropped features were removed or new features were added.

This returns the maximum value from the pixelmap index (not the number of distinct indices). ((maxIndex() + 1) is the useful length of the data array. Also, don't reload the index values from the pixelmap if it isn't necessary. When only the data updates, this saves drawing the pixelmap to a canvas.

Significantly increase the speed by caching more and updating less. Specifically, the working canvas is maintained between updates, and only changed portions are updated. canvas.toBlob is used in preference to canvas.toDataURL if it is available. maxIndex is cached. Added a fallback for boxSearch to the feature class to suppress some console warnings.

Use canvas properties for faster drawing of the final image. Compute only the extent of the changed area and only update that extent. This computes colors in one pass and updated pixels in a second pass rather than computing them in the same loop. The first render is probably very slightly slower.

…et is replaced.

Some changes were made to the quad feature and other details in PRs that haven't been merged in but affect the new pixelmap feature. This adjusts for them (mainly using the extra parameter on found points).

Also, handle more css color specifications. Currently, nothing takes advantage of a color which has an alpha value (though the pixelmap soon will). It would be nice to be able to use that in addition to opacity styles. Before, we handled colors of the form rgb object, #rrggbb, #rgb, and css color names. This adds parsing of #rrggbbaa, #rgba, rgb(), rgba(), hsl(), hsla(), and 'transparent', conforming that parsing to the css working group's current draft standard (there are other color formats in that draft that are not implemented). It also adds one additional css color name, as per that draft.

Refactored how image elements are used for the pixelmap so that the pixelmap becomes a deferred object. Fixed a petty issue with setting an image's crossOrigin property even when it was a data url.

codecov-io · 2016-10-27T15:12:16Z

Current coverage is 86.86% (diff: 100%)

Merging #637 into master will increase coverage by 0.32%

@@             master       #637   diff @@
==========================================
  Files            85         87     +2   
  Lines          8340       8544   +204   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7218       7422   +204   
  Misses         1122       1122          
  Partials          0          0

Powered by Codecov. Last update 6c9cd90...8f65be1

manthey · 2016-10-27T20:52:21Z

@aashish24 All dependent PRs have been merged in. This is ready for review.

aashish24 · 2016-10-28T12:29:55Z

Awesome feature 👏 👏 just has some minor things in the review.

aashish24

Had few comments / suggestions. I think mapColor could be color or colorMap for simplicity

aashish24 · 2016-10-28T12:02:52Z

src/canvas/pixelmapFeature.js

+  }
+  pixelmapFeature.call(this, arg);
+
+  var object = require('./object');


curious: why you had to do this?

In our various renderers, we have renderer-specific changes to objects, mostly to make a renderer-specific draw method.

okay, thanks.

aashish24 · 2016-10-28T12:10:34Z

src/pixelmapFeature.js

+
+  ////////////////////////////////////////////////////////////////////////////
+  /**
+   * Get the maximum index value from the pixelmap.  This is a value present in


Nitpick: in could be in the next line since its probably on column 79-80

'in' ends at column 79. Do you really want to change to less than 80 line lengths?

its fine then. on web-page it looks as if it was outside that's why I added probably

aashish24 · 2016-10-28T12:15:59Z

src/pixelmapFeature.js

+   * @returns {geo.pixelmap}
+   */
+  ////////////////////////////////////////////////////////////////////////////
+  this.mapColor = function (val) {


@manthey I am wondering why don't we just call it color like other features? since it is primarily a color transfer function? Also, I would think this should be a style element.

ah. I see later that it is a style element so why we need this method?

In many features, we have certain styles also accessible by functions (position often is, even if we store it in style as well). This allows for an easy side effect, such as only marking this as changed if it is actually different.

The problem with this approach in general is that we have different behaviors if these properties are set via style versus the special function -- for instance, style('position', <value>) won't update the dataTime and will always updated the modified time, even if the passed value is the same as the previous one. We might want to refactor the feature style method so that it is easier to have convenience functions and the behavior is identical regardless of whether the convenience function or style is used. However, that should be a general change, and not in this PR.

The only feature that has a plain color style is the solid-color quad feature. All other features have styles name fillColor, strokeColor, and probably others. My logic was to name it with a prefix to be clear what it was (but perhaps it doesn't help). We can rename this to color if you think that is better.

heatmap uses color. color could be static or a transfer function.

my preference would be slightly more towardscolor

The problem with this approach in general is that we have different behaviors if these properties are set via style versus the special function -- for instance, style('position', ) won't update the dataTime and will always updated the modified time, even if the passed value is the same as the previous one. We might want to refactor the feature style method so that it is easier to have convenience functions and the behavior is identical regardless of whether the convenience function or style is used. However, that should be a general change, and not in this PR.

Sure, let's keep this method and then we can revisit it once we refactor style function.

color would be better I think.

Okay. I'll switch it to color.

aashish24 · 2016-10-28T12:23:19Z

src/pixelmapFeature.js

+            m_srcImage.crossOrigin = this.style.get('crossDomain')() || 'anonymous';
+          }
+        }
+        m_srcImage.onload = function () {


I believe this code is only needed for else if and else?

The onload function is used on both a newly created Image and on an existing Image that isn't complete. It is within the else if clause on line 180.

aashish24 · 2016-10-28T12:25:10Z

src/pixelmapFeature.js

+  this._preparePixelmap = function () {
+    var i, idx, pixelData;
+
+    if (!m_srcImage || !m_srcImage.complete || !m_srcImage.naturalWidth ||


since this is a repeating code, may be in this or next PR we could write a function instead?

I'll make a utility function.

aashish24 · 2016-10-28T12:26:34Z

src/pixelmapFeature.js

+      m_info.imageData, 0, 0, 0, Math.floor(updateFirst / m_info.width),
+      m_info.width, Math.ceil((updateLast + 1) / m_info.width));
+
+    /* If we haven't made a quad feature, make one now.  The quad feature needs


Nitpick (can be done in another PR): Spacing between . (dot) and the.

I'm of the camp that there should be more spaces between sentences then between words. It is a topic of much bike-shedding (https://en.wikipedia.org/wiki/Sentence_spacing).

I am fine with that if we are consistent, may be we can change them at some point but i think right now we use single space. Not a roadblock for this PR though.

A quick grep shows 400+ uses of two spaces and 50 uses of 1 space. I think this should be left to the discretion of whomever writes comments, much like oxford commas, tight or loose punctuation, and other optional grammar rules.

Add a utility function to determine if an object is an HTML image element that is fully loaded.

aashish24 · 2016-10-28T15:13:07Z

A quick grep shows 400+ uses of two spaces and 50 uses of 1 space. I think this should be left to the discretion of whomever writes comments, much like oxford commas, tight or loose punctuation, and other optional grammar rules.

sure, as I referred earlier, I am not in favor of making it part for this PR.

manthey · 2016-10-28T16:05:11Z

@brianhelba We've renamed the mapColor style and function to color.

aashish24 · 2016-10-28T16:22:15Z

thanks @manthey. LGTM 👍

aashish24

LGTM 👍

aashish24 · 2016-10-28T16:54:48Z

Nice. Talked to @jbeezley and sounds like we are ready for another point release.

manthey added 13 commits October 12, 2016 11:36

Add a pixelmap feature.

bbf7669

This takes an image where the color values are treated as indices into the data data. This is used to generate a new color for each index. The resultant image is rendered as a quad. The feature is available of the d3, canvas, and vgl renderers.

Add and remove features on a feature layer.

9f5008e

The features() function replaced features but without properly ensure dropped features were removed or new features were added.

Add pixelmap example.

8886861

Add a event.pixelmap.prepared event.

3d0d00a

When constructing a mapInteractor with options, make sure the whole s…

8060563

…et is replaced.

Merge branch 'master' into pixelmap

88db688

Several adjustments to align with other PRs.

d98ca30

Some changes were made to the quad feature and other details in PRs that haven't been merged in but affect the new pixelmap feature. This adjusts for them (mainly using the extra parameter on found points).

Add tests.

b536513

Refactored how image elements are used for the pixelmap so that the pixelmap becomes a deferred object. Fixed a petty issue with setting an image's crossOrigin property even when it was a data url.

Merge branch 'master' into pixelmap

4b5d219

aashish24 reviewed Oct 28, 2016

View reviewed changes

Add isReadyImage function.

6e73a80

Add a utility function to determine if an object is an HTML image element that is fully loaded.

Rename mapColor to color.

8f65be1

aashish24 approved these changes Oct 28, 2016

View reviewed changes

manthey merged commit dcce0b6 into master Oct 28, 2016

manthey deleted the pixelmap branch October 28, 2016 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a pixelmap feature #637

Add a pixelmap feature #637

manthey commented Oct 27, 2016

codecov-io commented Oct 27, 2016 •

edited

Loading

manthey commented Oct 27, 2016

aashish24 commented Oct 28, 2016

aashish24 left a comment

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 Oct 28, 2016

manthey Oct 28, 2016

aashish24 commented Oct 28, 2016

manthey commented Oct 28, 2016

aashish24 commented Oct 28, 2016

aashish24 left a comment

aashish24 commented Oct 28, 2016

Add a pixelmap feature #637

Add a pixelmap feature #637

Conversation

manthey commented Oct 27, 2016

codecov-io commented Oct 27, 2016 • edited Loading

Current coverage is 86.86% (diff: 100%)

manthey commented Oct 27, 2016

aashish24 commented Oct 28, 2016

aashish24 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

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

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

aashish24 commented Oct 28, 2016

manthey commented Oct 28, 2016

aashish24 commented Oct 28, 2016

aashish24 left a comment

Choose a reason for hiding this comment

aashish24 commented Oct 28, 2016

codecov-io commented Oct 27, 2016 •

edited

Loading