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

line labels with 0 "collision circles" still get placed #6919

Closed
ChrisLoer opened this issue Jul 5, 2018 · 2 comments
Closed

line labels with 0 "collision circles" still get placed #6919

ChrisLoer opened this issue Jul 5, 2018 · 2 comments
Assignees

Comments

@ChrisLoer
Copy link
Contributor

Discovered in the native port: the symbol-placement/line-center-buffer test case actually doesn't generate any collision circles because the logic in CollisionFeature aborts for line features that cross tile boundaries. The test case didn't notice on gl-js because there's nothing to collide against and gl-js treats "no collision circles" as "ok to place", while gl-native treats that as "not placeable".

/cc @nickidlugash

@ChrisLoer ChrisLoer self-assigned this Jul 5, 2018
@ChrisLoer
Copy link
Contributor Author

Err... it's not the tile boundary itself that's the problem, it's just that the label in the test case ends up close enough to the end of the line that CollisionFeature thinks there's not room, even though it can still technically be rendered. Ideally we should fix CollisionFeature (I have in mind #5474), but until then probably gl-js should follow the same behavior as gl-native -- if we can't generate a single collision circle for a line, we shouldn't try to draw the item.

There is a separate but closely related problem with symbol-placement/line-center, which is that the label "." doesn't generate any collision circles because the width of "." is less than half of the collision circle size:

const nBoxes = Math.floor(labelLength / step);

There I think the right behavior is to always have at least one collision circle as long as the label length is non-zero.

@ChrisLoer ChrisLoer changed the title symbol-placement: line-center doesn't generate collision circles outside tile boundary line labels with 0 "collision circles" still get placed Jul 6, 2018
@ChrisLoer
Copy link
Contributor Author

@ansis we actually introduced this relatively recently with #6164 -- we were aiming to make it so that 0-length text wouldn't prevent an associated icon from being placed... but ended up treating "no collision boxes because they don't fit on the line" the same as "no collision boxes because the string is empty".

We could keep the fix for issue #6160 by following gl-native's approach. Instead of doing it based on whether the bucket has text/icon data:

const iconWithoutText = !bucket.hasTextData() || layout.get('text-optional');
const textWithoutIcon = !bucket.hasIconData() || layout.get('icon-optional');

We could do it based on whether the symbolInstance has glyphs/quads:

https://github.com/mapbox/mapbox-gl-native/blob/ec472cb45ace58b2f159d42f439f51199ca5e34b/src/mbgl/text/placement.cpp#L154

ChrisLoer added a commit that referenced this issue Jul 6, 2018
- Don't place features that _have_ text but don't have collision boxes
- Create a single collision box even when the length of a label is less than half the box size

Test updates:
- Add debug collision circles to line-center and line-center-buffer to verify collision behavior
- Add regression test that exercises case where a line label is almost exactly the same length as its line
ChrisLoer added a commit that referenced this issue Jul 6, 2018
- Don't place features that _have_ text but don't have collision boxes
- Create a single collision box even when the length of a label is less than half the box size

Test updates:
- Add debug collision circles to line-center to verify collision behavior
- Add regression test that exercises case where a line label is almost exactly the same length as its line
ChrisLoer added a commit that referenced this issue Jul 11, 2018
- Don't place features that _have_ text but don't have collision boxes
- Create a single collision box even when the length of a label is less than half the box size

Test updates:
- Add debug collision circles to line-center to verify collision behavior
- Add regression test that exercises case where a line label is almost exactly the same length as its line
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this issue Sep 11, 2018
…tive

- Don't place features that _have_ text but don't have collision boxes
- Create a single collision box even when the length of a label is less than half the box size

Test updates:
- Add debug collision circles to line-center to verify collision behavior
- Add regression test that exercises case where a line label is almost exactly the same length as its line
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

No branches or pull requests

1 participant