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.

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

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;

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

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

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

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.

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.

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.

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.

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!!

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

Thx.

Why did you move this after updateSelectedState?

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.

Does the clickedPts array ever have more than one item?

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!

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.

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

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.

Have you dug deeper since then?

I haven't had the time unfortunately.

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

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

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:

on master to this branch's

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!

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

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

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

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