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

Add stroking to polygons #597

Merged
merged 8 commits into from
Jul 18, 2016
Merged

Add stroking to polygons #597

merged 8 commits into from
Jul 18, 2016

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Jul 11, 2016

This is done by creating a sub-feature of lines for the polygons if the polygon feature has any value other than false for the stroke style (either true or a function). Lines are created for each exterior and interior loop in the polygons, and are not set for interaction. Lines are created with the same starting and ending point.

Strokes can have all of the properties that lines get -- strokeWidth, strokeColor, strokeOpacity, strokeStyle. Lines are rendered with the same renderer as the polygons (and on the same level).

Polygons now have a flag to turn off filling the polygon (though the polygon is still created and still earcut, so this doesn't speed things up for polygons with fill = false).

It would be better to add a flag to the line feature to indicate a closed loop.

Changed the gl.object class to not re-subclass sceneObject, as all gl features are already subclasses of sceneObject.

This requires a vgl update to allow unbinding vertex data. That change requires an update to the quad feature.

Fixed a typo in the lineFeature.

This resolves issue #494.

This is done by creating a sub-feature of lines for the polygons if the polygon feature has any value other than false for the stroke style (either true or a function).  Lines are created for each exterior and interior loop in the polygons, and are not set for interaction.  Lines are created with the same starting and ending point.

Strokes can have all of the properties that lines get -- strokeWidth, strokeColor, strokeOpacity, strokeStyle.  Lines are rendered with the same renderer as the polygons (and on the same level).

Polygons now have a flag to turn off filling the polygon (though the polygon is still created and still earcut, so this doesn't speed things up for polygons with fill = false).

It would be better to add a flag to the line feature to indicate a closed loop.

Changed the gl.object class to not re-subclass sceneObject, as all gl features are already subclasses of sceneObject.

This requires a vgl update to allow unbinding vertex data.  That change requires an update to the quad feature.

Fixed a typo in the lineFeature.
@manthey
Copy link
Contributor Author

manthey commented Jul 11, 2016

This is only a WIP until a new VGL version is available.

@manthey manthey changed the title [WIP] Add stroking to polygons Add stroking to polygons Jul 11, 2016
@codecov-io
Copy link

codecov-io commented Jul 11, 2016

Current coverage is 80.63%

Merging #597 into master will increase coverage by 1.82%

@@             master       #597   diff @@
==========================================
  Files            82         82          
  Lines          7354       7417    +63   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           5796       5981   +185   
+ Misses         1558       1436   -122   
  Partials          0          0          

Powered by Codecov. Last updated by eb4c2b1...6d53729

@jbeezley
Copy link
Contributor

This looks good, I just need to fix a couple of the selenium tests before it goes in.

@jbeezley
Copy link
Contributor

I found out the changes here broke styling in the jsonReader class because the call signature of the stroke and fill styles changed to be per polygon rather than per vertex. The change makes sense because you can't interpolate a boolean.

Anyway, I updated the glMultiPolygon test to add strokes and fixed a couple of other issues that were causing spurious test failures. With these changes, it LGTM.

@jbeezley
Copy link
Contributor

One other thing I noticed that we should look into after this goes in is the quality of the stroking, which at the moment is pretty bad. See the screenshot attached, which is at display resolution (not zoomed in).

  1. Many line segments seem to be missing triangles. This could be due to numerical precision.
  2. It almost looks like the fill bleeds out beyond the stroke, but it could be some weird antialiasing effect.

screen shot 2016-07-16 at 1 24 37 pm

@manthey
Copy link
Contributor Author

manthey commented Jul 18, 2016

I filed issue #596 about the way lines are rendered. They are rendered after the fill, so should never underneath it. You'll see the exact same lines (and same errors) with a fill opacity of 0. It appears like the vertices of some of the triangles are wrong due to an error in how mitering is calculated.

In calculating angles related to lines, two angles were averaged using (a + b) / 2, which doesn't work in the general case.
@manthey
Copy link
Contributor Author

manthey commented Jul 18, 2016

@jbeezley Can you check if the selenium tests need to be updated after the line fix?

@jbeezley
Copy link
Contributor

Done. LGTM.

@manthey manthey merged commit 4972064 into master Jul 18, 2016
@manthey manthey deleted the stroke-polygons branch July 18, 2016 19:49
@@ -96,6 +94,8 @@ var gl_lineFeature = function (arg) {
' else anglePrev = atan(deltaPrev.y / aspect, deltaPrev.x);',
' if (deltaNext.xy == vec2(0.0, 0.0)) angleNext = anglePrev;',
' float angle = (anglePrev + angleNext) / 2.0;',
' if (abs(anglePrev - angleNext) >= PI)',
' angle += PI;',
Copy link
Member

Choose a reason for hiding this comment

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

+1 👍

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.

4 participants