scatter marker color gradients #1620

alexcjohnson · 2017-04-22T17:12:14Z

radial, horizontal, and vertical gradients for marker fills
for all svg scatter types

cc @etpinard @charleyferrari - I won't be able to make changes until I'm back online mid-late week but wanted to post this for review.

- radial, horizontal, and vertical - for all svg scatter types

alexcjohnson · 2017-04-22T17:14:31Z

src/components/drawing/index.js

-    // scaled by given max and min to colorscales
-    var markerScale = drawing.tryColorscale(marker, ''),
-        lineScale = drawing.tryColorscale(marker, 'line');
+drawing.gradient = function(sel, gd, type, color1, color2) {


I was debating whether this goes in color or drawing - ended up deciding on drawing primarily because it's tied to gd (so it can create defs in the right place, without having to walk the tree with Lib.getPlotDiv for every marker) and nothing else in color is... but could be persuaded otherwise.

This belongs in Drawing no doubt. Color is really a lib (i.e. utility) module that shouldn't know anything about plotly attributes and especially gd.

alexcjohnson · 2017-04-22T17:19:10Z

src/traces/scattercarpet/attributes.js

@@ -107,7 +107,8 @@ module.exports = {
        line: extendFlat({},
            {width: scatterMarkerLineAttrs.width},
            colorAttributes('marker'.line)
-        )
+        ),
+        gradient: scatterMarkerAttrs.gradient


TODO: test image with carpet plots. Somehow imagetest isn't working for me yet with carpets, must be something I still need to update?

rm -rf node_modules?

another TODO: looks like this PR broke gl2d.

Can you clarify which PR? Do you mean scattercarpet or this PR? Edit: realizing you probably mean these attributes need to be either added+implemented or properly avoided in gl2d.

This as in #1620 You can calm down @rreusser 😄

scattercarpet image test in 105ce9b
gl2d fix still to come.

etpinard · 2017-04-24T14:23:06Z

test/image/mocks/ternary_multiple.json

@@ -31,7 +38,14 @@
            ],
            "subplot": "ternary2",
            "name": "b missing",
-            "uid": "6c0753"
+            "uid": "6c0753",


might as well 🔪 those uid fields.

🔪 🔪 🔪 a074259

⤴️ commit of the year

etpinard · 2017-04-24T14:24:18Z

src/traces/scatter/marker_defaults.js

@@ -15,12 +15,18 @@ var colorscaleDefaults = require('../../components/colorscale/defaults');

 var subTypes = require('./subtypes');

-
+/*
+ * opts: object of flags to control features not all marker users support


Thanks for 🔒 ing this pattern. 🎉

etpinard · 2017-04-24T14:27:05Z

src/components/drawing/index.js

+ * except all at once before a full redraw.
+ * The upside of this is arbitrary points can share gradient defs
+ */
+drawing.initGradients = function(gd) {


Nice pattern. Controlling gradient defs in one place should help us in #581 too.

etpinard · 2017-04-24T14:28:56Z

src/plot_api/plot_api.js

@@ -153,6 +153,9 @@ Plotly.plot = function(gd, data, layout, config) {
        makePlotFramework(gd);
    }

+    // clear gradient defs on each .plot call, because we know we'll loop through all traces
+    Drawing.initGradients(gd);


It would be nice to add jasmine test checking that restyle can update gradient defs properly.

etpinard · 2017-04-24T14:30:29Z

src/components/drawing/index.js

+    ).replace(/[^\w\-]+/g, '_');
+    var gradient = fullLayout._defs.select('.gradients').selectAll('#' + gradientID).data([0]);
+
+    gradient.enter()


I suspect this algo would work for adding gradients to bar traces? Would be a cool feature 😏

yep, would be cool to add bar gradients... for another PR

alexcjohnson · 2017-05-02T15:39:16Z

test/jasmine/tests/drawing_test.js

+                ['radial', '#456', '#fff'],
+                ['radial', '#456', '#eee'],
+                ['radial', '#456', '#ddd']
+            ]);


Not happy with this... there are a bunch of ways for it to blow up in actual use, like if someone makes 1000 different gradients via different marker colors, and animates them through 1000 intermediate states, suddenly you have a million gradients lying around.

I think I need to change it to associating the gradient with the point rather than with the color combo - unless someone has another idea how to keep this in check.

alexcjohnson · 2017-05-03T00:09:48Z

src/components/drawing/index.js

+            else gradientColor = markerGradient.color;
+
+            var gradientID = 'g' + gd._fullLayout._uid + '-' + trace.uid;
+            if(perPointGradient) gradientID += '-' + i;


Now we're making either one gradient for the whole trace (if we can) or one per point (if any of the attributes varies per point) and it will update if the attributes change. This means that several traces (or several points in one trace with array attributes) will no longer be able to share a gradient def, but I don't think there are any cases where this can blow up.

That sounds right 👍

alexcjohnson · 2017-05-03T05:47:58Z

src/traces/scatter/arrays_to_calcdata.js

@@ -15,6 +15,9 @@ var Lib = require('../../lib');
 // arrayOk attributes, merge them into calcdata array
 module.exports = function arraysToCalcdata(cd, trace) {

+    // so each point knows which index it originally came from
+    for(var i = 0; i < cd.length; i++) cd[i].i = i;


@etpinard to my comment earlier today about not using calcdata as much: I needed to put this in because what I first tried to do, getting the index from the selection .each(d, i), is not reliable, as evidenced by the scattercarpet test image: the 0th point on the plot was actually index 1 from the trace (point 0 is cut off), but then the legend thought IT was point 0 so overwrote the other point 0 gradient.

BUT once we have that, we could get rid of the rest of these Lib.mergeArray statements - which, to address your concern earlier, don't do any sanitization anyway, we only do that for the axis data - and just directly access the arrays (if they are arrays) from within the style routines.

I'm not going to do that now, but could be a nice 🚀 at some point.

Good 👁️ here and nice fix. But, I think the solution would be to make Lib.mergeArray sanatize those arrays instead of 🔪 it. We can debate this in a future issue / PR 😄

etpinard · 2017-05-03T06:35:13Z

src/components/drawing/index.js

-        lineScale = drawing.tryColorscale(marker, 'line');
+    var markerScale = drawing.tryColorscale(marker, '');
+    var lineScale = drawing.tryColorscale(marker, 'line');
+    var gd = Lib.getPlotDiv(s.node());


I wonder if it might be better to add a ref to those gradient defs in fullData[i] then to look for gd? Or even better refactor those Drawing method to expect fullLayout as argument.

I'll leave it for now - I agree this is suboptimal but neither of these alternatives seems particularly simpler to me. If I had to pick one I'd probably go for the first actually... stashing the <defs> in each trace could come in handy elsewhere too.

etpinard · 2017-05-03T06:38:57Z

I'm not a big fan of that Lib.getPlotDiv call as highlighted here, but, then again, it's not that big of a deal and I'll call that non- ⛔ for 1.27.0

💃 nicely done.

scatter marker color gradients

541460a

- radial, horizontal, and vertical - for all svg scatter types

alexcjohnson commented Apr 22, 2017

View reviewed changes

etpinard reviewed Apr 24, 2017

View reviewed changes

etpinard added status: in progress feature something new labels Apr 24, 2017

etpinard added this to the 1.27.0 milestone Apr 24, 2017

alexcjohnson added 5 commits May 2, 2017 01:41

Merge branch 'master' into gradient-fill

ff6c910

include gradient arrays in scattercarpet test image

105ce9b

🔪 uid and *src from image mocks

a074259

more robust no-gradient-object fallback

1b0a6bb

test gradient restyle behavior

d1c6b51

alexcjohnson commented May 2, 2017

View reviewed changes

link gradient to the point, not the color combo

e3af5eb

alexcjohnson commented May 3, 2017

View reviewed changes

alexcjohnson added status: reviewable and removed status: in progress labels May 3, 2017

alexcjohnson added 2 commits May 3, 2017 01:19

store index with each cd item instead of trying to get it from the join

ec4d9ee

ignore cd[i].i in unrelated tests

21650c7

alexcjohnson commented May 3, 2017

View reviewed changes

etpinard reviewed May 3, 2017

View reviewed changes

alexcjohnson merged commit 2105fd4 into master May 3, 2017

alexcjohnson deleted the gradient-fill branch May 3, 2017 07:32

etpinard mentioned this pull request May 17, 2017

Fix toImage for marker gradient #1694

Merged

alexcjohnson mentioned this pull request Aug 15, 2017

fillcolor gradient #1918

Open

cpsievert mentioned this pull request Sep 11, 2017

ggplot2 extension packages plotly/plotly.R#566

Open

27 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scatter marker color gradients #1620

scatter marker color gradients #1620

alexcjohnson commented Apr 22, 2017

alexcjohnson Apr 22, 2017

etpinard Apr 24, 2017

alexcjohnson Apr 22, 2017

rreusser Apr 23, 2017

etpinard Apr 24, 2017

rreusser Apr 24, 2017 •

edited

Loading

etpinard Apr 24, 2017

alexcjohnson May 2, 2017

etpinard Apr 24, 2017

alexcjohnson May 2, 2017

etpinard May 3, 2017

etpinard Apr 24, 2017

etpinard Apr 24, 2017

etpinard Apr 24, 2017

etpinard Apr 24, 2017

alexcjohnson May 2, 2017

alexcjohnson May 2, 2017

alexcjohnson May 3, 2017

etpinard May 3, 2017

alexcjohnson May 3, 2017

etpinard May 3, 2017

etpinard May 3, 2017

alexcjohnson May 3, 2017

etpinard commented May 3, 2017

scatter marker color gradients #1620

scatter marker color gradients #1620

Conversation

alexcjohnson commented Apr 22, 2017

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

rreusser Apr 24, 2017 • edited Loading

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

etpinard commented May 3, 2017

rreusser Apr 24, 2017 •

edited

Loading