Skip to content
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

Isosurface traces #2752

Closed
wants to merge 17 commits into from
Closed

Isosurface traces #2752

wants to merge 17 commits into from

Conversation

kig
Copy link
Contributor

@kig kig commented Jun 25, 2018

Isosurface trace

isosurface_plotly

@etpinard @alexcjohnson

Isosurface trace for plotly.js integration WIP.

  • Add support for axis-aligned clipping planes to construct a clipped version of the isosurface
  • Should isocaps be a separate trace or part of the isosurface? Now the isosurface has isocaps turned on and merged into the isosurface mesh. The lib supports breaking them into two and having separate colorscales for each.
  • Figure out how to add intensity values to the hover info popup
  • How to pass in data, now it's following the streamtube plot method of passing in x, y, z and u arrays, then constructing a meshgrid out of the x,y,z -values and using that as the coords for the u array of intensities. Is this ok?
  • make plotly.js compute the trace bounds. They shouldn't be a user setting.

@alexcjohnson
Copy link
Contributor

Looking good! @etpinard will likely have more to say when he's back tomorrow, but some thoughts of my own to start:

Add support for axis-aligned clipping planes to construct a clipped version of the isosurface

👍 the simplest way to do this would be 6 attributes (x|y|z)(min|max) and that might be the best in fact - we could do xrange: [min, max] or even range: {x: [min, max]} but those all seem like they make it harder to do the most common thing, which is to add a single clipping plane so you can see the data there.

Should isocaps be a separate trace or part of the isosurface? Now the isosurface has isocaps turned on and merged into the isosurface mesh. The lib supports breaking them into two and having separate colorscales for each.

Part of the same trace. My gut reaction is the isosurfaces should by default be colored according to the isocap colorscale, but that you should be free to provide separate colors for each isosurface. What do you mean by "separate colorscales"? Each isosurface just gets a single color, right?

How to pass in data, now it's following the streamtube plot method of passing in x, y, z and u arrays, then constructing a meshgrid out of the x,y,z -values and using that as the coords for the u array of intensities. Is this ok?

I think the input data format is good, though u I might rename to v or value - Or I guess intensity for consistency with mesh3d but I don't particularly like that attribute, I wish we had used value there.

I'm not sure what you mean by "constructing a meshgrid out of the x,y,z -values and using that as the coords for the u array" though - won't the meshgrid you actually display be some sort of interpolation between the provided (x, y, z) values to find the isovalue?

valType: 'number',
editType: 'calc',
description: 'Sets the maximum iso bound of the isosurface.'
},
Copy link
Contributor

Choose a reason for hiding this comment

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

imax/imin is OK if we rename u to intensity... but if it turns to v or value these should be vmax/vmin. I presume that if you only provide one of these, the other one is treated as +/-Infinity for the purpose of constructing the isocaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember us having the same concerns about intensity in surface traces. We ended up naming that attribute surfacecolor.

