-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
1852 persistent click #2824
Changes from 37 commits
25c56e4
5602335
f4243b4
b552ccf
b9c893f
c1925ea
0eddcbc
ca80c89
2fb35cf
436a746
64705e1
3900d5c
6a672ed
2bc81a2
f51afcf
a1efe01
722805e
4f0a05d
5125981
a79dea7
4d8b274
231fa6d
6bd46bf
427b4b6
3d21856
b0c32c1
a630ef8
e07210c
e082e58
a97af32
aac89ce
4b41e24
f0822d5
8428bbc
f4384de
bbd63d4
d7087a6
df351c3
0840100
b703a9c
d7e64dd
c4012e1
f9494b5
e7815e9
c88aee0
bd04fd8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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++) { | ||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you dug deeper since then? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't had the time unfortunately. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem. There's no hurry at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: plotly.js/src/plots/cartesian/select.js Lines 283 to 286 in def6aa5
on master to this branch's plotly.js/src/plots/cartesian/select.js Lines 242 to 254 in bd04fd8
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In hindsight it seams the new approach of splitting up Thinking back, the reason for splitting up Let me jot down some thoughts for the upcoming meeting: Current approach on masterThe approach to selections before tackling persistent select was as follows: each trace type module supporting selection exposes a Possible new approach to support click-to-selectThe whole polygon testing approach is covered in
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||||||||
}; |
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 might be better to make
Plotly.register
set a boolean_module.selected
somewhere here and use that boolean downstream (e.g. inPlots.supplyDefaults
) instead of looking forgetPointsIn
andtoggleSelected
.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.
Neat! Would
_module.selectable
even be a better name?