-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Volume traces #2753
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.
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
Volume traces #2753
Conversation
role: 'info', | ||
editType: 'calc', | ||
description: 'Sets the opacity scale of the volume, which opacity to use for which intensity. Array of 256 values in 0..1 range.' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mapping over the same range as colorscale
? If so, can we specify it similarly, as a piecewise-linear list of pairs [[0, opacity0], [v1, opacity1], ..., [1, opacityN]]
?
Is this used along with trace.opacity
? Like the two are multiplied together perhaps? However that works it should be in the description.
role: 'info', | ||
editType: 'calc', | ||
description: 'Sets the opacity of the volume.' | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opacity
is a standard attribute for all traces (unless you disable it with 'noOpacity'
in module.categories
) so you shouldn't need this attribute, nor the coerce('opacity')
, nor even trace.opacity === undefined ? 1 : trace.opacity
- you should just be able to do volumeOpts.opacity = trace.opacity;
since opacity defaults to 1.
src/traces/volume/defaults.js
Outdated
coerce('opacityscale'); | ||
|
||
coerce('boundmin'); | ||
coerce('boundmax'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to isosurface, could we change these to (x|y)(min|max)
? Doesn't look like these are implemented yet, and it doesn't feel to me as though they're as important here as in isosurface, so I'd be OK with removing them for the first version of this trace type.
… and make it colorscale-like
src/traces/volume/convert.js
Outdated
@@ -161,8 +182,6 @@ function convert(gl, scene, trace) { | |||
trace.imax === undefined ? trace.cmax : trace.imax | |||
]; | |||
|
|||
volumeOpts.opacity = trace.opacity === undefined ? 1 : trace.opacity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still set this, if it'll get used downstream - which it looks like it will based on your amended description for opacityscale
, thanks for fleshing that out!
My point was just that you don't need to handle the undefined
case here, since the global trace.opacity
attribute has a default of 1 you know at this point that you can only have a number between 0 and 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, right, thanks for catching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I wasn't able to test @kig's volume
on my laptop.
Punching in
var width = 128
var height = 128
var depth = 128
var xs = []
var ys = []
var zs = []
var data = []
for (var z=0; z<depth; z++)
for (var y=0; y<height; y++)
for (var x=0; x<width; x++) {
xs.push(2*x/width);
ys.push(3*y/height);
zs.push(z/depth);
var value = Math.pow(Math.abs((10000 + (0.5+Math.random())*500 * (
Math.sin(2 * 2*Math.PI*(z/depth-0.5)) +
Math.cos(3 * 2*Math.PI*(x*x/(width*width)-0.5)) +
Math.sin(4 * 2*Math.PI*(y*z/(height*depth)-0.5))
)) * Math.pow(z/depth,1/3) * (1-Math.sqrt(x*x / width*width + y*y / height*height)) % 500000)/1e6, 2);
data[z*height*width + y*width + x] = value
}
var opacityscale = []
for (var i=0; i<16; i++) {
opacityscale[i] = [i/15, Math.pow(i/15, 1.2)]
}
Plotly.newPlot(gd, [{
type: 'volume',
x: xs,
y: ys,
z: zs,
value: data,
cmin: 0.05,
cmax: 0.22,
imin: 0.05,
imax: 0.25,
opacity: 0.05,
colorscale: 'Portland',
opacityscale: opacityscale
}])
gives me a blank scene and:
Applying a patch similar to gl-vis/gl-cone3d@a9e5bb3 does not seem to resolve the issue. @kig do these console errors look familiar to you?
Moreover, @kig , would you be interested in adding 2 or 3 "data"/"layout" mocks in test/image/mocks/
and a few jasmine tests similar to:
- https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/streamtube_test.js and/or
- https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/cone_test.js
Volume trace don't have hover, so that's one less thing to worry about, but we should make sure autorange and Plotly.restyle
behave correctly.
src/traces/volume/convert.js
Outdated
return simpleMap(arr, function(v) { return ax.d2l(v) * scale; }); | ||
} | ||
|
||
var xs = getSequence(trace.x); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different / better than
plotly.js/src/traces/streamtube/convert.js
Lines 64 to 66 in 2a3dc26
function distinctVals(col) { | |
return Lib.distinctVals(col).vals; | |
} |
used in streamtubes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh, good to see. I'll change it to use Lib.distinctVals to have a single reference for this kind of function.
FWIW, getSequence operates on an array of sequences sorted in ascending order and returns the unique values of the first sequence. E.g. [1,1,2,2,3,3,1,1,2,2,3,3] -> [1,2,3]. So it is a bit more performant for this kind of data (no need to sort, can bail out after the first sequence), but I don't think it's worth the maintenance burden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/traces/volume/attributes.js
Outdated
].join(' ') | ||
}, | ||
|
||
value: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed this in a couple months ago, but thinking about this again, I think values
(n.b. plural) would be better. It would match values
attributes in pie, splom and parcoords traces.
src/traces/volume/convert.js
Outdated
var proto = Volume.prototype; | ||
|
||
proto.handlePick = function(selection) { | ||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just return
here if we're not going to implement hover for volume
traces.
src/traces/volume/attributes.js
Outdated
].join(' ') | ||
}, | ||
|
||
imin: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget. Did we settle on imin
/ imax
in the past? If not, I think I would prefer vmin
and vmax
to match the v in values
.
We should also explain how this pair of attributes is related to cmin
/ cmax
volumeOpts.isoBounds = [trace.cmin, trace.cmax];
volumeOpts.intensityBounds = [
trace.imin === undefined ? trace.cmin : trace.imin,
trace.imax === undefined ? trace.cmax : trace.imax
];
in the attribute description
.
src/traces/volume/attributes.js
Outdated
attrs.hoverinfo = extendFlat({}, baseAttrs.hoverinfo, { | ||
editType: 'calc', | ||
flags: ['x', 'y', 'z', 'intensity', 'text', 'name'], | ||
dflt: 'x+y+z+intensity+text+name' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove hoverinfo
completely, as we're not implementing a hover handler in this first iteration.
@etpinard The gl-texture2d failure is likely because a 128x128x128 volume tries to create a 16kpx high texture. 64x64x64 should work. I'm working on a proper fix to this issue by laying out the slices on a grid (and eventually using that as a raymarchable volume). |
Awesome 👌 |
@archmoj could you try pulling down this branch, and I'd like to confirm I'm not the only one. |
@etpinard Yes I am getting the same errors on Ubuntu & Windows too. |
Volume trace
@etpinard @alexcjohnson
Volume rendering trace for plotly.js. Renders 3D volumes as stacks of translucent images. Integration work in progress.
Asymmetric volume dimensions seem buggy, try with 64x32x64 for exampleHover?