Clip raster tiles by kkaefer · Pull Request #9468 · mapbox/mapbox-gl-native

kkaefer · 2017-07-10T20:49:15Z

This changes our raster tile rendering to avoid rendering in the parts of a tile that have children for. It accomplishes this by generating vertex buffers that only have quads for the respective areas, rather than having one quad to fill the entire tile.

The first part of the PR refactors Vertex/Index/Segment vectors into their own IndexedPrimitive/Drawable classes. We can apply the same pattern for other buckets in a separate PR.

The algorithm works by recursively descending from the tile in question and generating a list of tiles that would cover the tile, but not any of its children.

Finally, we're computing vertex buffers with our new IndexedPrimitive class and store the vertex buffers in a repository object.

Remaining tasks:

Render test
Delete unused mask buffers

Fixes #9415, #7760, and #8678

jfirebaugh · 2017-07-12T22:50:08Z

Is the actual raster clipping change here dependent on the IndexedPrimitive/Drawables/SegmentInfo changes or can those be an independent PR?

kkaefer · 2017-07-13T09:17:36Z

It's not 100% dependent, but then we'd have to implement something that is essentially like that as well. We have the same code duplication for all of the buckets where we have complex code to make sure that all associated vertices for a number of primitives go into the same segment and. Are you saying we should merge IndexedPrimitive/Drawables/SegmentInfo before or after raster clipping?

jfirebaugh · 2017-07-13T16:20:59Z


-template <class Attributes>
-class Segment {
+class SegmentInfo {


Once you rebase on master to pick up #9433, I'd prefer to reverse the naming here: keep the Segment class as it is and move Segment::vertexArrays to a new class that composes it and Segment (name suggestion: DrawableSegment). Then give Drawable a DrawableSegmentVector rather than a SegmentVector. That will reduce the size of the diff here (no need to add .info. everywhere).

That's what I attempted initially as well, but I opted to do it the other way around to keep the diff size small. However, I'm happy to rename Segment to DrawableSegment. Another candidate might be SegmentBuffers for naming alignment with VertexBuffer and IndexBuffer.

jfirebaugh · 2017-07-13T16:38:25Z

    offsetof(VertexType, a5)
 };

+template <class A1>


Seems these are only needed for a test; can we move them there? Regular code shouldn't need to compare vertices. (Same with operator== for SegmentInfo.)

jfirebaugh · 2017-07-13T16:56:21Z

+template <class DrawMode, class LayoutVertex, class AttributeLayout>
+class IndexedPrimitives {
+public:
+    void add(std::initializer_list<LayoutVertex> v,


If we're to convert all the bucket types to IndexedPrimitives, this will be used in very hot code paths. How much overhead is there in creating initializer_lists, checking the segment capacity on every add, checking for valid indexes, etc.?

jfirebaugh · 2017-07-13T17:00:14Z

+
+private:
+    friend class Drawable<DrawMode, LayoutVertex, AttributeLayout>;
+    gl::VertexVector<LayoutVertex> vertices;


Note that the IndexedPrimitives abstraction won't work for the current implementation of FillBucket, which shares vertices between triangle and line primitives. It will work if we fix mapbox/mapbox-gl-js#2080 though. Are there any other scenarios where we would want to index the same set of vertices multiple ways? I can't think of any.

ansis · 2017-07-14T17:09:01Z

What is the advantage of this approach over using the depth buffer or stencil buffers (if we have any bits left there) to do clipping?

jfirebaugh · 2017-07-14T17:31:53Z

Are you saying we should merge IndexedPrimitive/Drawables/SegmentInfo before or after raster clipping?

I think we should either implement raster clipping without any major refactoring, or do the refactor to IndexedPrimitive/Drawable first, in a separate PR, and do it fully, and then implement raster clipping on top of that.

A partial refactor to IndexedPrimitive/Drawable combined with implementing raster clipping leaves the codebase in an inconsistent state, and makes reviewing the changes more difficult.

lbud · 2017-07-14T20:25:11Z

+namespace mbgl {
+
+// A TileMask is a set of TileIDs that describe what part of a tile should be rendered. I.e. it
+// denotes the part of a tile that is covered by other/better tiles. If the entire tile should be


nit: these first two sentences seem to contradict each other — first that these parts of a tile "should be rendered" and then that this mask denotes parts that should be not be rendered

kkaefer · 2017-07-20T09:07:01Z

What is the advantage of this approach over using the depth buffer or stencil buffers (if we have any bits left there) to do clipping?

The advantage of this approach is that we're shading fewer pixels. Also, raster tiles aren't using stencil clipping since they typically load smaller tiles, which makes our stencil clipping mask overflow. It also allows us to draw the texture directly without first having to draw into the depth or stencil buffer.

kkaefer

Removed the Drawables/Segment changes. Instead of having a TileMaskRepository, mask vertex buffers are now owned directly by the RasterBucket objects. When there is no masking happening (=default case when the entire tile is drawn), we're still sharing the global vertex buffer.

jfirebaugh · 2017-07-21T20:30:20Z

+// A TileMask is a set of TileIDs that describe what part of a tile should be rendered. It omits
+// those parts of the tile that are covered by other/better tiles. If the entire tile should be
+// rendered, it contains the { 0, 0, 0 } tile. If it's empty, no part of the tile will be rendered.
+// TileMasks are typically generated with algorithm::updateTileMasks().


The key to understanding how the TileMask is generated and used is to understand the relationship between:

The tile ID of the tile being rendered

The tile IDs of the renderable descendants of that tile

The tile IDs of elements of the resulting TileMask

Can you spell out that relationship here or elsewhere, with either an algebraic definition or example?

Added an example with a schematic drawing to the algorithm file.

jfirebaugh · 2017-07-21T21:03:19Z

    }
 }

+inline CanonicalTileID CanonicalTileID::operator-(const CanonicalTileID& rhs) const {


It seems like this definition of subtraction is specific to tile masks. Should it be a named function in tile_mask.hpp or update_tile_masks.hpp instead?

Inlined this function

kkaefer added Core The cross-platform C++ core, aka mbgl rendering labels Jul 10, 2017

kkaefer requested review from jfirebaugh, lbud and tmpsantos July 10, 2017 20:49

kkaefer force-pushed the clip-raster-tiles branch from b616b16 to 23cfe73 Compare July 11, 2017 12:59

jfirebaugh reviewed Jul 13, 2017

View reviewed changes

lbud reviewed Jul 14, 2017

View reviewed changes

kkaefer mentioned this pull request Jul 20, 2017

Port raster tile masking mapbox/mapbox-gl-js#5003

Closed

kkaefer force-pushed the clip-raster-tiles branch from 23cfe73 to 0ba701f Compare July 20, 2017 12:51

kkaefer commented Jul 20, 2017

View reviewed changes

kkaefer force-pushed the clip-raster-tiles branch from 0ba701f to 4df6257 Compare July 20, 2017 12:55

kkaefer mentioned this pull request Jul 20, 2017

Add tile masking test mapbox/mapbox-gl-js#5004

Merged

kkaefer force-pushed the clip-raster-tiles branch from 4df6257 to 1eff949 Compare July 20, 2017 13:31

jfirebaugh mentioned this pull request Jul 20, 2017

Offline raster tiles does not use correct zoom level #9563

Closed

jfirebaugh approved these changes Jul 21, 2017

View reviewed changes

kkaefer added 2 commits July 24, 2017 11:44

[core] add algorithm for computing masks for raster tiles

d94c7d9

[core] generate masks for raster tiles to avoid painting over children

244fa47

kkaefer force-pushed the clip-raster-tiles branch from 1eff949 to 244fa47 Compare July 24, 2017 09:45

kkaefer merged commit c0efad5 into master Jul 24, 2017

kkaefer deleted the clip-raster-tiles branch July 24, 2017 10:10

This was referenced Jul 24, 2017

Flickering with translucent raster style layer #7760

Closed

Custom tileset rendering error #8678

Closed

mollymerp mentioned this pull request Aug 5, 2017

Clip raster tiles mapbox/mapbox-gl-js#5105

Merged

5 tasks

jfirebaugh mentioned this pull request Aug 14, 2017

Yet another Bucket refactor!? mapbox/mapbox-gl-js#5137

Merged

Conversation

kkaefer commented Jul 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfirebaugh commented Jul 12, 2017

Uh oh!

kkaefer commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansis commented Jul 14, 2017

Uh oh!

jfirebaugh commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkaefer commented Jul 20, 2017

Uh oh!

kkaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kkaefer commented Jul 10, 2017 •

edited

Loading