-
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
symbol-placement: line-center
#6821
Conversation
@mourner pointed out that maybe it makes sense to use Just looking at the image, I don't think there's much of a win in terms of developing an intuition for what the "center" of a line really means. On the other hand, using |
Agree. I think it's fine that line-center will require special consideration / prep of the underlying data. We already have a similar prep requirement for the much more basic task of labeling a polygon (without repeated labels across tiles). |
@ChrisLoer We have line geometries in the |
Yeah, if you could point me to a good tile in streets-v7 to test against (preferably one in which the label will go well past the edge of the tile), I can download that as a fixture and write a test case around it. For |
Tiles 4/4/7 and 4/5/7 are adjacent tiles that have a relevant line crossing the two of them.
I think this sounds right. What is the alternative – disabling By the way, some food for thought: the |
Either disabling it, or relaxing the "center" part and trying to re-sample until we find something close to the center that still fits. I think just hiding sounds right, though, I'm implementing it now.
Yeah that is good food for thought, although I don't feel like I could say anything very informed about it. I think implementation-wise it would not be too hard to make something like |
Here's a render of just 4/5/7: You can see the line for the Writing the test made me realize some problems, though... if we allow
🤔 I don't have an immediate strategy in mind for addressing those issues -- I think it would require something like a We could also try to avoid including the "same" label in multiple tiles, but then you'd have the downside that you'd depend on one particular tile getting loaded to show the label. |
Err, after thinking about this for a second more, it occurs to me that I probably just imagined the "draw symbols anchored outside the tile boundaries" requirement. I think the actual requirement may just be that the labels themselves should be able to draw across tile boundaries -- in which case we'd be able to skip the problems I just brought up. The only downside would be the same as in the "only include data in a single tile" case... The tile containing the center-point of the label would have to be loaded before a label could show, so for example you might expect a label that looked like "Carr..." clipped against the right edge of the screen, but it wouldn't start displaying until you moved a bit further to the right, you loaded the tile that contained the center of the label, and "Carribe..." started showing clipped against the right edge. |
@ChrisLoer – @ajashton and I both think this behavior is fine 👍 |
Yep, I agree. We already have this behavior for all non-centered labels so if we need to fix it we'll need to find a way that works for all labels. |
05c8f31
to
e33d2fa
Compare
symbol-placement: line-center
symbol-placement: line-center
OK, I think this is ready to go pending review/edits, then! @samanpwbb do you or anyone from Studio want to weigh in before we publish this? I think from a Studio UI point of view this should be pretty simple, my only concern would be that using |
src/style-spec/reference/v8.json
Outdated
@@ -861,6 +861,9 @@ | |||
}, | |||
"line": { | |||
"doc": "The label is placed along the line of the geometry. Can only be used on `LineString` and `Polygon` geometries." | |||
}, | |||
"line-center": { | |||
"doc": "The label is placed at the center of the line of the geometry. Can only be used on `LineString` and `Polygon` geometries. Note that a single feature in a vector tile may contain multiple line geometries. The center is calculated based on the entirety of the geometry included in the tile buffers, and the label may extend past the edge of the tile." |
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.
@samanpwbb you may want to review this description, since Studio will display this text for the property value tooltip.
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.
This description is a little on the long side. Is the last sentence necessary?
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.
this looks good to me! it might be a good idea to add some unit tests for this but the render tests are looking good so I'm not toooo concerned
also I think we'll need to add some ignores to gl-native for this one 😅
glyphSize: number, | ||
boxScale: number): number { | ||
return shapedText ? | ||
3 / 5 * glyphSize * boxScale : |
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.
a comment explaining the magic 3/5 number might be a good idea 🔢
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.
@ansis or @nickidlugash do you know the origin of this magic number? I guess I just assumed it was something "experimentally reasonable".
Good idea on unit tests for |
@@ -861,6 +861,9 @@ | |||
}, | |||
"line": { | |||
"doc": "The label is placed along the line of the geometry. Can only be used on `LineString` and `Polygon` geometries." | |||
}, | |||
"line-center": { |
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.
Can we add more specific compatibility info? See
mapbox-gl-js/src/style-spec/reference/v8.json
Lines 1035 to 1040 in a8cb4c4
"`auto` value": { | |
"js": "0.25.0", | |
"android": "4.2.0", | |
"ios": "3.4.0", | |
"macos": "0.3.0" | |
}, |
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.
src/style-spec/reference/v8.json
Outdated
@@ -861,6 +861,9 @@ | |||
}, | |||
"line": { | |||
"doc": "The label is placed along the line of the geometry. Can only be used on `LineString` and `Polygon` geometries." | |||
}, | |||
"line-center": { | |||
"doc": "The label is placed at the center of the line of the geometry. Can only be used on `LineString` and `Polygon` geometries. Note that a single feature in a vector tile may contain multiple line geometries. The center is calculated based on the entirety of the geometry included in the tile buffers, and the label may extend past the edge of the tile." |
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.
This description is a little on the long side. Is the last sentence necessary?
With this placement option, we will attempt to place a single label in the center of each "line" geometry of a feature. If the label doesn't fit or the 'text-max-angle' check fails, we don't place anything. Labels using 'line-center' are allowed to extend past the edge of the tile they're centered in, following line geometry included in the tile's buffers.
a8cb4c4
to
251b007
Compare
hey @ChrisLoer did you need to do |
Oops, yeah, thanks for catching that @andrewharvey! #6943 |
This is a potential implementation of #5061. Currently, it does the simplest thing possible, which is to just place labels in the middle of the line, ignoring
text-max-angle
limitations. I think we should actually implement some form oftext-max-angle
support (either hiding labels that don't pass or moving them away from center until they do pass).I added a simple test case using one of our Berlin test tiles:
I think it highlights one of the potential problems with using "line-center" on data in existing sources: the data is kind of organized by features (I have feature IDs mapped to colors in this example), but those features can be broken down into individual lines in ways that might not make much sense to a style designer. And of course the "center" they have in mind could easily be the center of multiple joined features.
I know @ajashton and @nickidlugash had in mind using this specifically on source layers designed for center labeling (e.g. a center line for a body of water)... to get a better feel for that use case, it would be helpful for us to design an actual vector tile taking advantage of the large buffers to do center-placing across tile boundaries. We can't just use GeoJSON like we do in most tests because that'll automatically get chopped up at tile boundaries (huh, I guess that could be an issue for someone trying to add cross-tile center-placed labels at runtime, too).
/cc @ansis