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

[core] TilePyramid has sorted render tiles #13739

Merged
merged 1 commit into from
Jan 16, 2019

Conversation

pozdnyakov
Copy link
Contributor

@pozdnyakov pozdnyakov commented Jan 15, 2019

Thus we obviate unneeded extra sorting of render tiles at each render layer.

@pozdnyakov pozdnyakov added the Core The cross-platform C++ core, aka mbgl label Jan 15, 2019
std::sort(sortedTiles.begin(), sortedTiles.end(), [](const RenderTile& a, const RenderTile& b) {
return std::tie(a.id.canonical.z, a.id.canonical.y, a.id.wrap, a.id.canonical.x) <
std::tie(b.id.canonical.z, b.id.canonical.y, b.id.wrap, b.id.canonical.x);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sort function sorts differently from the standard UnwrappedTileID's operator< function: It sorts by (z, y, wrap, x), while UnwrappedTileID::operator< sorts by (wrap, z, x, y). I'm not sure why we sort this way here, but might be worth investigating how a different sort order affects feature querying.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks for pointing that out!

@@ -43,7 +43,7 @@ RenderLayer::RenderTiles RenderLayer::filterRenderTiles(RenderTiles tiles) const
}

void RenderLayer::sortRenderTiles(const TransformState&) {
std::sort(renderTiles.begin(), renderTiles.end(), [](const auto& a, const auto& b) { return a.get().id < b.get().id; });
// no-op
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function is a no-op, we can remove it entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenderSymbolLayer still overrides it, however this is something I'd like to change in the following PR.

@@ -53,7 +53,7 @@ void TilePyramid::finishRender(PaintParameters& parameters) {
}

std::vector<std::reference_wrapper<RenderTile>> TilePyramid::getRenderTiles() {
return { renderTiles.begin(), renderTiles.end() };
return sortedTiles;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of maintaining two vectors in parallel, can we just sort renderTiles, and create the reference-wrapped copy of the vector in this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenderTile does not have a default constructor, so std::swap cannot handle it, therefore standard algorithms look not applicable there :-/

@pozdnyakov pozdnyakov force-pushed the mikhail_render_layer_sort branch from 1ae224e to 1c18585 Compare January 16, 2019 12:39
@pozdnyakov
Copy link
Contributor Author

@kkaefer Thanks for your comments! Could you take another look?

Copy link
Member

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please fix the spelling error in the commit message?

Thus we obviate unneeded extra sorting of render tiles at each render layer.
@pozdnyakov pozdnyakov force-pushed the mikhail_render_layer_sort branch from 1c18585 to f564baa Compare January 16, 2019 13:21
@pozdnyakov pozdnyakov changed the title [core] TylePyramid has sorted render tiles [core] TilePyramid has sorted render tiles Jan 16, 2019
@pozdnyakov pozdnyakov merged commit ac3d598 into master Jan 16, 2019
@pozdnyakov pozdnyakov deleted the mikhail_render_layer_sort branch January 16, 2019 14:16
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants