-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
base: main
Are you sure you want to change the base?
Conversation
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.
|
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. |
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
and then look at |
Edit: I think I mixed this up with discussion related to an error. Disregard the above comment about unit tests. |
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 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: mapbox-gl-js/src/data/bucket/line_bucket.js Lines 349 to 357 in 36533f3
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);
}
}
By adding the above to your line that fills in an invalid normal, I managed to get to something like the following: 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 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--; | ||
} |
There was a problem hiding this comment.
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 👍
Could you add this fix to v1 too? |
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.
Launch Checklist
@mapbox/map-design-team
@mapbox/static-apis
if this PR includes style spec API or visual changes@mapbox/gl-native
if this PR includes shader changes or needs a native portmapbox-gl-js
changelog:<changelog></changelog>