Better polar setConvert + a few misc polar touch ups #2895

etpinard · 2018-08-10T22:01:28Z

This is a preliminary PR for barpolar.

I dove back hard into the polar code in the past 10 days and I noticed a few hacks that were worth cleaning up before adding barpolar.

Commit 3d8eef2 is the big one. In brief, it locks down what 'c'alc means for angular coordinates and adds a new coordinate system (I called it 'g'eometric) that depends on things that interactions can update (i.e. radialaxis.range and angularaxis.rotation). I invite you to read the setConvertPolar docstring as well as commit 91064ca for more info. It's still not perfect: ideally we should try to factor things in Axes.doTicks to no longer rely on 2 radial and 2 angular axis objects per polar subplots, but this feels like a step in the right direction.

After that, this PR also:

adds support r0/dr and theta0/dtheta. I decided to compute the dtheta default using the angular period and the number of r coordinates in the trace - which seems to give pretty user-friendly results and will be useful for barpolar.
fixes scatterpolargl in 'lines' mode smears on axis rotation #2888
adds on-graph text to scatterpolargl following-up on Enable text for scattergl trace #2737
🔪 un-implemented hoveron from the scattergl and scatterpolargl attributes as discussed in Implement hoveron: 'fills' for scattergl and scatterpolargl #2887 (comment)

cc @alexcjohnson

- 🔪 one obsolete comment - use trace.orientation instead using same x/y logic as in supplyDefaults.

- introduce 'g'eomtric coordinate system, that handles angular rotation / directrion and radial range shifts - add 't'icks coord system for angular axes tick vals - add setGeometry (which sets the c2g converters) - make 'c' linear theta coordinates be in radians, eliminating trace.thetaunit deps after 'calc'

... which is on-par with fullLayout.polar.angularaxis

- where the dtheta default is the ax.period divided by the trace length (given by the length of the 'other' coordinate).

... until we actually implement it in #2887

- follow-up from #2737

alexcjohnson · 2018-08-13T15:14:53Z

src/traces/bar/calc.js


-    var xa = Axes.getFromId(gd, trace.xaxis || 'x'),
-        ya = Axes.getFromId(gd, trace.yaxis || 'y'),
-        orientation = trace.orientation || ((trace.x && !trace.y) ? 'h' : 'v'),


Nice catch!

alexcjohnson · 2018-08-13T16:04:37Z

src/plots/polar/set_convert.js

+ * + for category axes:
+ *   - d2c (in calc) just like for cartesian axes
+ *   - c2g (in plot) category indices -> 'geometric' radians
+ *   - t2g (in updateAngularAxis) 'tick' value (as category indices) -> 'geometric' radians


This would be easier for me to understand if instead of describing t2g etc, just describe t and g (and c and d if they're different from cartesian) for the linear and category cases)

(like

plotly.js/src/plots/cartesian/set_convert.js

Lines 35 to 50 in def6aa5

* d: data, in whatever form it's provided

* c: calcdata: turned into numbers, but not linearized

* l: linearized - same as c except for log axes (and other nonlinear

* mappings later?) this is used when we need to know if it's

* *possible* to show some data on this axis, without caring about

* the current range

* p: pixel value - mapped to the screen with current size and zoom

* r: ranges, tick0, and annotation positions match one of the above

* but are handled differently for different types:

* - linear and date: data format (d)

* - category: calcdata format (c), and will stay that way because

* the data format has no continuous mapping

* - log: linearized (l) format

* TODO: in v2.0 we plan to change it to data format. At that point

* shapes will work the same way as ranges, tick0, and annotations

* so they can use this conversion too.

)

latest attempt in -> 4c11c84

Much clearer - thanks!

alexcjohnson · 2018-08-13T16:07:03Z

src/plots/polar/set_convert.js

+        var rot = deg2rad(ax.rotation);
+
+        ax.rad2g = function(v) { return dir * v + rot; };
+        ax.g2rad = function(v) { return (v - rot) / dir; };


As discussed in slack, seems like we can 🔪 ax.rad2* and ax.*2rad entirely now, so we don't need to think about the difference between rad and g to understand all this.

done in f5bceda

- ?2rad and rad2? methods are now gone - ax._period is gone

alexcjohnson · 2018-08-13T16:12:40Z

src/plots/polar/polar.js

+ * - this.yaxis
+ *   setup so that polar traces can reuse plot methods of Cartesian traces
+ *   which mostly rely on 2pixel methods (e.g ax.c2p)
+ */


Thanks! 📚

alexcjohnson · 2018-08-13T16:21:15Z

src/plots/polar/polar.js

@@ -257,8 +257,6 @@ proto.updateLayout = function(fullLayout, polarLayout) {
    _this.angularAxis = _this.mockAxis(fullLayout, polarLayout, angularLayout, {
        _axislayer: layers['angular-axis'],
        _gridlayer: layers['angular-grid'],
-        // angular axes need *special* logic
-        _id: 'angular',


for reference, here's where _id gets set to angularaxis

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

Line 57 in def6aa5

axOut._id = axOut._name = axName;

alexcjohnson · 2018-08-13T16:24:27Z

src/plots/cartesian/axes.js

@@ -554,7 +554,7 @@ axes.calcTicks = function calcTicks(ax) {

    // If same angle over a full circle, the last tick vals is a duplicate.
    // TODO must do something similar for angular date axes.
-    if(ax._id === 'angular' && Math.abs(rng[1] - rng[0]) === 360) {
+    if(ax._id === 'angularaxis' && Math.abs(rng[1] - rng[0]) === 360) {


This gets a lot of reuse... perhaps function isAngular(axid) { return axid === 'angularaxis'; }?

good call -> 0c517d1

alexcjohnson · 2018-08-13T16:25:11Z

src/plots/cartesian/axes.js

@@ -1682,7 +1682,7 @@ axes.doTicksSingle = function(gd, arg, skipTitle) {
    var valsClipped = vals.filter(clipEnds);

    // don't clip angular values
-    if(ax._id === 'angular') {
+    if(ax._id === 'angularaxis') {


not new in this PR, but I think you have an axid here.

went one step further in 0c517d1

alexcjohnson · 2018-08-13T19:04:11Z

src/traces/scatterpolar/attributes.js

+            'By default, the `dtheta` step equals the subplot\'s period divided',
+            'by the length of the `r` coordinates.'
+        ].join(' ')
+    },


One of the most underrated features, IMHO. Thanks for adding it!

alexcjohnson · 2018-08-13T19:31:52Z

src/traces/scattergl/index.js

@@ -302,6 +302,8 @@ function sceneUpdate(gd, subplot) {
            }
            if(scene.scatter2d) {
                clearViewport(scene.scatter2d, vp);
+            } else if(scene.line2d) {
+                clearViewport(scene.line2d, vp);


The first arg to clearViewport is just used to find the regl._gl context... and so I guess the reason select2d is off by itself is that select gets its own context separate from the main data context?

Might be clearer then to refactor this as something like

var anyComponent = scene.scatter2d || scene.line2d || (scene.glText || [])[0]; if(anyComponent) clearViewport(anyComponent, vp);

Which would also make it easier to add error2d (can that ever be the only thing drawn?) and fill2d to the lookup list, looks plausible that these also have bugs similar to #2888.

BTW, looking at what clearViewport does - ie clears a rectangle from the context - what happens if two polar subplots have overlapping bounding boxes? Like if you made a hexagonal packing of polar subplots or you do something funny with sectors, like you make one large pac-man subplot eating a smaller one?

what happens if two polar subplots have overlapping bounding boxes?

This happens -> #2797

Might be clearer then to refactor this as something like

Absolutely -> c3044e0

add error2d (can that ever be the only thing drawn?)

Traces with error bar will always have either an active scene.scatter2d or scene.line2d.

BTW, looking at what clearViewport does - ie clears a rectangle from the context -

We should take a look at how much of a perf gain is to clear only a fraction of the canvas on pan/select. If that's negligible, then I'm thinking we should be keeping track of a graph-wide regl-draw-queue instead, always clear the whole canvas and draw all regl parts on pan/select. This should make this a lot more manageable. cc @dy

Traces with error bar will always have either an active scene.scatter2d or scene.line2d.

Even if mode: 'none'? That works fine for scatter, and sometimes tries to work with scattergl #2900

fill works with mode: 'none' too.

Bulletproofed in 75786e4

alexcjohnson · 2018-08-13T19:49:07Z

src/plots/polar/polar.js

@@ -993,11 +993,13 @@ proto.updateRadialDrag = function(fullLayout, polarLayout) {
            var moduleCalcDataVisible = Lib.filterVisible(moduleCalcData);
            var _module = moduleCalcData[0][0].trace._module;
            var polarLayoutNow = gd._fullLayout[_this.id];
+            var isGL = Registry.traceIs(k, 'gl');
+
+            if(isGL && _this._scene) _this._scene.clear();


is it possible to not have _this._scene at this point if there's a gl trace?

BTW this would be clearer with traceType instead of k.

I don't see a problem at the moment because there's only one gl type that can be here, but for future reference seems like if we ever add another we'll have to make sure only the first one clears the scene.

Thanks for tip. Improvements in -> b81885d

alexcjohnson · 2018-08-13T19:54:18Z

test/jasmine/tests/polar_test.js

@@ -1136,7 +1143,9 @@ describe('Test polar interactions:', function() {
                fig.layout.margin = {l: 50, t: 50, b: 50, r: 50};

                if(s.patch) s.patch(fig);
-                nTraces = fig.data.length;
+                nTraces = fig.data.reduce(function(acc, trace) {
+                    return (trace.type === 'scatterpolargl') ? ++acc : acc;


hmm, OK. I always find prefix ++ confusing, and perhaps because it encourages stuff like that reduce also often comes out confusing to me. I realize it's better perf this way, but that's irrelevant to a test.
fig.data.filter(function(trace) { return trace.type === 'scatterpolargl'; }).length?

you're right, filter is better: b4700ca

... that is move scene.clear() out of trace-type loop

alexcjohnson · 2018-08-14T20:46:00Z

Nice work! 💪 💃

etpinard added 10 commits August 9, 2018 13:26

🔪 obsolete things in Bar.calc

e9c3ef8

- 🔪 one obsolete comment - use trace.orientation instead using same x/y logic as in supplyDefaults.

require doTicksSingle and setConvertCartesian directly

80717d5

🔪 now useless intoCenter line calcdata hack

87a3920

add 📚 about axes in polar subplots

91064ca

sub angularAxis._id 'angular' -> 'angularaxis'

3787bfa

... which is on-par with fullLayout.polar.angularaxis

add r0/dr theta0/dtheta

4f881fa

- where the dtheta default is the ax.period divided by the trace length (given by the length of the 'other' coordinate).

fix #2888 - clear scene via gl ref in line2d object

449dc43

🔪 hoveron declaration in scattergl and scatterpolargl

dae3ccd

... until we actually implement it in #2887

add on-graph 'text' to scatterpolargl

d3b3615

- follow-up from #2737

etpinard added bug something broken feature something new status: reviewable labels Aug 10, 2018

etpinard added this to the v1.40.0 milestone Aug 10, 2018

alexcjohnson reviewed Aug 13, 2018

View reviewed changes

fix radial drag when scatterpolar & scatterpolargl are on same subplot

ec57a42

alexcjohnson reviewed Aug 13, 2018

View reviewed changes

🔪 unused keys from angular axis objects

f5bceda

- ?2rad and rad2? methods are now gone - ax._period is gone

alexcjohnson reviewed Aug 13, 2018

View reviewed changes

etpinard added 3 commits August 13, 2018 16:37

use isAngular helper in axes.js

0c517d1

improve polar setConvert docstring

4c11c84

use .filter instead of (confusing) .reduce

b4700ca

etpinard added 3 commits August 13, 2018 17:02

clean up cleanViewport logic

c3044e0

avoid trying to call scene.clear more than once per mousemove

b81885d

... that is move scene.clear() out of trace-type loop

add fill2d and error2d in clearViewport fallback list

75786e4

etpinard merged commit 665a9c4 into master Aug 15, 2018

etpinard deleted the polar-better-set-convert branch August 15, 2018 14:18

etpinard mentioned this pull request Sep 6, 2018

Polar 2.0 open items #2255

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better polar setConvert + a few misc polar touch ups #2895

Better polar setConvert + a few misc polar touch ups #2895

etpinard commented Aug 10, 2018 •

edited

Loading

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018 •

edited

Loading

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 14, 2018

etpinard Aug 14, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson Aug 13, 2018

etpinard Aug 13, 2018

alexcjohnson commented Aug 14, 2018

	* d: data, in whatever form it's provided
	* c: calcdata: turned into numbers, but not linearized
	* l: linearized - same as c except for log axes (and other nonlinear
	* mappings later?) this is used when we need to know if it's
	* possible to show some data on this axis, without caring about
	* the current range
	* p: pixel value - mapped to the screen with current size and zoom
	* r: ranges, tick0, and annotation positions match one of the above
	* but are handled differently for different types:
	* - linear and date: data format (d)
	* - category: calcdata format (c), and will stay that way because
	* the data format has no continuous mapping
	* - log: linearized (l) format
	* TODO: in v2.0 we plan to change it to data format. At that point
	* shapes will work the same way as ranges, tick0, and annotations
	* so they can use this conversion too.

Better polar setConvert + a few misc polar touch ups #2895

Better polar setConvert + a few misc polar touch ups #2895

Conversation

etpinard commented Aug 10, 2018 • 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

etpinard Aug 13, 2018 • 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

alexcjohnson commented Aug 14, 2018

etpinard commented Aug 10, 2018 •

edited

Loading

etpinard Aug 13, 2018 •

edited

Loading