I think naming this field u or v would confusing as it is an array of scalars, not a component of a vector field. I like value, but maybe volume would better (that's how MATLAB seems to call this field)?

Copy link
Contributor

Choose a reason for hiding this comment

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

About imin and imax, MATLAB calls it isovalue. Wouldn't isomin and isomax be better?

role: 'info',
editType: 'calc+clearAxisTypes',
description: [
'Sets the x coordinates of the isosurface'
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps "Sets the x coordinates of the volume data" (from which the isosurface will be calculated)


// var bounds = [
// isosurfaceOpts.boundmin || [xs[0], ys[0], zs[0]],
// isosurfaceOpts.boundmax || [xs[xs.length - 1], ys[ys.length - 1], zs[zs.length - 1]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see, these were to be the (x,y,z) limits... yeah, I still think it's better to make 6 separate attributes just since setting a single bound is (I think) the most common use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do, (x|y|z)(min|max)

isosurfaceOpts.isoBounds = [trace.imin, trace.imax];

isosurfaceOpts.isoCaps = trace.isocaps;
isosurfaceOpts.singleMesh = trace.singlemesh === undefined ? true : trace.singlemesh;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does singlemesh do? BTW logic like this belongs as attributes.singlemesh.dlft = true.

@etpinard
Copy link
Contributor

Thanks for the PRs @kig! Looking great 🎉

Unfortunately, I won't have time to look at them in detail until we release 1.39.0.

Now, would you be interested in looking at the broken tubes we noticed in #2658 (comment) to help us merge the streamtube PR?

@etpinard
Copy link
Contributor

etpinard commented Jul 10, 2018

For @alexcjohnson, who wonders what would happen when we try plot data that cross from >imax to <imin with no values between:

    var width = 64
    var height = 64
    var depth = 64
    var isomin = 1600
    var isomax = 2000

    var xs = []
    var ys = []
    var zs = []

    var data = new Uint16Array(width*height*depth)
    var k = 0
    for (var z=0; z<depth; z++)
    for (var y=0; y<height; y++)
    for (var x=0; x<width; x++) {
        xs.push(x/width)
        ys.push(y/height)
        zs.push(z/depth)
        data[k] = x < width/2 ? isomax+1 : isomin-1
	k++
    }

    Plotly.newPlot(gd, [{
      type: 'isosurface',

      x: xs,
      y: ys,
      z: zs,

      value: data,

      isomin: isomin,
      isomax: isomax,
      cmin: isomin - 100,
      cmax: isomax + 100,

      smoothnormals:  true,
      isocaps: true,

      singlemesh: true,

      colorscale: 'Portland',
      capscolorscale: 'Jet'
    }], {
      scene: {
        xaxis: {range: [0, 1]},
        yaxis: {range: [0, 1]},
        zaxis: {range: [0, 1]}
      }
    })

gives a blank scene.


For comparison with

data[k] = x < width/2 ? isomax : isomin

and every else the same, we get:

image

@etpinard
Copy link
Contributor

In a private convo, @alexcjohnson wrote:

It looks like it [i.e. gl-isosurface3d] is picking the closest grid points inside the min/max range, and making a surface from those points, rather than interpolating to the actual positions of the min and max values.

@kig is this in fact the current behavior?

@kig
Copy link
Contributor Author

kig commented Aug 2, 2018

Should isocaps be a separate trace or part of the isosurface? Now the isosurface has isocaps turned on and merged into the isosurface mesh. The lib supports breaking them into two and having separate colorscales for each.

Part of the same trace. My gut reaction is the isosurfaces should by default be colored according to the isocap colorscale, but that you should be free to provide separate colors for each isosurface. What do you mean by "separate colorscales"? Each isosurface just gets a single color, right?

In some traces, it's nice to have a separate colorscale for the caps. E.g. https://gl-vis.github.io/gl-isosurface3d/smooth_skin.html

isosurface_diff_colorscale

@kig
Copy link
Contributor Author

kig commented Aug 2, 2018

I think the input data format is good, though u I might rename to v or value - Or I guess intensity for consistency with mesh3d but I don't particularly like that attribute, I wish we had used value there.

Got it, I'll rename u to v.

I'm not sure what you mean by "constructing a meshgrid out of the x,y,z -values and using that as the coords for the u array" though - won't the meshgrid you actually display be some sort of interpolation between the provided (x, y, z) values to find the isovalue?

Hmm, the isosurface now works by running marching cubes on the box lattice defined by |x|,|y|,|z| and u. The result of this is a mesh with vertices at integers [0...|x|-1], [0...|y|-1], [0...|z|-1]. Then the mesh builder moves the vertices to the corresponding x/y/z coords, roughly: verts[i][j][k].xyz = vec3(meshgrid.x[i], meshgrid.y[j], meshgrid.z[k]).

In other words, it constructs a mesh with marching cubes with vertices at the meshgrid positions. (If the meshgrid coords grow at a fixed rate, this amounts to scaling and translating the mesh.)

@kig
Copy link
Contributor Author

kig commented Aug 2, 2018

@etpinard : For @alexcjohnson, who wonders what would happen when we try plot data that cross from >imax to <imin with no values between

This is the issue with marching cubes. What happens is that the data lattice is converted to a binary lattice with a 0 or 1 at each vertex depending on whether the vertex value is inside the iso range or not. Then the vertices of each 2x2 cube in the lattice are interpreted as the bits of an 8-bit number, so that each (top-left-front) corner of the lattice has a corresponding 8-bit number. Next, look up mesh fragments from a lookup array using the 8-bit number as an index, translate fragment vertices to corner, append to mesh.

You could work around it by upscaling the data until you get values in the iso range. I don't know if there's a surface construction algo that would do that itself.

The marching cubes isosurfaces can have holes in them since some sequences are ambiguous and you'd have to look at neighboring cells to resolve which way the planes should be pointing. Asymptotic decider algorithm would solve this according to Wikipedia.

@alexcjohnson
Copy link
Contributor

This is the issue with marching cubes.

You should be able to get the desired result using marching cubes, following the example of our 2D SVG contour plot, which uses marching SQUARES but extends it:

  • First, we do one level at a time, not two... so the binary values are below/above rather than between/outside. That way you can't lose the surface if it transitions too quickly between the two bounds.
  • Then we recognize that each of the "crossings" identified just refers to "somewhere on the line segment connecting two adjacent grid points" but we refine that crossing point by linear interpolation between those same two grid points, and use THAT as the mesh vertex, but connect these into a mesh the same way you do currently.

In some traces, it's nice to have a separate colorscale for the caps. E.g. https://gl-vis.github.io/gl-isosurface3d/smooth_skin.html

Yes, the caps get a colorscale. The isosurfaces, especially with the refinement ^^, will just get a single color each (one for the low, one for the high) so I don't think it's a good idea to make people pull those colors out of a colorscale, just specify one or two colors.

@jackparmer
Copy link
Contributor

@kig - Can you please give us an update on this one?

@archmoj archmoj mentioned this pull request Dec 6, 2018
@etpinard
Copy link
Contributor

etpinard commented Dec 6, 2018

now in #3311

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants