Refactor GL line rendering #649

manthey · 2016-11-21T14:05:52Z

This resolves issues #204 and #596.

Previously, lines were not rendered correctly for several conditions: (a) miter locations were sometimes miscalculated, resulting in inconsistent line widths. (b) when the miter limit was exceeded, an attempt to prevent an excessively long line produced skewed segments. (c) On the inside of acute angles on short line segments, some of the triangles used to render lines were twisted. (d) when lines overlap, the overlap was inconsistent.

All of this has been fixed. Additionally, we now support:

line caps: butt (default and what was avaulable before), round, square. These can vary by point.
line joins: miter (default), round, bevel, miter-clip. These can vary by point. Miter and miter-clip have a configurable miter limit (one value for the whole feature) which, if the miter would be longer than this, switches to using a bevel or a clipped miter.
antialiasing on edges and end caps. This can be specified for the whole feature.
strokeOffset: This can vary from -1 (shift left) to 1 (shift right), where 0 is the line centered on the vertices, and 1 is with the edge of the line on the vertices.

All of this is done by rendering two triangles per line segment and performing much of the computation in the vertex and fragment shaders.

There are a few limitations to the current implementation:

there is finite precision in the calculations, resulting in occasional artifacts along miter joins. These are mostly only noticeable on very wide lines using stroke offsets, and, even then, are subtle.
When the line width changes per vertex, sometimes the inside joins are not technically correct. Instead of using the angle of the lines meeting, the angle of the edges of the lines meeting would need to be calculated.
On short line segments, one edge of the miter can be by itself, which would ideally either be antialiased or wouldn't be trimmed. Currently, on wide short lines with multiple segments, the mitered edge can look blocky in limited instances.
When a line segment has zero length in screen space, it is not drawn at all. Adjacent line segments may not be drawn exactly as expected. Also, if line caps are used, perhaps zero-length segments should still be visible.

This method uses 18/15 as much memory as lines used before.

There is a debug flag to show all of the pixels sent to the fragment shader. If debug is not specified when the feature is created, the debug code is not even compiled into the fragment shader.

There could be some efficiency improvements in the shaders. For instance, it might worth it to have a quick test for simple line segments without joins.

The new features and changes are exposed in one of the selenium tests (test/selenium/glLines/?wide=true), which can take a variety of query options, such as strokeOffset, lineCap, lineJoin, miterLimit, antialiasing, strokeWidth, strokeColor, strokeOpacity, and debug. This should be turned into an example that also tests number of lines.

This resolves issues #204 and #596. Previously, lines were not rendered correctly for several conditions: (a) miter locations were sometimes miscalculated, resulting in inconsistent line widths. (b) when the miter limit was exceeded, an attempt to prevent an excessively long line produced skewed segments. (c) On the inside of acute angles on short line segments, some of the triangles used to render lines were twisted. (d) when lines overlap, the overlap was inconsistent. All of this has been fixed. Additionally, we now support: - line caps: butt (default and what was avaulable before), round, square. These can vary by point. - line joins: miter (default), round, bevel, miter-clip. These can vary by point. Miter and miter-clip have a configurable miter limit (one value for the whole feature) which, if the miter would be longer than this, switches to using a bevel or a clipped miter. - antialiasing on edges and end caps. This can be specified for the whole feature. - strokeOffset: This can vary from -1 (shift left) to 1 (shift right), where 0 is the line centered on the vertices, and 1 is with the edge of the line on the vertices. All of this is done by rendering two triangles per line segment and performing much of the computation in the vertex and fragment shaders. There are a few limitations to the current implementation: - there is finite precision in the calculations, resulting in occasional artifacts along miter joins. These are mostly only noticeable on very wide lines using stroke offsets, and, even then, are subtle. - When the line width changes per vertex, sometimes the inside joins are not technically correct. Instead of using the angle of the lines meeting, the angle of the edges of the lines meeting would need to be calculated. - On short line segments, one edge of the miter can be by itself, which would ideally either be antialiased or wouldn't be trimmed. Currently, on wide short lines with multiple segments, the mitered edge can look blocky in limited instances. - When a line segment has zero length in screen space, it is not drawn at all. Adjacent line segments may not be drawn exactly as expected. Also, if line caps are used, perhaps zero-length segments should still be visible. This method uses 18/15 as much memory as lines used before. There is a debug flag to show all of the pixels sent to the fragment shader. If debug is not specified when the feature is created, the debug code is not even compiled into the fragment shader. There could be some efficiency improvements in the shaders. For instance, it might worth it to have a quick test for simple line segments without joins. The new features and changes are exposed in one of the selenium tests (test/selenium/glLines/?wide=true), which can take a variety of query options, such as strokeOffset, lineCap, lineJoin, miterLimit, antialiasing, strokeWidth, strokeColor, strokeOpacity, and debug. This should be turned into an example that also tests number of lines.

manthey · 2016-11-21T14:06:41Z

I'm marking this as a WIP until I add an example and add some additional tests.

codecov-io · 2016-11-21T14:10:32Z

Current coverage is 88.87% (diff: 100%)

Merging #649 into master will decrease coverage by 1.10%

@@             master       #649   diff @@
==========================================
  Files            83         84     +1   
  Lines          8365       8497   +132   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7527       7552    +25   
- Misses          838        945   +107   
  Partials          0          0

Powered by Codecov. Last update f55e449...dd7b0a6

…one in the fragment shader.

Better comments, another preset, the show map option is explicit.

Allow the selectionAPI to be turned on or off.

This caches line positions and widths, and avoids using functions and exceptions within the core loop. For 500,000 line segments a point search was taking around 1500 ms, and now takes 20 ms or so.

This was added to compare how canvas, svg, and our gl code render lines. It could probably be made more efficient by not calculating the geometry on every update.

Lines were not removed when data got shorter. svgs could cause scrollbars to appear.

…nts as a constant.

This still needs a WebGL image comparison test.

manthey · 2016-12-19T22:04:48Z

This is ready for review.

Don't set body css to hidden on image tests, as it messes up the karma debug.html output.

aashish24 · 2016-12-21T04:14:04Z

👏 👏 started the review today...hoping to get it done in next few days.

Clean up after some tests.

Eventually, this will facilitate testing examples.

Test the lines example. Add screenshot capabilities. This requires ImageMagick and expects the test to be run on the same computer or VM as the karma test server.

Example tests must have the examples built first. This required upgrading the node-fs-extra package.

…les test.

This removes the network dependency to openstreetmap.org in the tests. As additional examples are added, the tiles.tgz file will need to be updated.

…t function for screen tests.

Test examples

aashish24

I made my first pass on the review. Will make another pass.

aashish24 · 2017-01-03T16:51:07Z

src/canvas/lineFeature.js

+      cur.opacity = strokeOpacityFunc(line[0], 0, d, i);
+      cur.linecap = lineCapFunc(line[0], 0, d, i);
+      cur.linejoin = lineJoinFunc(line[0], 0, d, i);
+      cur.strokeStyle = 'rgba(' + (cur.color.g * 255) + ', ' + (cur.color.g * 255) + ', ' + (cur.color.b * 255) + ', ' + (cur.opacity !== undefined ? cur.opacity : 1) + ')';


@manthey should we wrap these long lines?

aashish24 · 2017-01-12T23:37:18Z

src/d3/lineFeature.js

@@ -105,6 +106,10 @@ var d3_lineFeature = function (arg) {

      m_renderer._drawFeatures(m_style);
    });
