Skip to content
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 via select points #2944

Merged
merged 56 commits into from Sep 10, 2018

Conversation

@rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Aug 27, 2018

Superseding PR #2824, this PR will introduce the select-on-click / persistent selection feature discussed in issue #1852.

A few remarks:

  • There's one uncertainty regarding testing interactions that should have no effect but might be processed asynchronously. See open TODOs in select_test.js.
  • For now I've decided to not clear selection when user clicks a blank spot. I felt, that this cause losing a selection to easily. Instead clear a selection by click is easy with double click in lasso / select mode, and in zoom / pan mode one only needs to select one point and click it again.
  • As per the original request in #1852 we've not decided how the user "would be informed that points are clickable".
Reason: to be reviewable from a perf standpoint.
etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

We could speed up isPointSelected by building up a hash table instead of using 🐢 .indexOf but even at 1e6 points, this won't amount to much. No need to "fix" this TODO for the first implementation.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

Very nice solution. At 1e6 pts, click-to-select takes about ~100ms. At 1e5 pts it clocks in at ~10-20 ms. Solid work!

Now, it's important to note: as we shift+click on more points this routine runs slower and slower. For example, at 1e6 shift+click is slower by roughly 20ms each time there's an addition tester. At 1e5 pts, it's about 2-3 ms slower each time. Below that, the increase is barely noticeable.

So, we could try to "merge" the testers together as opposed to just appending to a "list" of testers to speed things up. Or maybe we could try to run about _module.selectPoints only the subset of points that are visible within the axis ranges? But honestly, these numbers are solid and very much good enough for the first implementation. 👌

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

Yeah, we usually use 'M0,0Z' to set a blank path.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

Nice observation. We'll have to wait for v2 though to standardize histogram event data with the rest of our trace types.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

Up to you here.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

So wait, isn't this how this branch works already?

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

By creating hash tables, this shouldn't be much of a factor, especially for click-to-selection which isn't a very hot code path.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 10, 2018

It would be nice to add info about the .contains keys in the docstring about.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 13, 2018

Okay. Though I wonder what is causing the decrease of speed. Is it the loop in multiTester?

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 13, 2018

Thx.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 13, 2018

Yes. Comment should have mentioned that.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 13, 2018

Out of curiosity, where would you attach those hash tables? Maybe similarly to dragOptions.polygons?

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 13, 2018

Is it the loop in multiTester?

Exactly, yes.

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 13, 2018

For example, turning

// given
// batch // => [/* */]

var hash = {};

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

and then

for(var i = 0; i < pts.length; i++) {
   // use this to check if pts[i] is in batch list
   hash[pts[i]] 
   // instead of 
   batch.indexOf(pts[i]) !== -1
}

I realise that hash probably isn't the best term here, a lookup might be less confusing.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 16, 2018

So, we could try to "merge" the testers

Though we need to be careful to keep the order of select operations. E.g. (i) lasso select a point and (ii) subtract click-select a point leads to a different result than the other way round...

etpinard
Copy link
Contributor

etpinard commented on 2fbaaaa Aug 16, 2018

Though we need to be careful to keep the order of select operations.

Good point. I was just thinking out loud.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 16, 2018

:-) me too. Wanted to add it to the discussion before I forget it again...

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 2fbaaaa Aug 29, 2018

The reason not to use pointNumber for histograms is it doesn't correspond to an index in the data arrays - that's what it puts in pointNumbers. Worth discussing (for v2) but it's not an accident, and the same rationale could certainly pop up in other trace types down the line.

rmoestl
Copy link
Contributor

rmoestl commented on 2fbaaaa Aug 30, 2018

I should have revisited that TODO earlier replacing it with an explanatory comment. It was more a work-in-progress TODO which I usually resolve before it makes it to a PR.

rmoestl added 21 commits Aug 16, 2018
- Set hoverMode to 'closest' where needed.
- Instead of an expandingIndex to identify the actual trace
  wanted, expecting a searchInfo object.
