Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Label legibility improvements: pitch-scaling and pitched line-following #9009

Merged
merged 15 commits into from
Jul 11, 2017

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented May 15, 2017

Fix #8967 by porting gl-js text pitch scaling behavior to gl-native.

  • Calculate shape of collision boxes based on tile distance from camera
  • Calculate collision tiles separately for wrapped copies of the same data tile
  • Update to new symbol shaders
  • Calculate label anchors and pass to updated shaders
  • Pass 'max-camera-distance' uniform to updated shaders
  • Throttle re-placement during pan operations (currently: change in tile distance always triggers placement, which combined with Main thread does not coalesce queued onPlacement messages #8435 can make for label dancing)
  • Modify CollisionFeature to support greater range of scales implied by pitch scaling
  • Selectively bind fill_color and halo_color vertex attributes to stay under 8 vertex attribute limit
  • Update debug collision box code
  • Re-enable tests

@ChrisLoer ChrisLoer added GL JS parity For feature parity with Mapbox GL JS text rendering ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels May 15, 2017
@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from 786dae6 to 5ae86cc Compare May 18, 2017 04:53
@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch 2 times, most recently from ad5bc3c to 9e9609d Compare May 26, 2017 21:54
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label May 30, 2017
@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from 9e9609d to ddb6c65 Compare May 31, 2017 23:22
@mollymerp
Copy link
Contributor

hello hi just tagging in so I know when to merge #9153

@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from ddb6c65 to 950f781 Compare June 1, 2017 20:22
@ChrisLoer
Copy link
Contributor Author

@asheemmamoowala Will the de-duplication of wrapped tiles in TilePyramid here play nice with your ImageSource changes?

@jfirebaugh Could you take a look at how I did selective binding fill_color/halo_color? I don't have it compiling yet on gcc, but I'm very uncertain about the approach I'm using. I think ideally maybe you could have the same vertex buffers but two sets of paint property binders you could pass in to the draw call in painter_symbol.cpp. I had a lot of trouble trying to tease that logic apart without dramatically changing how the binders work, and that seemed like overkill -- unless "selective binding" is a generalizable idea we'd want to use elsewhere? Still, my current implementation has some code smells: (1) setting a global flag in Context to enable the "selective binding", and (2) making the ordering of IconPaintProperties/TextPaintProperties magical (you need to put the "alternate" property at the end of the list in order to generate the right attribute locations.

@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch 2 times, most recently from d0deff0 to b34704d Compare June 2, 2017 00:11
@asheemmamoowala
Copy link
Contributor

@ChrisLoer - #8968 doesn't use OverscaledTileID, TilePyramid, or update_renderables so I think your changes to wrapped tiles shouldn't have any impact on ImageSource.

@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from b34704d to c3ecd5f Compare June 2, 2017 00:17
@ChrisLoer
Copy link
Contributor Author

Thanks @asheemmamoowala!

@ivovandongen This should be ready to test against the mythical 8-attribute device. Just a sanity check that labels have fill and halo colors should be enough, although I've also been testing the second pathway where fill/halo color are defined as a zoom function (had to make a custom style to test that).

@ChrisLoer ChrisLoer requested review from jfirebaugh and ansis June 2, 2017 00:25
@jfirebaugh
Copy link
Contributor

I'd hope to be able to do selective binding without modifying the low level OpenGL wrappers in mbgl/gl/attribute.cpp. The changes here are adding a pretty specialized case to what should be generic code.

I think the core requirement is to enable programs that bind a subset of attributes in a vertex array. Here's my attempt. The main blocker is the fact that Segment is templated on an Attributes type that must be the same as the Attributes type used by any Program that wants to draw those segments. The reason this is done is so that Segment can track the attribute bindings, and if they are different than the previous draw call, update the VAO state. The reason that this must be tracked on a per-Segment basis is that the VAO state effectively includes the offset from the beginning of the vertex buffer for that Segment.

Options:

  • The ARB_vertex_attrib_binding extension allows factoring this offset out. I believe this would allow Program to track the binding state instead of Segment. This would be ideal, but I doubt the extension is available everywhere.
  • Move the tracking of attribute bindings to Program anyway, and rebind when the vertex offset changes. This would nullify the performance advantage of VAOs in the case that a vertex buffer has more than 2^16 vertices -- rare, but it happens.
  • Allow Segment to track binding state for more than one Program type. Relax the requirement that its Attributes type match the Programs Attributes -- the Programs would merely need to be a subset. I think a previous iteration of VAO binding actually did this -- the moral equivalent of today's Segment was templated on the Program types that were going to use it, and there was a separate VAO for each one.

The second option sounds best to me right now.

@ivovandongen
Copy link
Contributor

@ChrisLoer The text/halo colors seem to work. But there is some general regressions in rendering on this device that also shows up in label rendering it seems:

text-colors
text-colors-less-ok

@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from c3ecd5f to a9b8410 Compare June 3, 2017 00:20
@ChrisLoer
Copy link
Contributor Author

@jfirebaugh OK, I took a stab at removing the Segment specialization on Attributes. I haven't figured out how to re-enable the VAO optimization yet. Is this in the direction you were thinking?

@ansis
Copy link
Contributor

ansis commented Jun 5, 2017

I think the core requirement is to enable programs that bind a subset of attributes in a vertex array.

If we implement text and halo rendering in a single draw call is this still necessary? mapbox/mapbox-gl-js#4709 (comment)

Are we going to hit the attribute limit for data-driven patterned/dashed lines? Will this require a different solution, like somehow packing multiple style properties in a single attribute?

@ChrisLoer
Copy link
Contributor Author

If we implement text and halo rendering in a single draw call is this still necessary?

If we do them in a single draw call, we have to pass both values down, so the selective binding wouldn't be possible any more. We'd have to get around the attribute limit some other way. Looking at your changes in mapbox/mapbox-gl-js#4781, it looks like there are 9 attributes but as much as 72 bits open?

  • Kind of 16 bits could be put in a_projected_pos[3] although the float/int thing is weird as is the dynamic updating.
  • a_data has 24 bits free.
  • a_size has 32 bits free, I think?

@ChrisLoer ChrisLoer force-pushed the cloer_text_pitch_scaling branch from a9b8410 to d297eae Compare June 5, 2017 20:45
@ansis
Copy link
Contributor

ansis commented Jun 6, 2017

All the pitch-scaling related changes look good to me

a_size has 32 bits free, I think?

Yep, but since we should move towards treating a_size as a regular data-driven property we might not want to pack into it.

ChrisLoer and others added 15 commits July 11, 2017 09:10
Necessary because collision boxes now change shape based on while tile they're part of.
…els.

Viewport-aligned curved labels start to look very strange in the distance. Until we have a better system for projecting them, just prevent them from showing.
This prevents TilePyramid from sharing wrapped copies of tiles.
This is necessary because two wrapped tiles no longer share the same CollisionTile.
It used to overwrite values in the middle of the calculation which would
cause problems when `out` and `a` were a reference to the same vector.
port mapbox/mapbox-gl-js#4781

This improves legibility of labels that follow lines in pitched views.
The previous approach used the limited information in the shader to
calculate put the glyph in approximatelyright place. The new approach
does this more accurately by doing it on the cpu where we have access to
the entire line geometry.
and rename BufferUsageType to BufferUsage
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold GL JS parity For feature parity with Mapbox GL JS text rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants