Skip to content

Make polar be more graceful on non sense inputs #3021

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
merged 7 commits into from
Sep 18, 2018
Merged

Make polar be more graceful on non sense inputs #3021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f0fc26
don't attempt to draw radial axis when *polar.hole* is set to 1
etpinard Sep 14, 2018
8589d99
no need to draw inner sector when range[0] is clamped to origin
etpinard Sep 17, 2018
a8838da
draw sectors that span more than 2pi as 'full' circle
etpinard Sep 17, 2018
35d1351
id should be a string not a length-1 array
etpinard Sep 17, 2018
b3fc904
try adding @noCI to plotly_webglcontextlost test
etpinard Sep 17, 2018
d9d626a
tag new select test as @flaky
etpinard Sep 17, 2018
cb5b336
add hole=1 subplot to polar_blank mock
etpinard Sep 17, 2018
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
4 changes: 3 additions & 1 deletion src/lib/angles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ function rad2deg(rad) { return rad / PI * 180; }
* is sector a full circle?
* ... this comes up a lot in SVG path-drawing routines
*
* N.B. we consider all sectors that span more that 2pi 'full' circles
*
* @param {2-item array} aBnds : angular bounds in *radians*
* @return {boolean}
*/
function isFullCircle(aBnds) {
return Math.abs(Math.abs(aBnds[1] - aBnds[0]) - twoPI) < 1e-15;
return Math.abs(aBnds[1] - aBnds[0]) > twoPI - 1e-15;
}

/**
Expand Down
55 changes: 35 additions & 20 deletions src/plots/polar/polar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ proto.updateRadialAxis = function(fullLayout, polarLayout) {
var radialLayout = polarLayout.radialaxis;
var a0 = mod(polarLayout.sector[0], 360);
var ax = _this.radialAxis;
var hasRoomForIt = innerRadius < radius;

_this.fillViewInitialKey('radialaxis.angle', radialLayout.angle);
_this.fillViewInitialKey('radialaxis.range', ax.range.slice());
Expand Down Expand Up @@ -410,8 +411,10 @@ proto.updateRadialAxis = function(fullLayout, polarLayout) {
_this.radialTickLayout = newTickLayout;
}

ax.setScale();
doTicksSingle(gd, ax, true);
if(hasRoomForIt) {
ax.setScale();
doTicksSingle(gd, ax, true);
}

// stash 'actual' radial axis angle for drag handlers (in degrees)
var angle = _this.radialAxisAngle = _this.vangles ?
Expand All @@ -420,24 +423,31 @@ proto.updateRadialAxis = function(fullLayout, polarLayout) {

var trans = strTranslate(cx, cy) + strRotate(-angle);

updateElement(layers['radial-axis'], radialLayout.showticklabels || radialLayout.ticks, {
transform: trans
});
updateElement(
layers['radial-axis'],
hasRoomForIt && (radialLayout.showticklabels || radialLayout.ticks),
{transform: trans}
);

// move all grid paths to about circle center,
// undo individual grid lines translations
updateElement(layers['radial-grid'], radialLayout.showgrid, {
transform: strTranslate(cx, cy)
})
.selectAll('path').attr('transform', null);

updateElement(layers['radial-line'].select('line'), radialLayout.showline, {
x1: innerRadius,
y1: 0,
x2: radius,
y2: 0,
transform: trans
})
updateElement(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recall that updateElement is

plotly.js/src/plots/polar/polar.js

Lines 1347 to 1355 in 7f0fc26

function updateElement(sel, showAttr, attrs) {
if(showAttr) {
sel.attr('display', null);
sel.attr(attrs);
} else if(sel) {
sel.attr('display', 'none');
}
return sel;
}

layers['radial-grid'],
hasRoomForIt && radialLayout.showgrid,
{transform: strTranslate(cx, cy)}
).selectAll('path').attr('transform', null);

updateElement(
layers['radial-line'].select('line'),
hasRoomForIt && radialLayout.showline,
{
x1: innerRadius,
y1: 0,
x2: radius,
y2: 0,
transform: trans
}
)
.attr('stroke-width', radialLayout.linewidth)
.call(Color.stroke, radialLayout.linecolor);
};
Expand Down Expand Up @@ -802,7 +812,8 @@ proto.updateMainDrag = function(fullLayout) {
var cpath;

if(clampAndSetR0R1(rr0, rr1)) {
path1 = path0 + _this.pathSector(r1) + _this.pathSector(r0);
path1 = path0 + _this.pathSector(r1);
if(r0) path1 += _this.pathSector(r0);
// keep 'starting' angle
cpath = pathCorner(r0, a0) + pathCorner(r1, a0);
}
Expand All @@ -827,7 +838,8 @@ proto.updateMainDrag = function(fullLayout) {
var cpath;

if(clampAndSetR0R1(rr0, rr1)) {
path1 = path0 + _this.pathSector(r1) + _this.pathSector(r0);
path1 = path0 + _this.pathSector(r1);
if(r0) path1 += _this.pathSector(r0);
// keep 'starting' angle here too
cpath = [
pathCornerForPolygons(r0, vangles0[0], vangles0[1]),
Expand Down Expand Up @@ -963,7 +975,10 @@ proto.updateRadialDrag = function(fullLayout, polarLayout, rngIndex) {

var radialDrag = dragBox.makeRectDragger(layers, className, 'crosshair', -bl2, -bl2, bl, bl);
var dragOpts = {element: radialDrag, gd: gd};
d3.select(radialDrag).attr('transform', strTranslate(tx, ty));

updateElement(d3.select(radialDrag), radialAxis.visible && innerRadius < radius, {
transform: strTranslate(tx, ty)
});

// move function (either rotate or re-range flavor)
var moveFn2;
Expand Down
2 changes: 1 addition & 1 deletion src/plots/polar/set_convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ function setConvertAngular(ax, polarLayout) {
// Set the angular range in degrees to make auto-tick computation cleaner,
// changing rotation/direction should not affect the angular tick value.
ax.range = Lib.isFullCircle(sectorInRad) ?
sector.slice() :
[sector[0], sector[0] + 360] :
sectorInRad.map(g2rad).map(rad2deg);
break;

Expand Down
2 changes: 1 addition & 1 deletion src/traces/splom/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ function handleAxisDefaults(traceIn, traceOut, layout, coerce) {
// fill in splom subplot keys
for(i = 0; i < axLength; i++) {
for(j = 0; j < axLength; j++) {
var id = [xaxes[i] + yaxes[j]];
var id = xaxes[i] + yaxes[j];

if(i > j && showUpper) {
layout._splomSubplots[id] = 1;
Expand Down
Binary file modified test/image/baselines/polar_blank.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.
13 changes: 13 additions & 0 deletions test/image/mocks/polar_blank.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@
"text": "<b>N.B.</b><br>radial<br>auotrange<br>for (0,0)<br>gives [0,1]",
"textposition": "left",
"subplot": "polar4"
}, {
"type": "scatterpolar",
"r": [1, 2, 3],
"theta": [0, 90, 200],
"subplot": "polar5"
}],
"layout": {
"polar": {
Expand Down Expand Up @@ -69,6 +74,14 @@
},
"radialaxis": {"visible": false}
},
"polar5": {
"domain": {
"x": [0.12, 0.34],
"y": [0.11, 0.34]
},
"hole": 1
},
"showlegend": false,
"width": 600,
"height": 500
}
Expand Down
2 changes: 1 addition & 1 deletion test/image/mocks/polar_sector.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,7 @@
"polar20": {
"sector": [
-180,
180
200
],
"bgcolor": "#d3d3d3",
"domain": {
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/gl2d_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ describe('Test gl plot side effects', function() {
.then(done);
});

it('@gl should fire *plotly_webglcontextlost* when on webgl context lost', function(done) {
it('@noCI @gl should fire *plotly_webglcontextlost* when on webgl context lost', function(done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4 successes in 4 test-jasmine2 runs with this @noCI

var _mock = Lib.extendDeep({}, require('@mocks/gl2d_12.json'));

function _trigger(name) {
Expand Down
32 changes: 32 additions & 0 deletions test/jasmine/tests/polar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ var doubleClick = require('../assets/double_click');
var drag = require('../assets/drag');
var delay = require('../assets/delay');

var customAssertions = require('../assets/custom_assertions');
var assertNodeDisplay = customAssertions.assertNodeDisplay;

describe('Test legacy polar plots logs:', function() {
var gd;

Expand Down Expand Up @@ -628,6 +631,35 @@ describe('Test relayout on polar subplots:', function() {
.catch(failTest)
.then(done);
});

it('should not attempt to draw radial axis when *polar.hole* is set to 1', function(done) {
var gd = createGraphDiv();

var queries = [
'.radial-axis', '.radial-grid', '.radial-line > line',
'.radialdrag', '.radialdrag-inner'
];

function _assert(msg, showing) {
var exp = showing ? null : 'none';
var sp3 = d3.select(gd).select('.polarlayer > .polar');
queries.forEach(function(q) {
assertNodeDisplay(sp3.selectAll(q), [exp], msg + ' ' + q);
});
}

Plotly.plot(gd, [{
type: 'scatterpolar',
r: ['a', 'b', 'c']
}])
.then(function() { _assert('base', true); })
.then(function() { return Plotly.relayout(gd, 'polar.hole', 1); })
.then(function() { _assert('hole=1', false); })
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it doesn't fail... does it work? ie draws all data on the rim regardless of radial value (within the radial range)? Do you want to include this in a mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's hole=1 subplot:

peek 2018-09-17 17-11

where angular drag still drag still works.

ie draws all data on the rim regardless of radial value (within the radial range)?

it shows no data points.

Do you want to include this in a mock?

done in cb5b336

Copy link
Collaborator

Choose a reason for hiding this comment

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

it shows no data points.

This comment is nonblocking (perhaps add to #2255 if you don't want to address it here):

With cliponaxis: false (which is the default anyway for polar) I would have expected it to show the data - and it currently doesn't, even if you explicitly specify a range. The argument for this behavior is to have hole: 1 match hole: 0.9999 as closely as possible. Perhaps not useful, though I guess I could see someone wanting to just show where their data is on an angular scale without involving a radial scale at all. That said they can always just use hole: 0.9999 as a workaround, so it's not really any missing functionality... but it feels hacky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The argument for this behavior is to have hole: 1 match hole: 0.9999 as closely as possible.

Ah I see. Good point.

Perhaps not useful, though I guess I could see someone wanting to just show where their data is on an angular scale without involving a radial scale at all

I'll wait until someone complains about it (maybe @nicolaskruchten ?) before addressing this. Now in -> #2255 (comment)

.then(function() { return Plotly.relayout(gd, 'polar.hole', 0.2); })
.then(function() { _assert('hole=0.2', true); })
.catch(failTest)
.then(done);
});
});

describe('Test polar interactions:', function() {
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/select_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,7 +1296,7 @@ describe('Test select box and lasso in general:', function() {
.then(done);
});

it('should cleanly clear and restart selections on double click when add/subtract mode on', function(done) {
it('@flaky should cleanly clear and restart selections on double click when add/subtract mode on', function(done) {
var gd = createGraphDiv();
var fig = Lib.extendDeep({}, require('@mocks/0.json'));

Expand Down