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 support for a boolean Line layout property called line-collapse. #10898

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krisdages
Copy link

Add support for a boolean Line layout property called line-collapse.
When true, lines or polygons will collapse to points if they have
equal vertices instead of being considered "invalid geometry" and not rendering.

Default value of line-collapse is false.

Re: #8635
I'm opening this PR to "resurface the discussion" as requested in the issue above, but I still need to look into how to create a render test for the changes, and how to address the other bullets in the checklist.

I did a little more digging. Tolerance did get past the issue with geojson-vt, but then the layer rendering itself was dropping the feature if the vertices of the line were the same.

I played around with it and tried adding a layout property for line-collapse where it would let the line render even if both vertices were the same. This fixed the issue I was having with single line segments and round endcaps.

Issues:

Didn't try much to make things work for polygons since I'm not too familiar with them and this is just a proof of concept
Collapsed lines with multiple segments can get chopped in half when they cross tile borders
Square endcaps on diagonal lines lose their orientation when the vertices are equal, as I just defaulted to using a horizontal normal vector instead of trying to find a way to compute the actual angle.

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

krisdages and others added 2 commits July 28, 2021 13:40
When true, lines or polygons will collapse to points if they have
equal vertices instead of being considered "invalid geometry" and not rendering.

Default value of `line-collapse` is false.
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@krisdages
Copy link
Author

If anyone can recommend a resource for how to implement tests for rendering styles (generating that 'expected.png', etc), that would be great... I'd rather not waste anyone's time doing it wrong.

@rreusser
Copy link
Contributor

Thanks for the PR, @krisdages! I have to catch up and start processing the content, but as for running the tests, Writing new tests should have the necessary commands.

(I don't have all the context yet, but it may be the case that if the PR modifies error states without changing any appearance, a unit test would be preferable to a render test.)

To implement a render test, I usually start by browsing the test/integration/render-tests directory and looking for something close to what I'm after. To iterate on it, I believe the best way is to prepend UPDATE=1, for example:

UPDATE=1 yarn run test-render tests=fog/2d/basic

and then look at expected.png in that directory. (I think the docs omit tests= which causes it to run all tests. I'll check that and fix, if that's indeed the case.) All that should be necessary to add a render test is a new directory in render-tests with style.json and expected.png.

@rreusser
Copy link
Contributor

Edit: I think I mixed this up with discussion related to an error. Disregard the above comment about unit tests.

@rreusser
Copy link
Contributor

rreusser commented Jul 28, 2021

Hi again, @krisdages! This really got me digging, and I thought I'd offer an idea.

For a start, as a general rule, we really try to avoid adding style properties whenever possible. It makes a gentle opt-in to avoid changes considered breaking, but it also requires documentation, UI controls in Studio, long-term maintenance, and is very hard to remove later. If possible, I'd love to interpret #8635 this as a "bug" to be fixed.

I wrote a fixture which might help explore the behavior. You're welcome to use as much or as little as you like!

line-triangulation/degenerate/style.json
{
  "version": 8,
  "metadata": {
    "test": {
      "width": 256,
      "height": 256
    }
  },
  "center": [
    0,
    0
  ],
  "zoom": 0,
  "sources": {
    "geojson-with-tolerance": {
      "type": "geojson",
      "data": {
        "type": "FeatureCollection",
        "features":  [
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-20, -30],
                [-20, -30]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-19.999999, -20],
                [-20.000001, -20]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-19.9999, -10],
                [-20.0001, -10]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-19.99, 0],
                [-20.01, 0]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-19.9, 10],
                [-20.1, 10]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-19, 20],
                [-21, 20]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [-5, 30],
                [-35, 30]
              ]
            }
          }
        ]
      },
      "tolerance": 0
    },
    "geojson-without-tolerance": {
      "type": "geojson",
      "data": {
        "type": "FeatureCollection",
        "features":  [
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [20, -30],
                [20, -30]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [19.999999, -20],
                [20.000001, -20]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [19.9999, -10],
                [20.0001, -10]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [19.99, 0],
                [20.01, 0]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [19.9, 10],
                [20.1, 10]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [19, 20],
                [21, 20]
              ]
            }
          },
          {
            "type":"Feature",
            "geometry": {
              "type":"LineString",
              "coordinates": [
                [5, 30],
                [35, 30]
              ]
            }
          }
        ]
      }
    }
  },
  "layers": [
    {
      "id": "background",
      "type": "background",
      "paint": {
        "background-color": "#FFF"
      }
    },
    {
      "id": "lhs-geojson-with-tolerance",
      "type": "line",
      "source": "geojson-with-tolerance",
      "layout": {
        "line-cap": "round",
        "line-join": "round"
      },
      "paint": {
        "line-width": 8
      }
    },
    {
      "id": "rhs-geojson-without-tolerance",
      "type": "line",
      "source": "geojson-without-tolerance",
      "layout": {
        "line-cap": "round",
        "line-join": "round"
      },
      "paint": {
        "line-width": 8
      }
    }
  ]
}

The status quo behavior for seven lines, stacked vertically, of decreasing length is below. On the left is tolerance = 0 and on the right is tolerance = default (0.375?). You can clearly see that no matter the tolerance, not more than three are rendered.

expected

In working through what's causing this, I wondered if this block might be problematic as it forces degenerate (identical endpoint) segments to nothingness, regardless of the tolerance:

// If the line has duplicate vertices at the ends, adjust start/length to remove them.
let len = vertices.length;
while (len >= 2 && vertices[len - 1].equals(vertices[len - 2])) {
len--;
}
let first = 0;
while (first < len - 1 && vertices[first].equals(vertices[first + 1])) {
first++;
}

Is something like the following necessary after that block to retroactively permit two identical vertices?

if (!isPolygon && len - first < 2) {
  // Expand the range by one in either direction, if possible
  if (first) {
    first--;
  } else {
    len = Math.min(len + 1, vertices.length);
  }
}

Hmm… I'm not completely sure whether the line code handles a single vertex and can add line caps or whether it requires a minimum of two vertices…… Edit: looks like it needs at least two vertices, even though one and a normal should technically be enough to render bare line caps.

By adding the above to your line that fills in an invalid normal, I managed to get to something like the following:

expected 2

You can see that the tolerance still has an effect, but by zeroing it out, you can at least force degenerate segments. I wish you didn't have to disable simplification in this manner in order to get the benefits though…

In short, I managed to make some progress and figure out some things based on your work, but I'm not 100% certain the full solution here. Oh, and I should mention that I also started digging in the simplify function of geojson-vt, but it seemed like that wasn't where this simplification was going wrong…

https://github.com/mapbox/geojson-vt/blob/35f4ad75feed64e80ff2cd02994976c6335859cd/src/simplify.js#L4

In case it's helpful, you can see the full approach I tried out here: https://github.com/mapbox/mapbox-gl-js/compare/ricky/render-degenerate-lines?expand=1 As noted, it doesn't quite check all the boxes, so I hope it maybe helps with some ideas, but I'd definitely prefer to get your PR to completion if we can!

len++;
} else if (first > 0) {
first--;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, hold on……… I think I accomplished this in a slightly different but actually exactly equivalent way. 👍 👍 I need to process the polygon part a bit, but this block looks great to me 👍

@Lewik
Copy link

Lewik commented Oct 17, 2021

Could you add this fix to v1 too?
BTW: v1 allows tolerance: 0, but it is not real 0

@asheemmamoowala asheemmamoowala marked this pull request as draft March 31, 2022 16:47
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