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

Scattergl selection / clustering fix #2593

Merged
merged 13 commits into from
May 1, 2018
Merged

Scattergl selection / clustering fix #2593

merged 13 commits into from
May 1, 2018

Conversation

dy
Copy link
Contributor

@dy dy commented Apr 30, 2018

Fixes #2586 (both issues), fixes #2334, fixes #2543, fixes #2457

Changes:

image

@etpinard
Copy link
Contributor

Sweet!

@dy can you explain what changes you needed to make to fix the problem?

@etpinard
Copy link
Contributor

Amazing. I'll run a few performance tests tomorrow morning to see these changes have a side-effect.

@etpinard etpinard added bug something broken status: reviewable labels Apr 30, 2018
@etpinard
Copy link
Contributor

In the meantime @dy, can you confirm that this branch works in IE11?

@dy
Copy link
Contributor Author

dy commented Apr 30, 2018

Sure
image

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Moreover @dy, can you test out the other regl components (regl-line2d, regl-error2d, and sploms) in Safari?

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

@dy I'm noticing differences in behavior using 1.36.1 and this branch off https://codepen.io/anon/pen/zjoWpO?editors=0010

in 1.36.1:

peek 2018-05-01 09-46

this branch:

peek 2018-05-01 09-47

where it looks like 1.36.1 clustered things a little more aggressively making dimmed points appears when selection on a 1e5 pt graph. Is this expected?

@dy
Copy link
Contributor Author

dy commented May 1, 2018

Yes, that is the result of additional levels of clustering rendered due to #2586 mentioned by Alex

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

In terms of performance, comparing 1.36.1 and this branch:

  • pan is slower at 1e5 (nothing too noticeable though),

  • but pan is significantly slower at 1e6 points:

1.36.1:

peek 2018-05-01 09-55

this branch:

peek 2018-05-01 09-56

I'd even say panning off this branch perform a little worse than in 1.35.2: https://codepen.io/etpinard/pen/MGpRdW?editors=0010

  • no significant slow down during selection comparing 1.36.1 and this branch.

So, @dy can you explain why panning is affected by your patches here? Does it have to?

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Oh and can you add a mock that lock down the fix for #2334? Thanks!

@dy
Copy link
Contributor Author

dy commented May 1, 2018

I imagine there might be a way of adaptive opacity based on the density of a cluster, but that better be addressed as a separate issue.

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

I imagine there might be a way of adaptive opacity based on the density of a cluster, but that better be addressed as a separate issue.

Ok, but why does a selection fix affect panning?

@dy
Copy link
Contributor Author

dy commented May 1, 2018

Slow panning is the direct result of increased number of points caused by #2334 fix. Although it should not be significantly slower than snap-points-2d in 1.35, but possibly can. I did not notice fps drop though. That is the result of z-ordering a point-cluster tree instead of sorting by x, which saves ~100ms of sorting routine on init by displaying extra 10-15% points in runtime.
It is possible to enable x-sorting though to get 1.35 results.

@dy
Copy link
Contributor Author

dy commented May 1, 2018

Although there might be another reason - non-square clustering. It needs a bit of experiment though.

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Thanks for the info @dy !

Can you describe what will happen in turns of performance improvement once (if 😉 ) regl-project/regl#474 is fixed?

@dy
Copy link
Contributor Author

dy commented May 1, 2018

@etpinard 1k 1-point scattergl traces will be rendered ~6 times faster. If number of points per trace more than 1, it is not that noticeable.

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Ok. Is there anything we can do to improve panning performance at 1e6 pts?

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Off this branch, zooming in repetitively

peek 2018-05-01 14-32

leads to:

image

dy referenced this pull request in dy/point-cluster May 1, 2018
@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

Very strong panning perf off dbd6049:

peek 2018-05-01 17-24

the bug from #2593 (comment) appears to be fixed too 🎉

@etpinard etpinard added this to the 1.37.0 milestone May 1, 2018
@@ -549,6 +549,21 @@ describe('@gl Test splom interactions:', function() {
.catch(failTest)
.then(done);
});

it('should work with typed arrays', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

locking a fix for #2585 from the new regl-splom version

@etpinard
Copy link
Contributor

etpinard commented May 1, 2018

All right. Tests are ✅ Let's merge this thing! 💃

🔈 Big shoutout to @dy for closing 5 issues in one PR. Huge! 🔈

1.37.0 will be out in the next few hours.

@etpinard etpinard merged commit 0bb032f into master May 1, 2018
@etpinard etpinard deleted the scattergl-fix-n branch May 1, 2018 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
2 participants