1852 persistent click

[rmoestl]

Though not complete, this pull request will introduce the select-on-click / persistent selection feature discussed in issue #1852.

The feature requires a change of the trace selection interface and how selection state is determined and controlled in general.

Tests (image and Jasmine alike) are primarily caused by trace types not yet migrated to the new selection interface.

[rmoestl]

Since this is a PR in progress, here's a quick overview of what is missing:

• Transition not yet migrated trace types to new selection interface
• Introducing the clickmode attribute and the corresponding logic in dragbox.js
• Emitting the correct plotly_selected, plotly_deselected etc. events after a click-on-select action
• Writing Jasmine tests ensuring click-to-select works for each supported trace type
• Writing Jasmine tests ensuring that a combination of subsequent polygon and click selects work as expected
• Clean up things (added TODOs in particular) and here and there adjust coding style
• Maybe adapt other bases plot types besides cartesian to use the new trace selection interface?!
• Also adapt API documentation of hovermode because click-to-select works best with closest
• Should a selection be cleared when user clicks on an empty area?
• Read through issue Persistent and Multiple Clicking and Unclicking Behaviour #1852 to ensure we haven't forgotten anything

[etpinard]

It might be better to make Plotly.register set a boolean _module.selected somewhere here and use that boolean downstream (e.g. in Plots.supplyDefaults) instead of looking for getPointsIn and toggleSelected.

[rmoestl]

Neat! Would _module.selectable even be a better name?

[etpinard]

Looks like Lib.difference is only called on array of number. We could take advantage of that by using a hash table instead of indexOf which can be slow for arrays with length > 1e5.

Something like:

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

if(a.length < b.length) {
  small = a;
  large = b;
} else {
  small = b;
  large = a;
}

for(i = 0; i < small.length; i++) {
  hash[small[i]] = 1;
}

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

return out;

[etpinard]

... it might be nice to look at the lodash's implementation.

[etpinard]

Looks like ternary subplots still set that subplot field as well as geo and mapbox subplots.

[rmoestl]

Thx! Really great you're also going over my TODOs because some of them would have required your or AJ's input anyways!

[etpinard]

Ah no. We need to keep that selectMode condition.

As creating one regl context takes about ~300ms, we don't want to create the selection regl context when in non-select interaction modes.

But perhaps, you only remove it before implementing clickmode. Please ignore if that's the case.

[rmoestl]

Oh, I'll have to dig into that again. With if(selectMode) scattergl traces weren't updated on select-on-click when in zoom/pan dragMode. I guess I'll have to adapt the logic when selectMode flag is set to true.

[rmoestl]

As far as I understand, the selection regl context is created by a call to createScatter(). When user switches to lasso or box mode, the plot is re-rendered for some reason and regl contexts are creating again.

I think you are absolutely right to take clickMode into account to decide if scene.select2d context needs to be created.

For now, I'll reintroduce the selectMode condition and add a TODO for later when I introduce clickmode.

[etpinard]

I have some bad news. The new selection algo perform very poorly above 1e5 points (which in practice only affects scattergl). From

var N = 1e5
var x = []
var y = []
var ms = []

for(var i = 0; i < N; i++) {
  x.push(Math.random())
  y.push(Math.random())
  ms.push(i % 2 ? 20 : 10)
}

console.time(N)
Plotly.newPlot(gd, [{
  type: 'scattergl',
  mode: 'markers',
  x: x,
  y: y,
  marker: {size: ms}
}], {
  dragmode: 'select',
})
console.timeEnd(N)

selecting is painfully slow.

---

I'll try to pin down what's causing this slow down later today.

[rmoestl]

Okay, thanks! Let me know if I can be of any assistance, e.g. if some of my code isn't clear...

BTW I didn't know console.time and console.timeEnd! Awesome!!

[etpinard]

Thanks for writing down that TODO.

I'm thinking when clickmode: 'select' we'll emit plotly_selected with the selected points info. Otherwise, we'll have to keep that dummy event below (until v2).

[rmoestl]

Thx.

[etpinard]

Why did you move this after updateSelectedState?

[rmoestl]

TBH, I'm not sure. Fun fact: I even had it beneath emitting plotly_selected event which then broke some unit tests that assume outline elements are no longer in DOM when plotly_selected event is received.

But why did I really moved it below updateStelectedState? As far as I can remember, in the early commits, I thought I could move the line before the if(numClicks === 2) condition. For some reason that wasn't correct and I moved it back into the if block. So I guess no rationale behind it other than me not putting it back where it always should have been.

[etpinard]

Does the clickedPts array ever have more than one item?

[rmoestl]

At the moment not. I changed it back when I was about to integrate the box trace type. See 231fa6d.

I figured it'd be a good idea to be ready for hoverdata one day containing more than one valid data point. E.g. when multiple data points are at the same location that was clicked.

Is allocating an array costly in terms of performance? Happy to change it back to non-array style if that is the case! Or, if we want to keep it, I could add a comment!

[etpinard]

Is allocating an array costly in terms of performance?

It can be costly, but I'll have to test on some real-world data to know for sure.

[rmoestl]

For now I postpone moving back to a non-array return type until there's proof that it hurts performance considerably.

[etpinard]

This PR is very impressive. 🎉 💪 🥇

Thanks very much @rmoestl for taking the time to improve parts of our internal API. Selections have been suffering from some growing pains in the last few months, this is for sure a step in the right direction.

I do have a few concerns about performance. Hopefully, speeding up Lib.difference will suffice. I'll report back what I found later today.

[etpinard]

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.

[rmoestl]

Have you dug deeper since then?

[etpinard]

I haven't had the time unfortunately.

[etpinard]

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

[rmoestl]

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

[etpinard]

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:

plotly.js/src/plots/cartesian/select.js

Lines 283 to 286 in def6aa5

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

on master to this branch's

plotly.js/src/plots/cartesian/select.js

Lines 242 to 254 in bd04fd8

	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!

[rmoestl]

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

[etpinard]

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. 👌

[rmoestl]

@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).

[etpinard]

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. 🚀

[rmoestl]

Thanks @etpinard for taking the time to do such a thorough review! 🙇‍♂️
I'll gradually work through the feedback.

[rmoestl]

@etpinard (CC @alexcjohnson) I've incorporated your feedback as much as possible at this stage and left TODOs etc. where implementation makes more sense later.

Next steps: I'll let CI run the tests and then tackle failing tests. That'll involve transitioning further trace types to the new selection interface. After that I'll start to add my own tests.

[etpinard]

Now in -> #2944