-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Scattergl lasso #1657
Scattergl lasso #1657
Conversation
This will be useful for the |
Hey @dfcreative will |
yes, |
Conflicts: src/plot_api/plot_api.js src/plots/cartesian/graph_interact.js
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.
First pass comments.
This is looking promising. Nice job @dfcreative
Looks like you broke the gl2d hover spikes (i.e. those black lines to go from data point down to the axes).
I think for this iteration we should keep the current gl2d hover routine (the mouseContainer
thing in scene2d.js
) and add just what we need from Cartesian
for selections.
src/plots/gl2d/index.js
Outdated
@@ -12,23 +12,22 @@ | |||
var Scene2D = require('./scene2d'); | |||
var Plots = require('../plots'); | |||
var xmlnsNamespaces = require('../../constants/xmlns_namespaces'); | |||
|
|||
var constants = require('../cartesian/constants'); | |||
var cartesian = require('../cartesian') |
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'd prefer
var Cartesian = require('../cartesian');
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
var lastX = 0, lastY = 0, lastMods = {shift: false, control: false, alt: false, meta: false}; | ||
mouseChange(element, handleInteraction); | ||
|
||
// enable simple touch interactions |
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.
Amazing 🎉
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.
That is unmerged #1507 here
src/traces/scattergl/convert.js
Outdated
@@ -303,6 +318,11 @@ proto.update = function(options) { | |||
// not quite on-par with 'scatter', but close enough for now | |||
// does not handle the colorscale case | |||
this.color = getTraceColor(options, {}); | |||
|
|||
//provide reference for selecting points | |||
if (cdscatter && cdscatter[0] && !cdscatter[0].plot) { |
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.
.plot
might not be best here. This is really a trace module though .trace
is already taken.
Maybe .glTrace
?
src/traces/scattergl/index.js
Outdated
|
||
// reuse the Scatter3D 'dummy' calc step so that legends know what to do | ||
ScatterGl.calc = require('../scatter3d/calc'); | ||
ScatterGl.calc = require('../scatter/calc'); |
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.
Smart 👍
Now, can you rm src/trace/scattergl/calc.js
?
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.
... but that might slow down scattergl
for 1e5+ point graphs. 😕
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.
For the record, we must make sure plotting 1e6 points with scattergl
(passing through updateFast
) clocks in at around 1 second.
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.
That is required to obtain selection in scattergl/select. Mb it is possible to optimize that, just haven't got that a chance.
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.
Actually I see no speed difference compared to old implementation for 1e6.
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.
@etpinard ~3300ms vs ~3500ms for 1e6 points, that is 200ms slowdown due to
var x = xa.makeCalcdata(trace, 'x'),
y = ya.makeCalcdata(trace, 'y');
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.
That's ~3300ms on master
?
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.
@etpinard just updated master and plugged laptop: ~3000ms vs ~3200ms, I hardly notice the difference due to fluctuations
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.
Wow. That's brutal. Does anyone else have to wait 3000ms
when plotting 1e6 points?
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.
With 6ac722d we're back in the ~1500ms for 1e6 first render as on master
🎉
Most of scattergl/calc.js
is now skipped when dragmode is set to pan/zoom (the default).
Calling Plotly.relayout
from pan/zoom to lasso/select now trigger a recalc where gd.calcdata
is filled so that the selection logic knows what to do.
src/plots/cartesian/dragbox.js
Outdated
@@ -79,7 +79,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) { | |||
var yIDs = [ya0._id]; | |||
|
|||
// if we're dragging two axes at once, also drag overlays | |||
subplots = [plotinfo].concat((ns && ew) ? plotinfo.overlays : []); | |||
subplots = [plotinfo].concat((ns && ew && plotinfo.overlays) ? plotinfo.overlays : []); |
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.
Why do we need the extra && plotinfo.overlays
now?
src/plots/gl2d/index.js
Outdated
// 3. errorbars for bars and scatter | ||
// 4. scatter | ||
// 5. box plots | ||
function joinPlotLayers(parent) { |
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.
... e.g all of these are useless in gl2d
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.
src/plots/gl2d/scene2d.js
Outdated
@@ -359,7 +358,6 @@ proto.destroy = function() { | |||
|
|||
if(!this.staticPlot) this.container.removeChild(this.canvas); | |||
this.container.removeChild(this.svgContainer); | |||
this.container.removeChild(this.mouseContainer); |
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. That probably breaks gl2d/camera.js
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.
So wait, are we 🔪 the gl2d picking routine?
It would be nice to merge the cartesian and gl2d hover logic at some point, but swapping in cartesian hover in gl2d probably means poor performance on graphs with 1e5 and up data points at this stage.
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.
The problem is that mouseContainer
intercepts mousemove, needed for cartesian selection to work. There seems to be no good way to hook up gl2d/camera so to enable cartesian/select − these are too separate concerns.
It is possible to switch to cartesian hover only for lasso/select mode, but the pick style differs visually, which is confusing.
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.
Probably resolved by 1451802
src/traces/scattergl/attributes.js
Outdated
error_x: scatterAttrs.error_x | ||
error_x: scatterAttrs.error_x, | ||
|
||
hoveron: { |
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 has no effect in gl2d. You must have added this in to make other things not break, correct?
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.
Picking was broken without hoveron
property
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.
Resolved in 648547f from
https://github.com/dfcreative/plotly.js/compare/scattergl-lasso...plotly:scattergl-lasso-etienne
@dfcreative you might want to merge in that commit too.
src/traces/scattergl/convert.js
Outdated
if(sel && !sel[index] && j === 3) { | ||
color *= DESELECTDIM; | ||
} | ||
this.scatter.options.colors[4 * i + j] = color; |
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.
Very nice. Thanks for covering both updateFast
and updateFancy
!!
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.
@dfcreative Something broke selection for fancy traces e.g. with array marker.size
:
var x = [];
var y = [];
var size = []
for( var i = 0; i < 1e4; i++)
{
x.push(i);
y.push(i*Math.random());
size.push(15 * Math.random())
}
var scatterglMarkers = {
x: x, y: y, type: "scattergl", mode: "markers",
marker: {size: size, opacity: 1}
};
console.time('scattergl')
Plotly.newPlot('graph', [scatterglMarkers], {dragmode: 'lasso'})
.then(() => console.timeEnd('scattergl'))
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.
Did you get the time to investigate
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.
Well, looks like this was never implemented.
this.selectedIds
in the scene2d.updateFancy
scope doesn't reference 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.
@etpinard done.
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.
Beautifully done. Thanks very much!
src/traces/scattergl/index.js
Outdated
ScatterGl.plot = require('./convert'); | ||
ScatterGl.selectPoints = require('./select'); |
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/plots/cartesian/axes.js
Outdated
ax.range = ax._rl = axes.getAutoRange(ax); | ||
|
||
ax._r = ax.range.slice(); | ||
ax._rl = Lib.simpleMap(ax._r, ax.r2l); |
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 is an interesting patch. I wonder if this might be fixing an undiscovered bugs(s).
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.
That is taken from https://github.com/dfcreative/plotly.js/blob/6de41e6d555d038299aa0aef7e075804f011db49/src/plots/cartesian/axes.js#L1657, just optimized it a drop
Started leaning on this branch as this feature is essential for crossfiltering if the number of points is over around 1k. No specific need for it now, but at some point, maybe we'd want to add the properties |
⬆️ cursory test shows that
is the culprit (slow irrespective of how many of the 125k points is currently in the visible, zoomed domain): |
TODO:
|
@monfera may I ask you to help reproducing this bug with stuck hover tooltip? I faced it once, but it works fine rest of the time. Is some special scenario to that? |
@dfcreative not showing up on my current branch right now, I'll make a minimal example if it occurs again |
@dfcreative I don't have an isolated case, just a hint at this time - the tooltip seems to get stuck if I delete the
In addition, when the |
includes PRs: - #1657 - gl-vis/gl-scatter2d-sdf#5 - #1781 - #1770
src/traces/scattergl/select.js
Outdated
y = ya.c2p(di.y); | ||
if(polygon.contains([x, y])) { | ||
selection.push({ | ||
id: 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.
done in debdd46
- to match svg 'scatter'
@@ -444,3 +444,149 @@ describe('Test hover and click interactions', function() { | |||
.then(done); | |||
}); | |||
}); | |||
|
|||
describe('Test gl2d lasso/select:', function() { |
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.
Painful! For some reason, these tests doesn't work on CI. Looks like the cases below can't even draw the scene:
https://circleci.com/gh/plotly/plotly.js/4535#tests/containers/1
Can anyone try to pull down this branch and try this out locally?
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.
Would anyone be opposed to merging this without CI test support? I can't figure out why these new tests keep failing on CircleCI. With the |
@etpinard sounds good to me! |
Enables svg interactions for scattergl plot.
Some minor issues TODO:
cdata.plot
opacity of huge number of points is indistinguishable from non-selected pointspanning does not clear selectionRelated dy/plotly-contrib#3