- Reason: the selection cache wasn't cleared when a user clicked the
  last selected point with Shift and thus a new selection started with
  Shift would add to the still cached selection testers from previous
  selection.
- Ensure click-to-select cleanly clears and starts selections
  although add/subtract mode on.
- Ensure double click in lasso and select mode cleanly clears
  and restarts selections when add/subtract mode on.
- TODO: deactivated a mouseout listener in geo.js cause
  with it, click-to-select won't work. To be reviewed.
- Include additional for choropleth that's based on geo.
- One double click location in an existing choropleth test
  needed to change though to be on an empty area that
  is not a world country.
- Reason: some tests ran into async timeout issues on CI.
- Thereby fixed bug in geo base plot: set a subplot id in dragOptions
  so that selection cache isn't coerced each time an interaction
  happens.
@etpinard
Copy link
Contributor

@etpinard etpinard commented Aug 31, 2018

@rmoestl about #2944 (comment). First, thanks very much for the detailed response.

Thinking about this again, I'm starting to believe that the current behavior isn't that bad. Or, in other words, fixing it won't be worth it. The current behavior only looks odd when users shift-select pt in zoom or pan mode before actually zoom or panning. Clearing selection state during prepFn is what we want until find a solution to your (1) pt, but that's for another PR. Consider my comment as non-blocking.

rmoestl added 2 commits Sep 4, 2018
- Wait until 'plotly_click' event is fired before checking that
  no point has been selected.
test/jasmine/tests/select_test.js Outdated Show resolved Hide resolved
test/jasmine/tests/select_test.js Outdated Show resolved Hide resolved
test/jasmine/tests/select_test.js Outdated Show resolved Hide resolved
@@ -16,6 +16,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);
}

coerce('clickmode');
Copy link
Contributor

@etpinard etpinard Sep 6, 2018

We're still missing @alexcjohnson's smart default suggestion where when a user sets clickmode: 'select' the default hovermode value becomes 'closest'.

Copy link
Contributor Author

@rmoestl rmoestl Sep 6, 2018

Oh, sorry, I misunderstood that. I thought mentioning in clickmode's description that hovermode set to closest is best for most applications was "enough".

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Copy link
Contributor

@alexcjohnson alexcjohnson Sep 7, 2018

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Yep! Which should be as simple as modifying the hovermodeDflt logic below.

Copy link
Contributor Author

@rmoestl rmoestl Sep 7, 2018

Done!

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

Thanks for adding defaults to this description!

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

minor: clickmode = coerce('clickmode') -> clickmode.indexOf('select') > -1

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

ha, even tests for the existing behavior 🎉 🏆

rmoestl
Copy link
Contributor

rmoestl commented on 87d0497 Sep 7, 2018

Ah, thx. Now, having tests paid off quickly 😆

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

Thanks for adding defaults to this description!

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

minor: clickmode = coerce('clickmode') -> clickmode.indexOf('select') > -1

alexcjohnson
Copy link
Contributor

alexcjohnson commented on 87d0497 Sep 7, 2018

ha, even tests for the existing behavior 🎉 🏆

rmoestl
Copy link
Contributor

rmoestl commented on 87d0497 Sep 7, 2018

Ah, thx. Now, having tests paid off quickly 😆

src/lib/polygon.js Outdated Show resolved Hide resolved
- Reason: it's pretty sure it'll no longer be called.
- See #2944 (comment)
@etpinard
Copy link
Contributor

@etpinard etpinard commented Sep 10, 2018

Amazing work @rmoestl . A hard-fought 💃

This thing will be in the running for PR of the year 🏆

@rmoestl
Copy link
Contributor Author

@rmoestl rmoestl commented Sep 10, 2018

🙇‍♂️ Thanks for your support, @etpinard and @alexcjohnson!

@rmoestl rmoestl merged commit fdae65b into master Sep 10, 2018
6 checks passed
@rmoestl rmoestl deleted the 1852-persistent-click-via-selectPoints branch Sep 10, 2018
@etpinard etpinard mentioned this pull request Sep 12, 2018
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants