-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
3D cone traces #2641
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
3D cone traces #2641
Conversation
- unfortunately they don't render correctly in `imagetest`
forgot to cc @alexcjohnson @jackparmer and @kig who maybe could help out ✅ a few checkboxes. |
Cool!
Agreed - a cone is a 3D shape, it would have no meaning in 2D, so just
I think we should have the required data be the vector field
Absolute vector norm seems far more useful. Leave this attribute out and let people normalize on their own, until someone complains. The hover data is always going to show the absolute vector anyway, right? Would be weird if this disagreed with the colorscale.
Much as I like Is there a use case to supply color as an independent data array? Perhaps someone wants to color just by |
Are the cones positioned with their tips at the data positions? That seems odd, I'd have thought the data position would go at its center of mass, ie 3/4 of the way from the tip to the base. |
Yes. That's how it works currently. |
Huh, MATLAB places the cones with their tails at the data positions. That also seems wrong to me... |
'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.' |
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.
What do you mean by a grid unit here? Is it one unit of the (x, y, z)
positions or the spacing between adjacent cones?
Almost always the vectors have different units from the position space they're displayed in - so I'd think if you specify nothing at all about the cone sizing, they should end up scaled so the largest one is about as big as the smallest spacing between adjacent cones.
There's definitely a use case for 'absolute'
- the user has pre-calculated everything and they want each cone to go from one precise (x, y, z)
to another precise (x, y, z)
. I'm not sure there's a use case though for scaling to one unit in position, I'd think 'scaled'
should refer to the minimum adjacent cone distance.
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.
To be honest, I'm not sure. I copied this from https://github.com/gl-vis/gl-cone3d#constructor I'll have to inspect
a little bit more.
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'll have a new look at the cone sizing parameters. Pre-meshgrid they were based on the position space units and scaled so that two cones with maximum norm, pointing to the same direction, would have their tip and base touch. Like <|<|. That would be with coneSize 2. With coneSize 1, you'd get no overlapping cones even when they're |><|.
The absolute sizing ditched the automatic cone scaling and used the cone vector norm to figure out the size of the cone model, so that a norm of 1 with absolute size factor 1 results in a cone that's one unit tall in the position unit space. Absolute size factor 2 => 2 unit tall cone, etc.
With the meshgrid sampling (and the realization that the cone positions may not be on a nice regular grid), I changed the automatic cone sizing to scale based on the smallest distance between two adjacent cone positions (adjacent in the sense that they've got successive indices in the positions array). This is not the right thing to do though, doing a smallest distance between any two cone positions would be, but that's O(n^2) with a naive algo.
Another idea floated on the sizing was to have pixel-based sizing as well, which'd probably work like "invert the projection, view and model transform matrices for cone model, scale cone model according to viewport size" but would result in ortho camera cones which might be confusing. (And it sounds like a bit of a pain to implement.)
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.
@kig that all sounds great. I guess we only need to worry about the smallest-distance calculation when using the (x, y, z)
data to position the cones; With the cartesian product of x/y/z interpolation grids each dimension should be pretty compact, even if the values are out of order. But I guarantee at some point we'll see a data set where the closest points are not successive. I don't know if there's a JS implementation already available but it's possible to do it in O(n log(n)^2) http://www.cs.ucsb.edu/~suri/cs235/ClosestPair.pdf
Lets not worry about pixel-based sizing, at least not for now. I guess I can see using this if we also do automatic grid spacing, ie when you zoom in and the cone spacing gets larger, at some point we decrease the grid size so we draw more cones between the existing ones. That's definitely not a v1 feature 😅
If it's just for color (which, per #2641 (comment), might not not even come from the cone lengths), can't we just find the bounds of the data provided, as a simple loop over the cone data? That should be a heck of a lot faster (quick test: ~5ms for 20k data points) and, I might argue, more faithful to the data than basing the color bounds on the interpolated data used in the actual cones. |
... using an invisible gl-scatter3d trace
src/traces/cone/convert.js
Outdated
var meshData = convert(scene, data); | ||
var mesh = createConeMesh(gl, meshData); | ||
|
||
var pts = createScatterPlot({ |
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.
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.
Nice, that was fast! Is it possible to get (u, v, w)
in the hovertext as well?
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.
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.
'x+y+z+norm'
by default and optionally up to:x+y+z+u+v+w+norm+text
I like it!
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.
done in e4a2035
... w/o using cone2mesh, and use that the find cmin & cmax which now correspond to "absolute" vector norm. This make cone/helpers.js obsolete.
... during autorange / scene bounds computation. - This pad values is a "worse case" solution (i.e. it pads too much for most situations), but gives nice visual results (I think).
src/traces/cone/calc.js
Outdated
|
||
colorscaleCalc(trace, meshData.vertexIntensity, '', 'c'); | ||
colorscaleCalc(trace, [normMin, normMax], '', 'c'); |
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.
Shoutout to @alexcjohnson for the tip!
src/plots/gl3d/scene.js
Outdated
var obj = objects[j]; | ||
var objBounds = obj.bounds; | ||
// add worse-case pad for cone traces | ||
var pad = (obj._trace.data._normMax || 0) * dataScale[i]; |
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.
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.
Looks good! I suppose based on gl-vis/gl-cone3d#6 we could drop it to 3/4 of that value, though then if we make that configurable it would depend on the offset... more trouble than it's worth, let's keep what you have here.
- x/y/z always refers vector positions (their length should match u/v/w) - if nothing else provided, x/y/z correspond to the cone position also, - to set cone position, use `cones.(x|y|z)` for now - update attribute descriptions and trace meta info
I still need to fixup this case brought up by @alexcjohnson // seems like something’s off with the axis padding - cones at 1 & 3 give axis ranges `[-2.46, 6.46]`? Shouldn’t that be `[0, 4]` if we’re padding by one full cell?
Plotly.newPlot(gd,[{type: 'cone', x: [1, 1, 1, 1, 3, 3, 3, 3], y: [1, 1, 3, 3, 1, 1, 3, 3], z: [1, 3, 1, 3, 1, 3, 1, 3],
u: [1, 2, 3, 4, 1, 2, 3, 4], v: [1, 1, 2, 2, 3, 3, 4, 4], w: [4, 4, 1, 1, 2, 2, 3, 3]}]) but other than, this PR seems ready. Dropping the discussion needed, tagging as reviewable. |
src/plots/gl3d/scene.js
Outdated
@@ -508,7 +508,7 @@ proto.plot = function(sceneData, fullLayout, layout) { | |||
var obj = objects[j]; | |||
var objBounds = obj.bounds; | |||
// add worse-case pad for cone traces | |||
var pad = (obj._trace.data._normMax || 0) * dataScale[i]; | |||
var pad = 0.75 * (obj._trace.data._compMax || 0) * dataScale[i]; |
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.
This gives a better estimate. The data off #2641 (comment) now gives:
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.
Great! Yeah, I was wrong with [0, 4]
- this looks awesome.
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.
hmm, this is still not quite right though. If I double all the u, v, w values, the cones don't change (as they shouldn't) but the padding increases.
var t={type: 'cone', x: [1, 1, 1, 1, 3, 3, 3, 3], y: [1, 1, 3, 3, 1, 1, 3, 3], z: [1, 3, 1, 3, 1, 3, 1, 3],
u: [1, 2, 3, 4, 1, 2, 3, 4], v: [1, 1, 2, 2, 3, 3, 4, 4], w: [4, 4, 1, 1, 2, 2, 3, 3]}
// the resulting image should be independent of what I multiply by here
function m(v) { return v*2; }
t.u = t.u.map(m); t.v = t.v.map(m); t.w=t.w.map(m); Plotly.newPlot(gd,[t], {scene: {camera: {eye: {x: -0.01, y: 2.52, z: 0.42}}}, width: 800, height: 800})
I don't need to tell you what happens with v / 10
do I? 😉
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.
latest attempt: 3d26d47
delete gd.data[0][k]; | ||
|
||
supplyAllDefaults(gd); | ||
expect(gd._fullData[0].visible).toBe(!k, 'missing array ' + k); |
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.
Clever, but maybe a little too compact to include ''
in this set 😈
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.
ok, fixed in 8dca9ae
test/jasmine/tests/cone_test.js
Outdated
}) | ||
.then(function() { | ||
_assertAxisRanges('after adding one cone outside range but with norm-0', | ||
[-0.45, 6.45], [-0.45, 6.45], [-0.45, 6.45] |
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.
@alexcjohnson unfortunately I can't get this to "stick" to the previous range[0]
values without breaking other gl3d mocks that depend on
plotly.js/src/plots/gl3d/scene.js
Lines 479 to 483 in fb75158
for(j = 0; j < objects.length; j++) { | |
var objBounds = objects[j].bounds; | |
sceneBounds[0][i] = Math.min(sceneBounds[0][i], objBounds[0][i] / dataScale[i]); | |
sceneBounds[1][i] = Math.max(sceneBounds[1][i], objBounds[1][i] / dataScale[i]); | |
} |
which scales the autorange bounds with the dataScale
:
plotly.js/src/plots/gl3d/scene.js
Lines 394 to 407 in fb75158
var dataScale = [1, 1, 1]; | |
for(j = 0; j < 3; ++j) { | |
if(dataBounds[0][j] > dataBounds[1][j]) { | |
dataScale[j] = 1.0; | |
} | |
else { | |
if(dataBounds[1][j] === dataBounds[0][j]) { | |
dataScale[j] = 1.0; | |
} | |
else { | |
dataScale[j] = 1.0 / (dataBounds[1][j] - dataBounds[0][j]); | |
} | |
} | |
} |
Hopefully this iteration is close enough.
As a side note, we should spend some time replacing this dataScale
hack. This thing was added to make date axes work properly in 3D. Timestamps don't fit in a Float32Array, so we can't just convert the datestrings to timestamps, but we do this better in scattergl
, where we use Float64Array or split the data into two Float32Arrays when not supported.
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.
New issue about the dataScale
mess -> #2646
src/traces/cone/convert.js
Outdated
@@ -107,6 +108,9 @@ function convert(scene, trace) { | |||
meshData.fresnel = trace.lighting.fresnel; | |||
meshData.opacity = trace.opacity; | |||
|
|||
// stash autorange pad value | |||
trace._pad = anchor2coneSpan[trace.anchor] * meshData.vectorScale * trace._normMax * trace.sizeref; |
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.
Awesome, I think we have a winner now! I don't see any cases (As I change the cone spacing, cone count in each direction, and vector length) where this fails to encompass all the cones, nor any where the padding looks unpleasantly large. 🏆
test/jasmine/tests/cone_test.js
Outdated
@@ -102,7 +102,7 @@ describe('@gl Test cone autorange:', function() { | |||
}) | |||
.then(function() { | |||
_assertAxisRanges('scaled up', | |||
[-0.39, 4.39], [-0.39, 4.39], [-0.39, 4.39] | |||
[-0.66, 4.66], [-0.66, 4.66], [-0.66, 4.66] |
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.
Can you add one more test, to 🔒 this in a little more since it went through so many iterations: scale up the x/y/z arrays. At least one of the previous iterations had a problem with this but otherwise looked pretty good.
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.
there's this one:
plotly.js/test/jasmine/tests/cone_test.js
Lines 121 to 144 in abb11e0
var trace = fig.data[0]; | |
var x = trace.x.slice(); | |
x.push(5); | |
var y = trace.y.slice(); | |
y.push(5); | |
var z = trace.z.slice(); | |
z.push(5); | |
var u = trace.u.slice(); | |
u.push(0); | |
var v = trace.v.slice(); | |
v.push(0); | |
var w = trace.w.slice(); | |
w.push(0); | |
return Plotly.restyle(gd, { | |
x: [x], y: [y], z: [z], | |
u: [u], v: [v], w: [w] | |
}); | |
}) | |
.then(function() { | |
_assertAxisRanges('after adding one cone outside range but with norm-0', | |
[-0.72, 6.72], [-0.72, 6.72], [-0.72, 6.72] | |
); |
am I missing anything?
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.
It's close, but there's a difference between adding another point that doesn't change the minimum spacing between cones, and expanding the whole x/y/z space and therefore expanding the minimum spacing as well.
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.
got it. Thanks!
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.
done in -> 2c08357
Alright! I'm happy if you are! 💃 💃 💃 |
Ok. Merging this thing! Shoutout to @kig for this feature 🎉 Thanks very much @alexcjohnson for the help making this plotly-proof! 💪 This will be part of v1.38.0 set to be released on Tuesday. 🚀 by the way @kig I published |
Cones are coming to plotly.js
Things we need to decide on
cone
, like a cone trace, similar tosurface
. But since a cone trace has often many cones, maybecones
would be better.cone3d
similar tomesh3d
could work, but I think the 3d is not necessary, quiver is more popular in 2d. Decision: we're going withcone
.vx
,vy
andvz
(v for vector), but maybemeshgrid.x
,meshgrid.y
andmeshgrid.z
would be best. Note that these attributes are optional. Decision:going withRemoved from this 1st implementation -> 5a42de0cones.(x|y|z)
in cb7ef43 which will allows us to configure non-cartesian-product meshgrids if we want down the road.cmin
andcmax
to be positive also? Moreover, maybe we should makeViridis
the default colorscale. Decision: Let's not restrict cmin, cmax in case we add anintensity
field to color the cones by something other than their vector norm down the road. No go for Viridis default until v2.Things we need to fix/add
Update: Ok, but not great solution done in db8222b and improved multiple times in subsequent commits.
size
settings (attributessizeref
andsizemode
) - done in 863b0daimagetest
(works fine in orca) - Update: non-perfect solution in aaf7249 wheregl3d_cone-*
mocks are tests using in orca during./tasks/noci_test.sh
.Things that work ok, but could improve
cone2mesh
twice which can be slow (~300ms for 20k cones) per.plot
call. Once incalc
to get the colors (i.e. intensity) values and another inconvert
to compute the correctly scaled coordinate (note the the scene's scale isn't available duringcalc
). I would be nice to split gl-cone3d'screateConePlot
into two subroutines. Update: not any more after a2db567Things that could wait future iterations
x
,y
andz
optional, generate linspaces when omitted[ ] providing an underdetermined meshgrid can lead to exceptions in gl-cone3d'snot an issue until we figure how to implement our version of meshgrid.createConePlot