+    for (i = data.length; i < m_maxIdx; i += 1) {


Why this is required now?

It was always required; not having it was a bug. In the past, if you reduced the number of lines you had in the d3 renderer, they wouldn't be taken away.

aashish24 · 2017-01-12T23:45:51Z

src/feature.js

@@ -31,7 +31,7 @@ var feature = function (arg) {

  var m_this = this,
      s_exit = this._exit,
-      m_selectionAPI = arg.selectionAPI === undefined ? false : arg.selectionAPI,
+      m_selectionAPI = arg.selectionAPI === undefined ? false : !!arg.selectionAPI,


I have heard good and bad about using double negation (https://www.sitepoint.com/javascript-double-negation-trick-trouble/)

We could use Boolean() instead. !! is substantially faster than Boolean() in Chrome, though, and, it is a common enough javascript idiom, that I think it is fine.

Right !! Is fast and i read about that earlier. Inam fine with !! For now

I meant in chrome

manthey · 2017-01-13T13:59:05Z

@aashish24 One question I meant to ask before: do we want to set antialiasing to a default value? Currently, it defaults to 0. I think lines actually look best when it is 2 (though mathematically, I feel that sqrt(2) should look as good, it doesn't on nearly vertical lines).

aashish24 · 2017-01-14T01:46:38Z

+2 for a value of 2. I think thank makes sense.

manthey · 2017-01-16T18:44:10Z

I've pushed a change to make the default antialiasing 2.

aashish24 · 2017-01-17T15:29:30Z

Roger that @manthey working on my review

aashish24 · 2017-01-18T05:51:37Z

src/gl/lineFeature.js

+          '  int nearMode = int(mod(floor(flags / 4.0), 8.0));',
+          '  int farMode = int(mod(floor(flags / 32.0), 8.0));',
+          '  float offset = mod(floor(flags / 256.0), 2048.0) / 1023.0;',
+          '  if (offset > 1.0)  offset -= 2048.0 / 1023.0;',


would be nice to have explanation for 2048/1023.0 division

aashish24 · 2017-01-18T07:13:49Z

src/gl/lineFeature.js

+          'varying vec4 info;',  /* near mode, far mode, offset */
+          'varying vec4 angles;', /* near angle cos, sin, far angle cos, sin */
+
+          'const float PI = 3.14159265358979323846264;',


perhaps we can define it some where else (utils?) so that we don't have to define it multiple times.

Let's do that when we address issue #655 (in a different PR).

Alternately, we could use the value in javascript, i. e., 'const float PI = ' + Math.PI.toFixed(20) + ';'. We aren't guaranteed the same precision choices in javascript and WebGL, though. The differences almost certainly don't matter much here.

sounds good to me.

aashish24 · 2017-01-18T07:14:18Z

src/gl/lineFeature.js

+           * calculate the angles between the lines formed by prev-pos and
+           * pos-next, and between pos-next and next-far, plus know the angle
+           *   (prev)---(pos)---(next)---(far) => A---B---C---D */
+          '  vec4 A = projectionMatrix * modelViewMatrix * vec4(prev.xyz, 1);',


worth using a function?

Use a function for coordinate transformation in the vertex shader.

aashish24

LGTM 👍 👏 I did some basic check and as discussed with @manthey will do the shader review together as de-coding every math / logic is time consuming. Thank you for the patience and hard work @manthey

manthey changed the title ~~Refactor GL line rendering~~ [WIP] Refactor GL line rendering Nov 21, 2016

manthey added 13 commits November 21, 2016 09:39

Change the threshold for straight joins.

f660570

Increase the zone used with miter joints to ensure that clipping is d…

30ae945

…one in the fragment shader.

Merge branch 'master' into line-rendering

dd8203f

Sped up line creation by calling less functions.

32bd75f

Add example for lines.

29e2635

Make the osm layer optional on the lines example.

29af57e

Tweaks to the example.

d07b57b

Better comments, another preset, the show map option is explicit.

Add hover text to the lines example.

f338e00

Allow the selectionAPI to be turned on or off.

Speed up point search on lines.

64a345a

This caches line positions and widths, and avoids using functions and exceptions within the core loop. For 500,000 line segments a point search was taking around 1500 ms, and now takes 20 ms or so.

Merge branch 'master' into line-rendering

a656e9e

Support lineCap, lineJoin, and miterLimit in svg lines.

8c7255c

Support lines on the canvas renderer.

a05f30c

This was added to compare how canvas, svg, and our gl code render lines. It could probably be made more efficient by not calculating the geometry on every update.

Fix some issues in the d3 line renderer.

f875752

Lines were not removed when data got shorter. svgs could cause scrollbars to appear.

jbeezley mentioned this pull request Nov 28, 2016

Collection geojson properties on click Kitware/minerva#383

Merged

manthey added 3 commits December 1, 2016 16:21

Merge branch 'master' into line-rendering

be3c65d

Support renderer selection in the glLines test for comparison purposes.

1420881

Older versions of firefox can use trig functions with constant argume…

e9e7f10

…nts as a constant.

manthey force-pushed the line-rendering branch from 2f528e9 to e9e7f10 Compare December 5, 2016 18:44

manthey added 3 commits December 19, 2016 15:04

Merge branch 'master' into line-rendering

c635028

Add line tests.

81c54e9

This still needs a WebGL image comparison test.

Add an image test for webgl lines.

6d53e61

manthey changed the title ~~[WIP] Refactor GL line rendering~~ Refactor GL line rendering Dec 19, 2016

Remove the selenium glLines test.

62d5b48

Don't set body css to hidden on image tests, as it messes up the karma debug.html output.

Move the WebGL lines speed test to the headless webgl test method.

8bce80d

Clean up after some tests.

manthey added 3 commits January 1, 2017 20:24

Change how test data is proxied through karma.

42417cd

Eventually, this will facilitate testing examples.

Add the ability to test examples.

7eb7e53

Test the lines example. Add screenshot capabilities. This requires ImageMagick and expects the test to be run on the same computer or VM as the karma test server.

Fix travis build of example tests.

9bf511b

Example tests must have the examples built first. This required upgrading the node-fs-extra package.

manthey mentioned this pull request Jan 3, 2017

Test examples #658

Merged

manthey added 4 commits January 3, 2017 14:42

Use a proxy with firefox to avoid loading external files in the examp…

04f04cf

…les test.

Allow examples to use local files for tiles instead of external data.

71c146a

This removes the network dependency to openstreetmap.org in the tests. As additional examples are added, the tiles.tgz file will need to be updated.

Include imagemagick in the recommended provisioning. We use the impor…

f85f792

…t function for screen tests.

Merge pull request #658 from OpenGeoscience/test-examples

60b1b2b

Test examples

aashish24 reviewed Jan 12, 2017

View reviewed changes

Wrap some long lines.

87801cd

Set the default antialiasing to 2.

b8c9e26

manthey force-pushed the line-rendering branch from e2f7ed8 to b8c9e26 Compare January 16, 2017 16:04

aashish24 reviewed Jan 18, 2017

View reviewed changes

manthey added 2 commits January 18, 2017 08:46

Merge branch 'master' into line-rendering

c7c2db0

Add comments about packing values into flags.

dd7b0a6

Use a function for coordinate transformation in the vertex shader.

aashish24 approved these changes Jan 20, 2017

View reviewed changes

manthey merged commit d7b72e9 into master Jan 20, 2017

manthey deleted the line-rendering branch January 20, 2017 15:53

This was referenced Jan 20, 2017

Provide ability to draw anti-aliased lines in OpenGL #204

Closed

Line features in GL don't render correctly #596

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor GL line rendering #649

Refactor GL line rendering #649

manthey commented Nov 21, 2016

manthey commented Nov 21, 2016

codecov-io commented Nov 21, 2016 •

edited

Loading

manthey commented Dec 19, 2016

aashish24 commented Dec 21, 2016

aashish24 left a comment

aashish24 Jan 3, 2017

aashish24 Jan 12, 2017

manthey Jan 13, 2017

aashish24 Jan 14, 2017

aashish24 Jan 12, 2017

manthey Jan 13, 2017

aashish24 Jan 14, 2017

aashish24 Jan 14, 2017

manthey commented Jan 13, 2017 •

edited

Loading

aashish24 commented Jan 14, 2017

manthey commented Jan 16, 2017

aashish24 commented Jan 17, 2017

aashish24 Jan 18, 2017

aashish24 Jan 18, 2017

manthey Jan 18, 2017

aashish24 Jan 19, 2017

aashish24 Jan 18, 2017

manthey Jan 18, 2017

aashish24 left a comment

Refactor GL line rendering #649

Refactor GL line rendering #649

Conversation

manthey commented Nov 21, 2016

manthey commented Nov 21, 2016

codecov-io commented Nov 21, 2016 • edited Loading

Current coverage is 88.87% (diff: 100%)

manthey commented Dec 19, 2016

aashish24 commented Dec 21, 2016

aashish24 left a 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

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

manthey commented Jan 13, 2017 • edited Loading

aashish24 commented Jan 14, 2017

manthey commented Jan 16, 2017

aashish24 commented Jan 17, 2017

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

Choose a reason for hiding this comment

aashish24 left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 21, 2016 •

edited

Loading

manthey commented Jan 13, 2017 •

edited

Loading