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

Support geographically-sized text symbols #9208

Closed
wants to merge 1 commit into from

Conversation

mike-unearth
Copy link

@mike-unearth mike-unearth commented Jan 20, 2020

Fixes #7163
Fixes #7563

This PR adds support for text layers that scale with the map by removing the restriction on text-size for source and composite expressions.

The main problem is that text-size has been given the same precision as icon-size, even though the former is in units of pixels and the latter is in units of relative size. Integer precision is fine for pixel units, but terrible for icon-size units. To support icon-size, these values are scaled by SIZE_PACK_FACTOR (previously 256 but recently reduced to 128), packed in to a UInt16, and then divided by SIZE_PACK_FACTOR in the shader. This change skips that upscaling and downscaling when we're dealing with text-size instead of icon-size, significantly increasing the upper range of text-size without reducing the precision of icon-size.

Here are some JSFiddles to demonstrate:

Using v1.6.1: Note that no text is rendered and a console warning says Value for "text-size" is >= 255. Reduce your "text-size".

Using this fork: Note that you as you zoom, the text scales with the map.

  • 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/studio and/or @mapbox/map-design-team if this PR includes style spec 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>Adds support for geographically-sized text</changelog>

@asheemmamoowala
Copy link
Contributor

Hello @mike-unearth! Thank you for contributing to this project - and for the jsfiddle showing the new behavior!

In the demo, starting at zoom 17, you can see that text starts to be displayed less precisely. This, I believe, is hitting fundamental limitations of the SDF approach for rendering symbols in this project.

Past zoom 20, the text disappears entirely. I'm not certain it is related to this change, but it is not an issue on master so there's probably an investigation needed here.

Zoom 17
image

Zoom 18
image

cc @ansis @alexshalamov @arindam1993

@ryanhamley ryanhamley modified the milestones: release-c, release-d Mar 5, 2020
@mike-unearth
Copy link
Author

Hi @asheemmamoowala - thanks for the review! Sorry for the delayed response, I've been out of the country. To your points:

  • SDF gets ugly at large sizes: completely agree that this is an issue with text in Mapbox, but I'm not sure that's related to the scope of this PR. I know that my customers are willing to accept this, but they are not willing to use our app without geo-scaled text.
  • Text disappears at high zoom: this is because the default setting for text-allow-overlap is false. Adding 'text-allow-overlap': true to the layout style fixes it.

@karimnaaji karimnaaji modified the milestones: release-d, release-e Apr 6, 2020
@mike-unearth mike-unearth force-pushed the text-size branch 3 times, most recently from 5c0d3fb to 8c2a30b Compare April 21, 2020 00:07
Update 4/20/20: adding fix to support non-text SDF icons
@mike-unearth
Copy link
Author

^ Don't mind the force-pushes above, I was working to get all tests passing. Fixed issues with text along a line and with non-text SDF icons.

@sgolbabaei sgolbabaei removed this from the release-e milestone May 20, 2020
@arindam1993 arindam1993 changed the base branch from master to main June 18, 2020 18:13
@asheemmamoowala
Copy link
Contributor

@mike-unearth Thanks for bearing with the slow turn around on this. This feature would work better after the introduction of 2+ channel SDFs, as described in mapbox/mapbox-gl-native#137. I've opened #9857 in the mean time to track this feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants