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

Meetar/line offsets #547

Closed
wants to merge 44 commits into from
Closed

Meetar/line offsets #547

wants to merge 44 commits into from

Conversation

meetar
Copy link
Member

@meetar meetar commented Apr 17, 2017

No description provided.

@meetar
Copy link
Member Author

meetar commented Apr 17, 2017

This technique works pretty well for a constant offset, with one caveat – in places where multiple points create a curve with a radius r, if the offset is larger than r the curve will become inverted:

angles2

So the style would have to ensure this doesn't happen by setting appropriate offsets at various zoom levels.

As I understand the situation there isn't really any way around this without some kind of smarter context-aware clipping algorithm à la Clipper or this paper, neither of which would be ~easily compatible with our vertex shader… but I'm happy to be proven wrong :)

@nvkelso
Copy link
Member

nvkelso commented Apr 17, 2017

We use Clipper for some geom validation tasks in Tilezen.

Love this illustration :)

screen shot 2017-04-17 at 12 16 14

demos/test.json Outdated
@@ -0,0 +1,30 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

We should remove test files from final PR

demos/test2.json Outdated
@@ -0,0 +1,98 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Ditto let's not merge test files

@@ -21,7 +21,7 @@ export default class RenderStateManager {
this.defaults = {};

// Culling
this.defaults.culling = true;
this.defaults.culling = false;
Copy link
Member

Choose a reason for hiding this comment

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

Need to turn culling back on

@@ -237,6 +241,7 @@ Object.assign(Lines, {
draw.width = StyleParser.createPropertyCache(draw.width, StyleParser.parseUnits);
draw.next_width = StyleParser.createPropertyCache(draw.width, StyleParser.parseUnits); // width will be computed for next zoom
draw.z = StyleParser.createPropertyCache(draw.z, StyleParser.parseUnits);
draw.offset = StyleParser.createPropertyCache(draw.offset || 0, StyleParser.parseUnits);
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to only accept px units (which I think we decided on), we should enforce that, probably here (first time the parameter is processed). Will need to double-check, but I think we should be able to do that by checking (if draw.offset isn't null) that draw.offset.value.units === 'px', and if not, log a warning and setting draw.offset to null. We may also want to add the ability to specify different default units for parseUnits though, so that offset values can have pixels as default units (if we decide we want that).

@@ -30,6 +30,8 @@ attribute vec4 a_color;
// z: half-width of line (amount to extrude)
// w: scaling factor for interpolating width between zooms
attribute vec4 a_extrude;
// xy: direction of line, for getting perpendicular offset
attribute vec4 a_offset;
Copy link
Member

Choose a reason for hiding this comment

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

offset is only a vec2

// Adjust line width based on zoom level, to prevent proxied lines from being either too small or too big.
// "Flattens" the zoom between 1-2 to peg it to 1 (keeps lines from prematurely shrinking), then interpolate
// and clamp to 4 (keeps lines from becoming too small when far away).
vec2 offset = a_offset.xy; // values have an 8-bit fraction;
Copy link
Member

Choose a reason for hiding this comment

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

I think 8-bit fraction comment is no longer accurate since we're not multiplying/un-multiplying this value anymore

@@ -77,27 +79,36 @@ void main() {
vec4 position = vec4(a_position.xy, a_position.z / TANGRAM_HEIGHT_SCALE, 1.); // convert height back to meters

#ifdef TANGRAM_EXTRUDE_LINES
// vec2 extrude = a_extrude.xy / 256.; // values have an 8-bit fraction
Copy link
Member

Choose a reason for hiding this comment

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

remove old line


if (cross) { // ccw bend
// pivot
addVertex(coord, Vector.neg(eC), false, uC, context, -1);
Copy link
Member

Choose a reason for hiding this comment

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

Bevel joins are broken - expected third parameter of addVertex() should be the offset vector (line's 2d normal), but it's being set to false here (not sure what the correct vector value should be).

@bcamper bcamper added this to the v0.14.0 milestone Jun 20, 2017
@bcamper
Copy link
Member

bcamper commented Jun 20, 2017

@bcamper
Copy link
Member

bcamper commented Oct 2, 2017

Merged to master via mesh-variants-rebase branch

🎊

@bcamper bcamper closed this Oct 2, 2017
@bcamper bcamper deleted the meetar/line-offsets branch October 2, 2017 20:46
@meetar
Copy link
Member Author

meetar commented Oct 19, 2017

Merged to master via mesh-variants-rebase branch

Prepping the docs for this – is there an associated PR or commit I can refer to?

@bcamper
Copy link
Member

bcamper commented Oct 19, 2017

It's all internals beyond the line offsets themselves. (Separately there's related work for specifying texture and line dash patterns in draw blocks, but I think we already covered that for docs elsewhere.)

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.

3 participants