WebGL Implementation of main plotting interface in Grapher. #223

adamcw · 2016-07-12T00:02:48Z

Resolves issues #185 and #208. Makes use of WebGL to do the data plotting portion of each graph in order to greatly improve the performance of live data view as well as zoom/pan operations.

d3.js was retained for the axes and and interface as SVG, while THREE.js library was introduced to handle the plotting of the data via WebGL.

This dual library usage was used to get the most performance gain for the least development time, as implementing axes and other features in WebGL will be time consuming. It is a longer term goal to move all plotting towards WebGL in order to allow screenshotting (Issue #184).

Performance was also improved by causing only the parts of the graph that need to be re-rendered under actions like zooming to be re-rendered. This also makes it possible to now retain the zoom level during live updates, the changing of plotting types, and the toggling of traces.

Automatic reordering by node and jspm Updated d3 to v3.5.17 Progress to THREE support Rough first pass at WebGL graphing intergration using THREE.js with d3 Got dots, rectfill and cargrid modes all working. Got 1D plots to work with WebGL Updating code to Google style standards.

…feature is beyond this PR

…in one PR than necessary.

…n a number of points to represent all the vertices

adamcw · 2016-07-12T00:05:19Z

client-js/app/elements/labrad-plot.ts

+/**
+ * Import the THREE namespace from the module.
+ */
+const THREE = three.default;


This throws a compiler error, but was the only way I could get the library to play nicely with TypeScript. If someone can work out how to import the library without this hack, please let me know, I wasted a few hours trying and ultimately moved on.

The problem here is that the typescript definition files for these libraries (and most libraries actually) were set up assuming they would be imported using CommonJS style imports like in node, rather than es6 imports. To fix this, you have to change the typings file slightly, like so: 7096169

Yes, I did track down that I could probably fix it by changing the typings file, but I didn't like the idea of changing it manually. Doing this will lead to problems whenever someone tries to update the typings in the future without knowing to update the format of the exports.

I agree it's not ideal to keep these local changes, but this is the reason why we've got the .d.ts definition files checked into the repo, rather than installing them using one of the tools to manage typescript definitions (originally it was tsd, then typings, now the latest thing is to install types packages from npm like @types/three, who knows what it will be next month...). In practice we don't update these definitions very often, but if we do make an update and forget to change the module declaration, at least we get compiler errors to remind us to fix it :-)

I think I will leave this line as-is, then submit another PR making the required typings changes and then removing this line.

adamcw · 2016-07-12T01:03:34Z

I have mostly created this PR to start a dialog on breaking it up further. I figured this would be best done as a semi-team process as this sort of thing is likely to be common as large scale refactors/redesigns become required throughout the codebase(s).

This is ultimately the smallest PR that maintains feature parity as well as builds/runs. This opens the question of if stacking smaller, non-building, PRs may be preferred where they are only merged once all have been reviewed and approved. I don't believe this concept is currently an accepted workflow so I thought a discussion first may be useful.

maffoo · 2016-07-12T03:50:50Z

I get a couple of typescript compilation errors, but they're not too hard to fix: e341a33

adamcw · 2016-07-12T03:56:37Z

Interesting, I do not get any compilation warnings. This might suggest we have different versions of TypeScript running?

maffoo · 2016-07-12T13:27:44Z

client-js/app/elements/labrad-plot.ts

+  /**
+   * Resets the current zoom level to fit the data.
+   */
+  resetZoomControl() {


Looks like this is not referenced anywhere.

You mean attached()? or resetZoomControl()?

attached() is called when the component loads.
resetZoomControl() is called from the html UI element.

I meant resetZoomControl(). I don't see it referenced anywhere here or in the element html (https://github.com/labrad/scalabrad-web/blob/185-graphing-performance-webgl/client-js/app/elements/labrad-plot.html); am I missing something?

Ahh, I must have merged it poorly. The intent was to have resetZoom be private, then have resetZoomControl be the pubic hook the UI uses.

It's basically a separation of the API the UI calls and the API the internal module calls.

Ultimately, it's a bit of a waste since it's just an alias, but the idea would be that the UI hooks can be altered independently, as well as having UI elements call descriptive names from the HTML, while internally calling a UI independent name.

adamcw · 2016-07-13T16:47:14Z

@joshmutus 2D Datasets not working is a strange regression, I'm looking into it now.

The zoom rectangle will probably need to be rendered in WebGL rather than SVG. I could render it as a second SVG layer, but since we want to move everything toward WebGL anyway, may as well do it that way now.

joshmutus · 2016-07-13T22:23:19Z

@adamcw Where you able to recreate the behaviour? I can send you the dataset that chokes up on my system if you like

… nicer implementation

adamcw · 2016-07-13T22:42:55Z

I've fixed the rendering portion if that is what you refer to. I restructured the rendering pipeline due to comments and it meant that not everything was updated when it had to be after new data was added.

joshmutus · 2016-07-13T23:04:38Z

@adamcw renders super nicely for large datasets now. Really really snappy. Thanks.

I have a weird problem, the plot doesn't show up when I'm looking at the app through remote desktop... None of the plot widget loads at all. Neither the axes or the data:

Renders fine on the same machine, just not through Chrome remote desktop session:

Any idea what could be happening there?

adamcw · 2016-07-13T23:06:42Z

That error is when WebGL is disabled in the browser (I've encountered it before trying to do headless testing of WebGL applications). For some reason via remote desktop, WebGL is being disabled, Chrome is probably detecting that you don't have the ability to do proper hardware acceleration through the Remote Desktop and shuts it off.

joshmutus · 2016-07-13T23:13:04Z

Yikes. Is there a way to force it on? This might be inconvenient if you have to remote desktop into a system to access the webserver, but I suppose that should never happen?

joshmutus · 2016-07-13T23:17:58Z

This did the trick. We might want to catch this error and bring up a dialog to point to this workaround.

Step 1: Open Google Chrome
Step 2: Type chrome://flags in the address bar
Step 3: Press Ctrl + f and type ” Rendering list “, “Override software rendering list” should come up, >Now click on Enable and restart the browser.

adamcw · 2016-07-14T02:59:24Z

I determined, that since the zoomRectangle will never be in screenshots anyway, there was no reason not to just render it as a div on top of the plot. PTAL.

…other performance improvements.

maffoo · 2016-07-15T02:42:32Z

client-js/app/elements/labrad-plot.ts

+   * Fires when the component is attached to the DOM.
+   */
+  attached(): void {
+    this.plotID = Math.random();


Typescript compiler throws an error here.

Line shouldn't have even been there, fixed.

maffoo · 2016-07-15T18:36:10Z

client-js/app/elements/labrad-plot.ts

@@ -1,9 +1,25 @@
 import 'd3';
-
+import {THREE} from 'three';


I get runtime errors from this. I think this needs to get the default import, rather than an item named 'THREE' from inside the module. That is:

import THREE from 'three';

maffoo · 2016-07-15T18:36:57Z

One comment about import (ts compiler doesn't complain, but it fails at runtime; we can investigate this separately). Otherwise LGTM.

adamcw added 13 commits June 30, 2016 19:00

Merged master

9ee7051

Fixed color bar when plot resizes

39ff547

Moved all sizing code from initialization into just resize

7033257

Code cleanup

8024f82

Fixed limits to prevent zoom reset when changing 2D modes

e795ebb

Fixed errors when rendering 1D plots due to no zAxis

245c8da

Removed extraneous paramter to redrawScene and restored public status

fcfb30a

Changed all this calls to p inside initializeSVGPlot for consistency

9c47a06

Removed dotScaleSize as it was added to allow a scaling UI, but this …

0ed4446

…feature is beyond this PR

Put extraneous space back into gulpfile to avoid touching more files …

77a6fe4

…in one PR than necessary.

Converted numVertices to actually be a number of vertices, rather tha…

19b0d39

…n a number of points to represent all the vertices

Clarifying comments

0f1aa1a

adamcw assigned maffoo and joshmutus Jul 12, 2016

adamcw reviewed Jul 12, 2016
View reviewed changes

Fixed mistakes

8dad16b

maffoo reviewed Jul 12, 2016
View reviewed changes

adamcw added 3 commits July 13, 2016 10:22

Fix graph resizing issues when lots of data points are available

2ad3568

Fix positioning of RectFill point

e7ca055

Fix issue with 1D plots becoming offset

ef188f7

Replace broken and horrible Point sizing code with a working and much…

51118a7

… nicer implementation

Fixed zoomRectangle

699174d

adamcw added 3 commits July 14, 2016 11:01

Fix no zoom reset when a scroll has occurred

9953786

Fix color scaling to update every frame rather than being fixed.

8b19c33

Merged Master

6ef83d0

adamcw mentioned this pull request Jul 14, 2016

Auto-scaling behavior when new points are added #231

Closed

adamcw added this to the v2.0 milestone Jul 14, 2016

adamcw added 2 commits July 14, 2016 15:19

Moved various functions into the render loop for efficiency, amongst …

e030599

…other performance improvements.

Properly defer graph projection when resizing a plot

ea8e66b

maffoo reviewed Jul 15, 2016
View reviewed changes

adamcw and others added 4 commits July 14, 2016 20:10

Removed plotID used for debug

50fe660

Merge branch 'master' into 185-graphing-performance-webgl

b49c5f1

Merge branch 'master' into 185-graphing-performance-webgl

b633e94

Updated labrad-plot to use new ES6 import for THREEjs

e119b9e

maffoo reviewed Jul 15, 2016
View reviewed changes

Get THREE js to import and compile nicely

4b4f684

adamcw merged commit 7d46390 into master Jul 15, 2016

adamcw deleted the 185-graphing-performance-webgl branch July 15, 2016 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebGL Implementation of main plotting interface in Grapher. #223

WebGL Implementation of main plotting interface in Grapher. #223

adamcw commented Jul 12, 2016

adamcw Jul 12, 2016

maffoo Jul 12, 2016

adamcw Jul 12, 2016

maffoo Jul 12, 2016

adamcw Jul 12, 2016

adamcw commented Jul 12, 2016

maffoo commented Jul 12, 2016

adamcw commented Jul 12, 2016

maffoo Jul 12, 2016

adamcw Jul 12, 2016

maffoo Jul 12, 2016

adamcw Jul 12, 2016

adamcw commented Jul 13, 2016 •

edited

Loading

joshmutus commented Jul 13, 2016

adamcw commented Jul 13, 2016

joshmutus commented Jul 13, 2016

adamcw commented Jul 13, 2016

joshmutus commented Jul 13, 2016

joshmutus commented Jul 13, 2016

adamcw commented Jul 14, 2016

maffoo Jul 15, 2016

adamcw Jul 15, 2016

maffoo Jul 15, 2016

maffoo commented Jul 15, 2016

WebGL Implementation of main plotting interface in Grapher. #223

WebGL Implementation of main plotting interface in Grapher. #223

Conversation

adamcw commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcw commented Jul 12, 2016

maffoo commented Jul 12, 2016

adamcw commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamcw commented Jul 13, 2016 • edited Loading

joshmutus commented Jul 13, 2016

adamcw commented Jul 13, 2016

joshmutus commented Jul 13, 2016

adamcw commented Jul 13, 2016

joshmutus commented Jul 13, 2016

joshmutus commented Jul 13, 2016

adamcw commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maffoo commented Jul 15, 2016

adamcw commented Jul 13, 2016 •

edited

Loading