Skip to content

gl3d reversescale fixup #3418

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
etpinard merged 4 commits into from
Jan 8, 2019
Merged

gl3d reversescale fixup #3418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
379ffae
fix reversescale for gl3d traces
etpinard Jan 8, 2019
d408d9a
add gl3d reversescale mock
etpinard Jan 8, 2019
5cc458d
use 'title.text' in mocks
etpinard Jan 8, 2019
454eed8
:hocho: duplicate `coerce('colorscale')`
etpinard Jan 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/lib/gl_format_color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,13 @@ function formatColor(containerIn, opacityIn, len) {
return colorOut;
}

function parseColorScale(colorscale, alpha) {
function parseColorScale(cont, alpha) {
if(alpha === undefined) alpha = 1;

var colorscale = cont.reversescale ?
Colorscale.flipScale(cont.colorscale) :
cont.colorscale;

return colorscale.map(function(elem) {
var index = elem[0];
var color = tinycolor(elem[1]);
Expand Down
2 changes: 1 addition & 1 deletion src/traces/cone/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ function convert(scene, trace) {
trace._len
);

coneOpts.colormap = parseColorScale(trace.colorscale);
coneOpts.colormap = parseColorScale(trace);
coneOpts.vertexIntensityBounds = [trace.cmin / trace._normMax, trace.cmax / trace._normMax];
coneOpts.coneOffset = anchor2coneOffset[trace.anchor];

Expand Down
2 changes: 1 addition & 1 deletion src/traces/mesh3d/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ proto.update = function(data) {
this.color = '#fff';
config.vertexIntensity = data.intensity;
config.vertexIntensityBounds = [data.cmin, data.cmax];
config.colormap = parseColorScale(data.colorscale);
config.colormap = parseColorScale(data);
} else if(data.vertexcolor) {
this.color = data.vertexcolor[0];
config.vertexColors = parseColorArray(data.vertexcolor);
Expand Down
2 changes: 1 addition & 1 deletion src/traces/streamtube/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ function convert(scene, trace) {
tubeOpts.startingPositions = startingPositions;
}

tubeOpts.colormap = parseColorScale(trace.colorscale);
tubeOpts.colormap = parseColorScale(trace);
tubeOpts.tubeSize = trace.sizeref;
tubeOpts.maxLength = trace.maxdisplayed;

Expand Down
2 changes: 1 addition & 1 deletion src/traces/surface/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ proto.update = function(data) {
var sceneLayout = scene.fullSceneLayout;
var surface = this.surface;
var alpha = data.opacity;
var colormap = parseColorScale(data.colorscale, alpha);
var colormap = parseColorScale(data, alpha);
var scaleFactor = scene.dataScale;
var xlen = data.z[0].length;
var ylen = data._ylength;
Expand Down
4 changes: 0 additions & 4 deletions src/traces/surface/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
* LICENSE file in the root directory of this source tree.
*/


'use strict';

var Registry = require('../../registry');
Expand All @@ -15,7 +14,6 @@ var Lib = require('../../lib');
var colorscaleDefaults = require('../../components/colorscale/defaults');
var attributes = require('./attributes');


module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout) {
var i, j;

Expand Down Expand Up @@ -61,8 +59,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout

var surfaceColor = coerce('surfacecolor');

coerce('colorscale');

var dims = ['x', 'y', 'z'];
for(i = 0; i < 3; ++i) {

Expand Down
Binary file added test/image/baselines/gl3d_reversescale.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
148 changes: 148 additions & 0 deletions test/image/mocks/gl3d_reversescale.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
{
"data": [{
"type": "surface",
"z": [
[8.41, 3, 4],
[1, 2, 3],
[2, 43, 1]
],
"reversescale": true,
"colorbar": {
"title": {"text": "surface"},
"x": -0.15,
"len": 0.3,
"y": 0.7
}
}, {
"type":"mesh3d",
"name": "colorscale + intensity",
"x":[0, 1, 2, 0],
"y":[0, 0, 1, 2],
"z":[0, 2, 0, 1],
"i":[0, 0, 0, 1],
"j":[1, 2, 3, 2],
"k":[2, 3, 1, 3],
"colorscale": [
[0, "rgb(0, 0, 0)"],
[0.33, "rgb(255, 0, 0)"],
[0.66, "rgb(0, 255, 0)"],
[1, "rgb(0, 0, 255)"]
],
"intensity": [0, 0.33, 0.66, 1],
"reversescale": true,
"colorbar": {
"title": {"text": "mesh3d"},
"x": -0.15,
"len": 0.3,
"y": 0.4
}
}, {
"type": "streamtube",
"x": [0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2],
"y": [0, 0, 0, 1, 1, 1, 2, 2, 2, 0, 0, 0, 1, 1, 1, 2, 2, 2, 0, 0, 0, 1, 1, 1, 2, 2, 2],
"z": [0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2, 0, 1, 2],
"u": [
1,
1,
1,
1,
1,
1,
1,
1,
1,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.8414709848078965,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817,
1.9092974268256817
],
"v": [
1,
1,
1,
0.5403023058681398,
0.5403023058681398,
0.5403023058681398,
-0.4161468365471424,
-0.4161468365471424,
-0.4161468365471424,
1,
1,
1,
0.5403023058681398,
0.5403023058681398,
0.5403023058681398,
-0.4161468365471424,
-0.4161468365471424,
-0.4161468365471424,
1,
1,
1,
0.5403023058681398,
0.5403023058681398,
0.5403023058681398,
-0.4161468365471424,
-0.4161468365471424,
-0.4161468365471424
],
"w": [
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106,
0,
0.08865606199840186,
0.1693927420185106
],
"reversescale": true,
Copy link
Contributor

@archmoj archmoj Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if the user also added "reversescl": false here it would overwrite "reversescale" attribute.

So you may consider adding those conditions here:

plotly.js/src/plot_api/helpers.js

Line 326 in 673b542

trace.reversescale = trace.reversescl;

For example:

// scl->scale, reversescl->reversescale
if('scl' in trace) {
    if(!'colorscale' in trace) trace.colorscale = trace.scl;
    delete trace.scl;
}
if('reversescl' in trace) {
    if(!'reversescale' in trace) trace.reversescale = trace.reversescl;
    delete trace.reversescl;
}

Copy link
Contributor Author

@etpinard etpinard Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a test in the cleanData block

plotly.js/test/jasmine/tests/plot_api_test.js

Lines 2248 to 2535 in 673b542

describe('cleanData & cleanLayout', function() {
var gd;
beforeEach(function() {
gd = createGraphDiv();
});
afterEach(destroyGraphDiv);
it('should rename \'YIGnBu\' colorscales YlGnBu (2dMap case)', function() {
var data = [{
type: 'heatmap',
colorscale: 'YIGnBu'
}];
Plotly.plot(gd, data);
expect(gd.data[0].colorscale).toBe('YlGnBu');
});
it('should rename \'YIGnBu\' colorscales YlGnBu (markerColorscale case)', function() {
var data = [{
type: 'scattergeo',
marker: { colorscale: 'YIGnBu' }
}];
Plotly.plot(gd, data);
expect(gd.data[0].marker.colorscale).toBe('YlGnBu');
});
it('should rename \'YIOrRd\' colorscales YlOrRd (2dMap case)', function() {
var data = [{
type: 'contour',
colorscale: 'YIOrRd'
}];
Plotly.plot(gd, data);
expect(gd.data[0].colorscale).toBe('YlOrRd');
});
it('should rename \'YIOrRd\' colorscales YlOrRd (markerColorscale case)', function() {
var data = [{
type: 'scattergeo',
marker: { colorscale: 'YIOrRd' }
}];
Plotly.plot(gd, data);
expect(gd.data[0].marker.colorscale).toBe('YlOrRd');
});
it('should rename \'highlightColor\' to \'highlightcolor\')', function() {
var data = [{
type: 'surface',
contours: {
x: { highlightColor: 'red' },
y: { highlightcolor: 'blue' }
}
}, {
type: 'surface'
}, {
type: 'surface',
contours: false
}, {
type: 'surface',
contours: {
stuff: {},
x: false,
y: []
}
}];
spyOn(Plots.subplotsRegistry.gl3d, 'plot');
Plotly.plot(gd, data);
expect(Plots.subplotsRegistry.gl3d.plot).toHaveBeenCalled();
var contours = gd.data[0].contours;
expect(contours.x.highlightColor).toBeUndefined();
expect(contours.x.highlightcolor).toEqual('red');
expect(contours.y.highlightcolor).toEqual('blue');
expect(contours.z).toBeUndefined();
expect(gd.data[1].contours).toBeUndefined();
expect(gd.data[2].contours).toBe(false);
expect(gd.data[3].contours).toEqual({ stuff: {}, x: false, y: [] });
});
it('should rename \'highlightWidth\' to \'highlightwidth\')', function() {
var data = [{
type: 'surface',
contours: {
z: { highlightwidth: 'red' },
y: { highlightWidth: 'blue' }
}
}, {
type: 'surface'
}];
spyOn(Plots.subplotsRegistry.gl3d, 'plot');
Plotly.plot(gd, data);
expect(Plots.subplotsRegistry.gl3d.plot).toHaveBeenCalled();
var contours = gd.data[0].contours;
expect(contours.x).toBeUndefined();
expect(contours.y.highlightwidth).toEqual('blue');
expect(contours.z.highlightWidth).toBeUndefined();
expect(contours.z.highlightwidth).toEqual('red');
expect(gd.data[1].contours).toBeUndefined();
});
it('should rename *filtersrc* to *target* in filter transforms', function() {
var data = [{
transforms: [{
type: 'filter',
filtersrc: 'y'
}, {
type: 'filter',
operation: '<'
}]
}, {
transforms: [{
type: 'filter',
target: 'y'
}]
}];
Plotly.plot(gd, data);
var trace0 = gd.data[0];
var trace1 = gd.data[1];
expect(trace0.transforms.length).toEqual(2);
expect(trace0.transforms[0].filtersrc).toBeUndefined();
expect(trace0.transforms[0].target).toEqual('y');
expect(trace1.transforms.length).toEqual(1);
expect(trace1.transforms[0].target).toEqual('y');
});
it('should rename *calendar* to *valuecalendar* in filter transforms', function() {
var data = [{
transforms: [{
type: 'filter',
target: 'y',
calendar: 'hebrew'
}, {
type: 'filter',
operation: '<'
}]
}, {
transforms: [{
type: 'filter',
valuecalendar: 'jalali'
}]
}];
Plotly.plot(gd, data);
var trace0 = gd.data[0];
var trace1 = gd.data[1];
expect(trace0.transforms.length).toEqual(2);
expect(trace0.transforms[0].calendar).toBeUndefined();
expect(trace0.transforms[0].valuecalendar).toEqual('hebrew');
expect(trace1.transforms.length).toEqual(1);
expect(trace1.transforms[0].valuecalendar).toEqual('jalali');
});
it('should cleanup annotations / shapes refs', function() {
var data = [{}];
var layout = {
annotations: [
{ ref: 'paper' },
null,
{ xref: 'x02', yref: 'y1' }
],
shapes: [
{ xref: 'y', yref: 'x' },
null,
{ xref: 'x03', yref: 'y1' }
]
};
Plotly.plot(gd, data, layout);
expect(gd.layout.annotations[0]).toEqual({ xref: 'paper', yref: 'paper' });
expect(gd.layout.annotations[1]).toEqual(null);
expect(gd.layout.annotations[2]).toEqual({ xref: 'x2', yref: 'y' });
expect(gd.layout.shapes[0].xref).toBeUndefined();
expect(gd.layout.shapes[0].yref).toBeUndefined();
expect(gd.layout.shapes[1]).toEqual(null);
expect(gd.layout.shapes[2].xref).toEqual('x3');
expect(gd.layout.shapes[2].yref).toEqual('y');
});
it('removes direction names and showlegend from finance traces', function() {
var data = [{
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: true,
name: 'Yeti goes up'
},
decreasing: {
showlegend: 'legendonly',
name: 'Yeti goes down'
},
name: 'Snowman'
}, {
type: 'candlestick', open: [1], high: [3], low: [0], close: [2],
increasing: {
name: 'Bigfoot'
},
decreasing: {
showlegend: false,
name: 'Biggerfoot'
},
name: 'Nobody'
}, {
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
name: 'Batman'
},
decreasing: {
showlegend: true
},
name: 'Robin'
}, {
type: 'candlestick', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: false,
},
decreasing: {
name: 'Fred'
}
}, {
type: 'ohlc', open: [1], high: [3], low: [0], close: [2],
increasing: {
showlegend: false,
name: 'Gruyere heating up'
},
decreasing: {
showlegend: false,
name: 'Gruyere cooling off'
},
name: 'Emmenthaler'
}];
Plotly.plot(gd, data);
// Even if both showlegends are false, leave trace.showlegend out
// My rationale for this is that legends are sufficiently different
// now that it's worthwhile resetting their existence to default
gd.data.forEach(function(trace) {
expect(trace.increasing.name).toBeUndefined();
expect(trace.increasing.showlegend).toBeUndefined();
expect(trace.decreasing.name).toBeUndefined();
expect(trace.decreasing.showlegend).toBeUndefined();
});
// Both directions have names: ignore trace.name, as it
// had no effect on the output previously
// Ideally 'Yeti goes' would be smart enough to truncate
// at 'Yeti' but I don't see how to do that...
expect(gd.data[0].name).toBe('Yeti goes');
// One direction has empty or hidden name so use the other
// Note that even '' in both names would render trace.name impact-less
expect(gd.data[1].name).toBe('Bigfoot');
// One direction has a name but trace.name is there too:
// just use trace.name
expect(gd.data[2].name).toBe('Robin');
// No trace.name, only one direction name: use the direction name
expect(gd.data[3].name).toBe('Fred');
// both names exist but hidden from the legend: still look for common prefix
expect(gd.data[4].name).toBe('Gruyere');
});
});

at some point, but I won't do this in this PR.

Copy link
Contributor

@archmoj archmoj Jan 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply opened #3419 in this regard...

"colorbar": {
"title": {"text": "streamtube"},
"x": -0.15,
"len": 0.3,
"y": 0,
"yanchor": "bottom"
}
}],
"layout": {
"title": {"text": "gl3d trace with reversescale:true"},
"scene": {
"camera": {
"eye": {"x": -2, "y": -0.35, "z": 0.34}
}
}
}
}