Skip to content

1852 persistent click #2824

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

Closed
wants to merge 46 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
25c56e4
Add click-to-select behavior for scatter [1852]
rmoestl May 18, 2018
5602335
Support deselect on click for scatter [1852]
rmoestl Jun 7, 2018
f4243b4
Transition to no selection style after deselecting a data point [1852]
rmoestl Jun 7, 2018
b552ccf
Allow to retain current selection with pressing Shift key [1852]
rmoestl Jun 7, 2018
b9c893f
Take retention mode into acount on deselect [1852]
rmoestl Jun 7, 2018
c1925ea
Retain click-selected points in box or lasso select mode [1852]
rmoestl Jun 14, 2018
0eddcbc
Introduce change of trace selection interface [1852]
rmoestl Jun 21, 2018
ca80c89
Implement polygon subtract mode in new approach [1852]
rmoestl Jun 21, 2018
2fb35cf
Remove polygon outlines when select-on-click [1852]
rmoestl Jun 21, 2018
436a746
Clean up click-on-select code [1852]
rmoestl Jun 21, 2018
64705e1
Fix bug in deletion of polygon select outlines [1852]
rmoestl Jun 26, 2018
3900d5c
Condense traces' selection interface surface [1852]
rmoestl Jun 26, 2018
6a672ed
Fix typo scatter's select module [1852]
rmoestl Jul 17, 2018
2bc81a2
Transition bar trace to new selection mechanism [1852]
rmoestl Jul 17, 2018
f51afcf
Support click-to-select in a multi-traces setting [1852]
rmoestl Jun 29, 2018
a1efe01
Extract 'finding selectable traces' in cartesian/select [1852]
rmoestl Jun 29, 2018
722805e
Transition scattergl to new selection interface [1852]
rmoestl Jul 3, 2018
4f0a05d
Adapt cartesian's selectOnClick to support histogram [1852]
rmoestl Jul 3, 2018
5125981
Use clearEntireSelection flag in cartesian/select [1852]
rmoestl Jul 3, 2018
a79dea7
Fix bug when clearing entire selection [1852]
rmoestl Jul 3, 2018
4d8b274
Implement new selection interface for box trace type [1852]
rmoestl Jul 4, 2018
231fa6d
Consider first eventData element only as clicked point [1852]
rmoestl Jul 4, 2018
6bd46bf
Clean up code in box/select.js [1852]
rmoestl Jul 4, 2018
427b4b6
Not select a clicked box in box trace type [1852]
rmoestl Jul 5, 2018
3d21856
Not change selection state when a box is clicked in box trace [1852]
rmoestl Jul 5, 2018
b0c32c1
Remove unused testPoly variable in cartesian select module [1852]
rmoestl Jul 18, 2018
a630ef8
Remove obsolete `selectPoints` from box trace's select module [1852]
rmoestl Jul 18, 2018
e07210c
Quick fix in dragbox module to satisfy ESLint [1852]
rmoestl Jul 18, 2018
e082e58
Remove obsolete `selectPoints` from scattergl's select module [1852]
rmoestl Jul 18, 2018
a97af32
Transition violin trace type to new selection interface [1852]
rmoestl Jul 18, 2018
aac89ce
Reactivate emitting 'plotly_click' events [1852]
rmoestl Jul 18, 2018
4b41e24
Emit 'plotly_deselect' after outlines have been removed [1852]
rmoestl Jul 18, 2018
f0822d5
Fix testing if double-click cleans selection in box/lasso mode [1852]
rmoestl Jul 18, 2018
8428bbc
Fix testing selection subtraction in box/lasso mode [1852]
rmoestl Jul 18, 2018
f4384de
Transition scattergeo trace type to new selection interface [1852]
rmoestl Jul 19, 2018
bbd63d4
Transition scattercarpet trace type to new selection interface [1852]
rmoestl Jul 19, 2018
d7087a6
speed up Lib.difference
etpinard Jul 19, 2018
df351c3
Introduce _module.selectable flag for trace modules [1852]
rmoestl Jul 20, 2018
0840100
Merge branch 'master' into 1852-persistent-click
rmoestl Aug 7, 2018
b703a9c
Consider subplot when determining traces for click-to-select [1852]
rmoestl Aug 7, 2018
d7e64dd
Put back call to remove outlines back to original location [1852]
rmoestl Aug 7, 2018
c4012e1
Reintroduce selectMode guard in scattegl [1852]
rmoestl Aug 7, 2018
f9494b5
Fix modebar Jasmine tests involving selectable traces [1852]
rmoestl Aug 8, 2018
e7815e9
Transition scattermapbox trace type to new selection interface [1852]
rmoestl Aug 8, 2018
c88aee0
Prevent mapbox removing select outlines on every drag operation [1852]
rmoestl Aug 8, 2018
bd04fd8
Disable click-to-select in pan/zoom mode temporarily [1852]
rmoestl Aug 8, 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
5 changes: 2 additions & 3 deletions src/components/modebar/manage.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,6 @@ function areAllAxesFixed(fullLayout) {
}

// look for traces that support selection
// to be updated as we add more selectPoints handlers
function isSelectable(fullData) {
var selectable = false;

Expand All @@ -194,7 +193,7 @@ function isSelectable(fullData) {

var trace = fullData[i];

if(!trace._module || !trace._module.selectPoints) continue;
if(!trace._module || !trace._module.selectable) continue;

if(Registry.traceIs(trace, 'scatter-like')) {
if(scatterSubTypes.hasMarkers(trace) || scatterSubTypes.hasText(trace)) {
Expand All @@ -205,7 +204,7 @@ function isSelectable(fullData) {
selectable = true;
}
}
// assume that in general if the trace module has selectPoints,
// assume that in general if the trace module has getPointsIn and toggleSelected,
// then it's selectable. Scatter is an exception to this because it must
// have markers or text, not just be a scatter type.
else selectable = true;
Expand Down
3 changes: 3 additions & 0 deletions src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ lib.variance = statsModule.variance;
lib.stdev = statsModule.stdev;
lib.interp = statsModule.interp;

var setOpsModule = require('./set_operations');
lib.difference = setOpsModule.difference;

var matrixModule = require('./matrix');
lib.init2dArray = matrixModule.init2dArray;
lib.transposeRagged = matrixModule.transposeRagged;
Expand Down
43 changes: 43 additions & 0 deletions src/lib/set_operations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/**
* Copyright 2012-2018, Plotly, Inc.
* All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

'use strict';


/**
* Computes the set difference of two arrays.
*
* @param {array} a
* @param {array} b
* @returns out all elements of a that are not in b.
* If a is not an array, an empty array is returned.
* If b is not an array, a is returned.
*/
function difference(a, b) {
if(!Array.isArray(a)) return [];
if(!Array.isArray(b)) return a;

var hash = {};
var out = [];
var i;

for(i = 0; i < b.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit makes selection of 1e5 scattergl markers work ok, but still about an order of magnitude too slow.

I'll have to dig deeper tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you dug deeper since then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't had the time unfortunately.

Copy link
Contributor

@etpinard etpinard Aug 7, 2018

Choose a reason for hiding this comment

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

... I'll find some time to review it before the meeting on Thursday though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. There's no hurry at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok very briefly just to get a conversion started (and not break my promise):

Doing some order of operation analysis in the hot selection code path comparing:

searchInfo = searchTraces[i];
traceSelection = searchInfo._module.selectPoints(searchInfo, testPoly);
traceSelections.push(traceSelection);

on master to this branch's

searchInfo = searchTraces[i];
module = searchInfo._module;
if(!retainSelection) module.toggleSelected(searchInfo, false);
var currentPolygonTester = polygonTester(currentPolygon);
var pointsInCurrentPolygon = module.getPointsIn(searchInfo, currentPolygonTester);
module.toggleSelected(searchInfo, !subtract, pointsInCurrentPolygon);
var pointsNoLongerSelected = difference(pointsInPolygon[i], pointsInCurrentPolygon);
traceSelection = module.toggleSelected(searchInfo, false, pointsNoLongerSelected);
pointsInPolygon[i] = pointsInCurrentPolygon;

we can see why this branch is roughly an order of magnitude slower:

On master that block scales as O(n). To handle addition/subtraction/merge selections, we use an algo acting on the selections polygon with I believe is O(number-of-vertices^2) but results nonetheless in not that many computations in most use cases (e.g. rectangles with 4 vertices)

On this branch, we have

if(!retain) toggleSelected('clear')
// which is currently O(2*n)

getPolyonTester()
// not sure why this one is called in the hot code path

getPointsIn()
// this one is O(n) assuming 'fast' is-in-polygon computations

toggleSelected('add or substract')
// this one currently is O(n + p)

difference()
// O(n + p)

toggleSelected('clear')
// this one is O(n + (n-p))

which when added all up gives a total of O(6n + p) where n is the number of points and p is number of selected points ~ roughly one order of magnitude.

So, we should try to combine all the toggleSelected loops into one. Don't get me wrong, I don't think splitting up selectedPoints into toggleSelected and getPointsIn was a bad idea, but perhaps we could bring a version of selectPoints that reuses getPointsIn? We can talk more about this tomorrow during the meeting. Thanks for your hard work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In hindsight it seams the new approach of splitting up selectedPoints wasn't such a good idea :-( I'm not sure if it's possible to speed things up that much with that approach. To be honest, it feels like to be at square one again.

Thinking back, the reason for splitting up selectPoints was to cover a couple of edge cases when users would use click-to-select and polygon-select interchangeably. Back then we discussed possible approaches here.

Let me jot down some thoughts for the upcoming meeting:

Current approach on master

The approach to selections before tackling persistent select was as follows: each trace type module supporting selection exposes a selectPoints function that accepts a searchInfo parameter describing the actual trace and a polygon object that offers a contains function. selectPoints iterates through the data points of the passed trace (identified by searchInfo) and calls polygon.contains for each data point to see if the data point is within the selection area. Adding/subtracting from an existing selection is supported by wrapping multiple polygons into one multiPolygon and passing that to selectPoints.

Possible new approach to support click-to-select

The whole polygon testing approach is covered in lib/polygon.js which exposes tester (for testing a single polygon) and multiTester (wrapper for multiple tester objects). What about having a third tester type that tests by data point index? What would that mean for the client code?

  • Go back to selectPoints in trace type modules
  • Add data point index as a parameter to the signature of contains
  • In selectPoints implementations do not only pass the x and y (or lan, lat etc.) of a data point when calling contains but also the data point index
  • Adapt multiTester to be able to wrap the new index-based tester

Copy link
Contributor

@etpinard etpinard Aug 9, 2018

Choose a reason for hiding this comment

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

Your new approach sounds very promising! I'd say give it a shot with scatter and scattergl and ping me so that I can quickly review and perf-test it. 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard proof-of-concept is ready to review. It was easier to start from scratch. So the new approach can be found at https://github.com/plotly/plotly.js/tree/1852-persistent-click-via-selectPoints. (Although it would make reviewing easier, I figured code is not quite ready for a new PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a few comments on 2fbaaaa . Your solution is looking great! 💎

I'd say go ahead and try to implement it for all selectable trace modules. 🚀

hash[b[i]] = 1;
}

for(i = 0; i < a.length; i++) {
var ai = a[i];
if(!hash[ai]) out.push(ai);
}

return out;
}

module.exports = {
difference: difference
};
19 changes: 15 additions & 4 deletions src/plots/cartesian/dragbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ var doTicksSingle = require('./axes').doTicksSingle;
var getFromId = require('./axis_ids').getFromId;
var prepSelect = require('./select').prepSelect;
var clearSelect = require('./select').clearSelect;
var selectOnClick = require('./select').selectOnClick;

var scaleZoom = require('./scale_zoom');

var constants = require('./constants');
Expand Down Expand Up @@ -158,7 +160,7 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
// to pan (or to zoom if it already is pan) on shift
if(e.shiftKey) {
if(dragModeNow === 'pan') dragModeNow = 'zoom';
else if(!isSelectOrLasso(dragModeNow)) dragModeNow = 'pan';
else if(!isBoxOrLassoSelect(dragModeNow)) dragModeNow = 'pan';
}
else if(e.ctrlKey) {
dragModeNow = 'pan';
Expand All @@ -171,10 +173,11 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
if(dragModeNow === 'lasso') dragOptions.minDrag = 1;
else dragOptions.minDrag = undefined;

if(isSelectOrLasso(dragModeNow)) {
if(isBoxOrLassoSelect(dragModeNow)) {
dragOptions.xaxes = xaxes;
dragOptions.yaxes = yaxes;
// this attaches moveFn, clickFn, doneFn on dragOptions
// TODO Maybe rename the function to prepSelectOnDrag
prepSelect(e, startX, startY, dragOptions, dragModeNow);
} else {
dragOptions.clickFn = clickFn;
Expand Down Expand Up @@ -212,8 +215,10 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
if(numClicks === 2 && !singleEnd) doubleClick();

if(isMainDrag) {
Fx.click(gd, evt, plotinfo.id);
handleClickInMainDrag(gd, numClicks, evt, xaxes, yaxes, plotinfo.id);
}
// Allow manual editing of range bounds through an input field
// TODO consider extracting that to a method for clarity
else if(numClicks === 1 && singleEnd) {
var ax = ns ? ya0 : xa0,
end = (ns === 's' || ew === 'w') ? 0 : 1,
Expand Down Expand Up @@ -620,6 +625,12 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
redrawObjs(gd._fullLayout.images || [], Registry.getComponentMethod('images', 'draw'), true);
}

function handleClickInMainDrag(gd, numClicks, evt, xaxes, yaxes, subplot) {
// TODO differentiate based on `clickmode` attr here as soon as it is available
selectOnClick(gd, numClicks, evt, xaxes, yaxes);
Fx.click(gd, evt, subplot);
}

function doubleClick() {
if(gd._transitioningWithDuration) return;

Expand Down Expand Up @@ -1013,7 +1024,7 @@ function showDoubleClickNotifier(gd) {
}
}

function isSelectOrLasso(dragmode) {
function isBoxOrLassoSelect(dragmode) {
return dragmode === 'lasso' || dragmode === 'select';
}

Expand Down
Loading