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

point-cluster for scattergl #2493

Closed
wants to merge 6 commits into from
Closed

point-cluster for scattergl #2493

wants to merge 6 commits into from

Conversation

dy
Copy link
Contributor

@dy dy commented Mar 23, 2018

This PR includes improvement on regl-scatter2d & introduces point-clustering.
Btw selection, panning & updating are significantly faster.
Clustering is somewhat faster.
Introducing async clustering - later.

cc @etpinard

// create scatter instance by cloning scatter2d
scene.select2d = createScatter(fullLayout._glcanvas.data()[1].regl, {clone: scene.scatter2d});
// create select scatter instance by cloning scatter2d
scene.select2d = createScatter(fullLayout._glcanvas.data()[1].regl);
Copy link
Contributor Author

@dy dy Mar 23, 2018

Choose a reason for hiding this comment

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

Since we reuse clustering, creating new scatter is way faster, we don't need to clone data.

Copy link
Contributor

Choose a reason for hiding this comment

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

sweet!

Copy link
Contributor

Choose a reason for hiding this comment

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

... which makes switching from zoom/pan to select/lasso drag modes waaay faster. Thanks!

@etpinard etpinard changed the title Five minutes gl test on ci point-cluster for scattergl Mar 23, 2018
@etpinard
Copy link
Contributor

Thanks @dfcreative ! We should probably apply those commits on top of #2492 to avoid conflicts.

if(errorOpts.copy_ystyle) {
errorOpts = trace.error_y;
}
if (!trace.unselected) unselectedOptions.opacity = DESELECTDIM;
Copy link
Contributor

@etpinard etpinard Mar 23, 2018

Choose a reason for hiding this comment

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

Why do we need this all of a sudden?

@etpinard
Copy link
Contributor

etpinard commented Mar 23, 2018

I'm seeing ~200ms improvements in first-render speed at 1e6 pts (from 1400-1500 down to ~1200ms) which is pretty amazing 🎉

That said, on my machine drag and selection appear slower off this branch. Maybe one or a few dependencies haven't been updated properly. Oh wait! Yeah, this branch is still using the regl-scatter2d@2.1.17. I push a commit linking it to regl-scatter2d@master.

@etpinard
Copy link
Contributor

etpinard commented Mar 23, 2018

with d972023, the first-render improvements are even better, I'm getting <1000ms first-render time at 1e6 pts which is nothing short of outstanding 🎉

That said, yeah I'm still experiencing slow drag and selections. @dfcreative maybe we'll need to update parts of update routine.

@etpinard
Copy link
Contributor

etpinard commented Mar 23, 2018

@dfcreative I made scattergl-dry-convert...point-cluster-dev which I think captures of the required updates in scattergl and scatterpolargl of this branch and applies them to #2492 to avoid conflicts down the road. I suggest closing this PR and working off point-cluster-dev.

@dy dy closed this Mar 23, 2018
@dy dy deleted the five-minutes-gl-test-on-ci branch March 23, 2018 16:09
@etpinard
Copy link
Contributor

@dfcreative Here are some problems I noticed off scattergl-dry-convert...point-cluster-dev :

  • gl2d_12, gl2d_14, gl2d_axes_range_type, gl2d_error_bars_log, gl2d_layout_image, are erroring out with:

image

  • gl2d_marker_symbols errors out with:

image

  • most other mocks produces a diff (see: badbd59). With the exception of gl2d_scatter-color-clustering all those diff appear to be sub-pixel glitches 👌 . That said, the marker colors in gl2d_scatter-color-clustering are clearly different. @dfcreative was that on-purpose?

@etpinard
Copy link
Contributor

as for the jasmine tests, a few tests (6 in total) are failing due to the TypeError: Cannot read property 'offsets' of undefined problem mentioned ⤴️ . No other issues were detected so far.

@dy dy mentioned this pull request Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants