Skip to content

Cone sizeref fixes #2715

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 9 commits into from
Jun 11, 2018
Merged

Cone sizeref fixes #2715

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8619ce1
use gl-cone3d picking instead of gl-scatter3d overlay
etpinard Jun 8, 2018
6257642
[tmp] use gl-cone3d from https://github.com/gl-vis/gl-cone3d/pull/12
etpinard Jun 8, 2018
4836861
try fixing cone 'absolute' scaling
etpinard Jun 8, 2018
bb4bde7
new cone baselines
etpinard Jun 7, 2018
67f372d
fixup cone tests
etpinard Jun 11, 2018
10475be
fixups in sizeref description
etpinard Jun 11, 2018
7236e2d
improve sizeref default for 0-norm cones
etpinard Jun 11, 2018
c8a40d5
bump gl-cone3d to 1.1.0
etpinard Jun 11, 2018
a1ba953
more sizemode/sizeref fixups
etpinard Jun 11, 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: 1 addition & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"es6-promise": "^3.0.2",
"fast-isnumeric": "^1.1.1",
"font-atlas-sdf": "^1.3.3",
"gl-cone3d": "^v1.0.1",
"gl-cone3d": "[email protected]:gl-vis/gl-cone3d#3b3ef9d465a8e30d14a9a4a1cc88d4deebec64a9",
"gl-contour2d": "^1.1.4",
"gl-error3d": "^1.0.7",
"gl-heatmap2d": "^1.0.4",
Expand Down
20 changes: 13 additions & 7 deletions src/traces/cone/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,26 @@ var attrs = {
editType: 'calc',
dflt: 'scaled',
description: [
'Sets the mode by which the cones are sized.',
'If *scaled*, `sizeref` scales such that the reference cone size',
'for the maximum vector magnitude is 1.',
'If *absolute*, `sizeref` scales such that the reference cone size',
'for vector magnitude 1 is one grid unit.'
'Determines whether `sizeref` is set as a *scaled* (i.e unitless) scalar',
'(normalized by the max u/v/w norm in the vector field) or as',
'*absolute* value (in unit of velocity).'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't use the word "velocity" here - you can show many other quantities as vector fields (electric fields, as just one common example). Just say "in the same units as the vector field" or some such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, good call!

].join(' ')
},
sizeref: {
valType: 'number',
role: 'info',
editType: 'calc',
min: 0,
dflt: 1,
description: 'Sets the cone size reference value.'
description: [
'Adjusts the cone size scaling.',
'The size of the cones is determined by their u/v/w norm multiplied a factor and `sizeref`.',
'This factor (computed internally) corresponds to the minimum "time" to travel across two',
'two successive x/y/z positions at the average velocity of those two successive positions',
Copy link
Collaborator

Choose a reason for hiding this comment

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

two twos is one two too many

'All cones in a given trace use the same factor.',
'With `sizemode` set to *scaled*, `sizeref` is unitless, its default value is *0.5*',
'With `sizemode` set to *absolute*, `sizeref` has units of velocity, its the default value is',
'half the sample\'s maximum vector norm.'
].join(' ')
},

anchor: {
Expand Down
38 changes: 12 additions & 26 deletions src/traces/cone/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

'use strict';

var createScatterPlot = require('gl-scatter3d');
var conePlot = require('gl-cone3d');
var createConeMesh = require('gl-cone3d').createConeMesh;

Expand All @@ -19,14 +18,13 @@ function Cone(scene, uid) {
this.scene = scene;
this.uid = uid;
this.mesh = null;
this.pts = null;
this.data = null;
}

var proto = Cone.prototype;

proto.handlePick = function(selection) {
if(selection.object === this.pts) {
if(selection.object === this.mesh) {
var selectIndex = selection.index = selection.data.index;
var xx = this.data.x[selectIndex];
var yy = this.data.y[selectIndex];
Expand Down Expand Up @@ -61,7 +59,6 @@ function zip3(x, y, z) {
}

var axisName2scaleIndex = {xaxis: 0, yaxis: 1, zaxis: 2};
var sizeMode2sizeKey = {scaled: 'coneSize', absolute: 'absoluteConeSize'};
var anchor2coneOffset = {tip: 1, tail: 0, cm: 0.25, center: 0.5};
var anchor2coneSpan = {tip: 1, tail: 1, cm: 0.75, center: 0.5};

Expand Down Expand Up @@ -90,17 +87,21 @@ function convert(scene, trace) {

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

coneOpts[sizeMode2sizeKey[trace.sizemode]] = trace.sizeref;
coneOpts.coneOffset = anchor2coneOffset[trace.anchor];

var meshData = conePlot(coneOpts);
if(trace.sizemode === 'scaled') {
// unitless sizeref
coneOpts.coneSize = trace.sizeref || 0.5;
} else {
// sizeref here has unit of velocity
coneOpts.coneSize = (trace.sizeref / trace._normMax) || 0.5;
Copy link
Contributor Author

@etpinard etpinard Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Previously, coneSize under sizemode: 'absolute' was scaled using the max norm of the "scaled" u/v/w vectors (that same dataScale than in #2646). This made sizeref under sizemode: 'absolute' had little meaning.

Now, sizeref under sizemode: 'absolute' is scaled by the "input" u/v/w max norm should make this thing a little less confusing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Two questions:

  • When is the || 0.5 (in either case) necessary?
  • What if the field is uniformly zero? then _normMax will be 0 and coneSize will be Infinity... which I suppose is OK if the end result is no errors and nothing drawn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the field is uniformly zero?

Oh right, Infinity is truthy. Good call!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When is the || 0.5 (in either case) necessary?

Currently, the sizeref attribute declaration does not set a dflt field. In practice I guess it does have a hard default of 0.5. But technically under the sizemode: 'absolute', the sizemode default is 0.5 * trace._normMax.

}

// stash positions for gl-scatter3d 'hover' trace
meshData._pts = coneOpts.positions;
var meshData = conePlot(coneOpts);

// pass gl-mesh3d lighting attributes
meshData.lightPosition = [trace.lightposition.x, trace.lightposition.y, trace.lightposition.z];
var lp = trace.lightposition;
meshData.lightPosition = [lp.x, lp.y, lp.z];
meshData.ambient = trace.lighting.ambient;
meshData.diffuse = trace.lighting.diffuse;
meshData.specular = trace.lighting.specular;
Expand All @@ -109,8 +110,7 @@ function convert(scene, trace) {
meshData.opacity = trace.opacity;

// stash autorange pad value
trace._pad = anchor2coneSpan[trace.anchor] * meshData.vectorScale * trace.sizeref;
if(trace.sizemode === 'scaled') trace._pad *= trace._normMax;
trace._pad = anchor2coneSpan[trace.anchor] * meshData.vectorScale * meshData.coneScale * trace._normMax;

return meshData;
}
Expand All @@ -119,14 +119,10 @@ proto.update = function(data) {
this.data = data;

var meshData = convert(this.scene, data);

this.mesh.update(meshData);
this.pts.update({position: meshData._pts});
};

proto.dispose = function() {
this.scene.glplot.remove(this.pts);
this.pts.dispose();
this.scene.glplot.remove(this.mesh);
this.mesh.dispose();
};
Expand All @@ -137,21 +133,11 @@ function createConeTrace(scene, data) {
var meshData = convert(scene, data);
var mesh = createConeMesh(gl, meshData);

var pts = createScatterPlot({
gl: gl,
position: meshData._pts,
project: false,
opacity: 0
});

var cone = new Cone(scene, data.uid);
cone.mesh = mesh;
cone.pts = pts;
cone.data = data;
mesh._trace = cone;
pts._trace = cone;

scene.glplot.add(pts);
scene.glplot.add(mesh);

return cone;
Expand Down
4 changes: 2 additions & 2 deletions test/jasmine/tests/cone_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,15 @@ describe('@gl Test cone interactions', function() {
var scene = gd._fullLayout.scene._scene;
var objs = scene.glplot.objects;

expect(objs.length).toBe(4);
expect(objs.length).toBe(2);

return Plotly.deleteTraces(gd, [0]);
})
.then(function() {
var scene = gd._fullLayout.scene._scene;
var objs = scene.glplot.objects;

expect(objs.length).toBe(2);
expect(objs.length).toBe(1);

return Plotly.deleteTraces(gd, [0]);
})
Expand Down