[core] Transform{,State} cleanups #6790

brunoabinader · 2016-10-21T19:00:17Z

Reuse TransformState::{zoomScale,scaleZoom,getZoomFraction} and util::log2 whenever possible.

mention-bot · 2016-10-21T19:00:18Z

@brunoabinader, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @kkaefer and @ansis to be potential reviewers.

jfirebaugh · 2016-10-21T20:53:00Z

src/mbgl/renderer/painter_symbol.cpp

@@ -256,8 +256,7 @@ void Painter::renderSymbol(PaintParameters& parameters,
        auto& collisionBoxShader = shaders->collisionBox;
        context.program = collisionBoxShader.getID();
        collisionBoxShader.u_matrix = tile.matrix;
-        // TODO: This was the overscaled z instead of the canonical z.
-        collisionBoxShader.u_scale = std::pow(2, state.getZoom() - tile.id.canonical.z);
+        collisionBoxShader.u_scale = state.zoomScale(state.getZoomFraction());


Is this correct in the case of overzoomed tiles?

Good question - I'm not sure. I'll revert this particular change.

jfirebaugh · 2016-10-21T22:33:41Z

src/mbgl/tile/geometry_tile.cpp

@@ -139,7 +139,7 @@ void GeometryTile::queryRenderedFeatures(
                        queryGeometry,
                        transformState.getAngle(),
                        util::tileSize * id.overscaleFactor(),
-                        std::pow(2, transformState.getZoom() - id.overscaledZ),
+                        transformState.zoomScale(transformState.getZoomFraction()),


(Same question here BTW.)

Yes, I guess I didn't account for overscaled zoom on my tests. We can revisit it again with proper unit tests to verify later.

brunoabinader · 2016-10-30T09:02:46Z

Took liberty to include a few other refactors and bug fixes related. Rebased after #6468 (/cc @jfirebaugh).

kkaefer · 2016-10-31T10:43:26Z

src/mbgl/layout/symbol_layout.cpp

-
-    const float placementZoom = ::fmax(std::log(scale) / std::log(2) + zoom, 0);
+    static const uint16_t vertexLength = 4;
+    const float placementZoom = util::max(std::log2(scale) + zoom, 0.0f);


Not using util::log2

Oops - forgot to update these, thanks!

kkaefer · 2016-10-31T10:44:11Z

src/mbgl/layout/symbol_layout.cpp

        const auto &anchorPoint = symbol.anchorPoint;

        // drop upside down versions of glyphs
        const float a = std::fmod(symbol.anchorAngle + placementAngle + M_PI, M_PI * 2);
        if (keepUpright && placement == style::SymbolPlacementType::Line &&
-            (a <= M_PI / 2 || a > M_PI * 3 / 2)) {
+            (a <= M_PI_2 || a > M_PI_2 * 3)) {


Not sure this makes sense for readability. The compiler almost certainly optimizes this anyway.

Fair enough 👍 From reading http://stackoverflow.com/posts/1610346/revisions it seems that using M_PI_2 actually decreases a bit of precision in favor of pre computation.

kkaefer · 2016-10-31T10:45:54Z

src/mbgl/math/log2.cpp

-#endif
-}
+template <> float log2(float x);
+template <> double log2(double x);


Why are these needed? The implementation is all in the header anyway.

This accounts for explicit instantiation, which from my understanding instantiates these template specializations to be reused by each compilation unit without duplications.

Yeah, but they are only needed when you want to force certain instances to exist even if they're not used, or when the template definition is in an implementation file.

kkaefer · 2016-10-31T10:46:41Z

src/mbgl/layout/symbol_layout.cpp

            buffer.segments.emplace_back(buffer.vertices.size(), buffer.triangles.size());
        }

        // We're generating triangle fans, so we always start with the first
        // coordinate in this polygon.
        auto& segment = buffer.segments.back();
-        size_t index = segment.vertexLength;
+        uint16_t index = uint16_t(segment.vertexLength);


Maybe assert here that it's <= 65535?

kkaefer · 2016-10-31T10:49:51Z

src/mbgl/layout/symbol_layout.cpp

@@ -418,6 +418,7 @@ std::unique_ptr<SymbolBucket> SymbolLayout::place(CollisionTile& collisionTile)

 template <typename Buffer>
 void SymbolLayout::addSymbols(Buffer &buffer, const SymbolQuads &symbols, float scale, const bool keepUpright, const style::SymbolPlacementType placement, const float placementAngle) {
+    static const uint16_t vertexLength = 4;


Can be constexpr

Sure - btw this accounts for the longest type declaration in our codebase: static constexpr const uint16_t 😄

- Added util::{MIN,MAX}_ZOOM_F to avoid consecutive conversions from double to float - Move util::log2 to its own header (part of mbgl/math)

jfirebaugh · 2016-10-31T15:49:15Z

Can you please revert the GL_LINEAR change? Enabling GL_LINEAR during pan-only operations was the intended behavior -- it prevents icons from "wiggling" during the pan.

brunoabinader added the Core The cross-platform C++ core, aka mbgl label Oct 21, 2016

jfirebaugh reviewed Oct 21, 2016

View reviewed changes

brunoabinader force-pushed the transformstate-helpers branch 3 times, most recently from 6f3a220 to acbb073 Compare October 24, 2016 06:33

brunoabinader mentioned this pull request Oct 25, 2016

Precompute more state in TransformState #2129

Closed

brunoabinader changed the title ~~[core] Reuse TransformState helpers~~ [core] Transform{,State} cleanups Oct 29, 2016

brunoabinader force-pushed the transformstate-helpers branch 2 times, most recently from cbee3dc to 689c8b1 Compare October 30, 2016 09:01

brunoabinader added the ✓ ready for review label Oct 30, 2016

brunoabinader force-pushed the transformstate-helpers branch from 689c8b1 to ff9f2fc Compare October 30, 2016 09:18

kkaefer suggested changes Oct 31, 2016

View reviewed changes

brunoabinader added 2 commits October 31, 2016 15:11

[core] Use overscaledZ for scale in collision box shader

97483c8

[core] Don't use GL_LINEAR if panning w/o rotation nor pitch

815c408

brunoabinader force-pushed the transformstate-helpers branch from af691bf to f1f2840 Compare October 31, 2016 13:11

brunoabinader added 3 commits October 31, 2016 16:12

[core] Moved util::log2 to its own header

62fe443

- Added util::{MIN,MAX}_ZOOM_F to avoid consecutive conversions from double to float - Move util::log2 to its own header (part of mbgl/math)

[core] Use numeric_limits<>::max() for checking element groups

697fa9f

[core] Cleanup TransformState::getIntegerZoom

6a756e0

brunoabinader force-pushed the transformstate-helpers branch from f1f2840 to 6a756e0 Compare October 31, 2016 14:14

kkaefer approved these changes Oct 31, 2016

View reviewed changes

brunoabinader merged commit 7b3e24d into master Oct 31, 2016

brunoabinader deleted the transformstate-helpers branch October 31, 2016 14:53

mikemorris mentioned this pull request Oct 31, 2016

Investigate pixelated symbols in labels #6863

Closed

brunoabinader mentioned this pull request Oct 31, 2016

[core] Use GL_LINEAR on sprite texture if panning #6866

Merged

1ec5 removed the ✓ ready for review label Nov 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Transform{,State} cleanups #6790

[core] Transform{,State} cleanups #6790

brunoabinader commented Oct 21, 2016

mention-bot commented Oct 21, 2016

jfirebaugh Oct 21, 2016

brunoabinader Oct 21, 2016

jfirebaugh Oct 21, 2016

brunoabinader Oct 21, 2016

brunoabinader commented Oct 30, 2016

kkaefer Oct 31, 2016

brunoabinader Oct 31, 2016

kkaefer Oct 31, 2016

brunoabinader Oct 31, 2016

kkaefer Oct 31, 2016

brunoabinader Oct 31, 2016

kkaefer Oct 31, 2016

kkaefer Oct 31, 2016

kkaefer Oct 31, 2016

brunoabinader Oct 31, 2016

jfirebaugh commented Oct 31, 2016

[core] Transform{,State} cleanups #6790

[core] Transform{,State} cleanups #6790

Conversation

brunoabinader commented Oct 21, 2016

mention-bot commented Oct 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brunoabinader commented Oct 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh commented Oct 31, 2016