From e79162fa79f25d62aaf46e992fac6dc2cc7714db Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Fri, 25 Aug 2017 10:42:14 -0700 Subject: [PATCH 1/2] [core] Add optional type template parameter to util::unit, so that it doesn't always pick up the default type of 'double' from util::mag. --- src/mbgl/util/math.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mbgl/util/math.hpp b/src/mbgl/util/math.hpp index fcde01c1a34..c18ce0c2549 100644 --- a/src/mbgl/util/math.hpp +++ b/src/mbgl/util/math.hpp @@ -77,9 +77,9 @@ T mag(const S& a) { return std::sqrt(a.x * a.x + a.y * a.y); } -template +template S unit(const S& a) { - auto magnitude = mag(a); + auto magnitude = mag(a); if (magnitude == 0) { return a; } From f71ccb61ebaac6d0251d57f9d81fbdb7c2852fbc Mon Sep 17 00:00:00 2001 From: Chris Loer Date: Fri, 25 Aug 2017 10:45:24 -0700 Subject: [PATCH 2/2] [core] Fix issue #9843 (overzoomed line label rendering glitch). Ports fix for GL JS issue #5112. Line label projection can't be based on tile geometry that's behind the plane of the camera. The relevant tests are still ignored because the overzoomed collision behavior is different between native and JS. --- platform/node/test/ignores.json | 4 +- src/mbgl/layout/symbol_projection.cpp | 72 ++++++++++++++++++++------- 2 files changed, 55 insertions(+), 21 deletions(-) diff --git a/platform/node/test/ignores.json b/platform/node/test/ignores.json index 9ffc9d30b45..597b9a5d1a9 100644 --- a/platform/node/test/ignores.json +++ b/platform/node/test/ignores.json @@ -63,8 +63,8 @@ "render-tests/text-pitch-alignment/auto-text-rotation-alignment-viewport": "https://github.com/mapbox/mapbox-gl-native/issues/9732", "render-tests/text-pitch-alignment/map-text-rotation-alignment-map": "https://github.com/mapbox/mapbox-gl-native/issues/9732", "render-tests/text-pitch-alignment/map-text-rotation-alignment-viewport": "https://github.com/mapbox/mapbox-gl-native/issues/9732", - "render-tests/text-pitch-alignment/viewport-overzoomed": "https://github.com/mapbox/mapbox-gl-native/issues/9843", - "render-tests/text-pitch-alignment/viewport-overzoomed-single-glyph": "https://github.com/mapbox/mapbox-gl-native/issues/9843", + "render-tests/text-pitch-alignment/viewport-overzoomed": "https://github.com/mapbox/mapbox-gl-js/issues/5095", + "render-tests/text-pitch-alignment/viewport-overzoomed-single-glyph": "https://github.com/mapbox/mapbox-gl-js/issues/5095", "render-tests/text-pitch-alignment/viewport-text-rotation-alignment-map": "https://github.com/mapbox/mapbox-gl-native/issues/9732", "render-tests/text-pitch-alignment/viewport-text-rotation-alignment-viewport": "https://github.com/mapbox/mapbox-gl-native/issues/9732", "render-tests/text-pitch-scaling/line-half": "https://github.com/mapbox/mapbox-gl-native/issues/9732", diff --git a/src/mbgl/layout/symbol_projection.cpp b/src/mbgl/layout/symbol_projection.cpp index 8ccd6a04101..279d251f8f4 100644 --- a/src/mbgl/layout/symbol_projection.cpp +++ b/src/mbgl/layout/symbol_projection.cpp @@ -94,10 +94,12 @@ namespace mbgl { } - Point project(const Point& point, const mat4& matrix) { + typedef std::pair,float> PointAndCameraDistance; + + PointAndCameraDistance project(const Point& point, const mat4& matrix) { vec4 pos = {{ point.x, point.y, 0, 1 }}; matrix::transformMat4(pos, pos, matrix); - return { static_cast(pos[0] / pos[3]), static_cast(pos[1] / pos[3]) }; + return {{ static_cast(pos[0] / pos[3]), static_cast(pos[1] / pos[3]) }, pos[3] }; } float evaluateSizeForFeature(const ZoomEvaluatedSize& zoomEvaluatedSize, const PlacedSymbol& placedSymbol) { @@ -150,9 +152,20 @@ namespace mbgl { NotEnoughRoom, NeedsFlipping }; + + Point projectTruncatedLineSegment(const Point& previousTilePoint, const Point& currentTilePoint, const Point& previousProjectedPoint, const float minimumLength, const mat4& projectionMatrix) { + // We are assuming "previousTilePoint" won't project to a point within one unit of the camera plane + // If it did, that would mean our label extended all the way out from within the viewport to a (very distant) + // point near the plane of the camera. We wouldn't be able to render the label anyway once it crossed the + // plane of the camera. + const Point projectedUnitVertex = project(previousTilePoint + util::unit(previousTilePoint - currentTilePoint), projectionMatrix).first; + const Point projectedUnitSegment = previousProjectedPoint - projectedUnitVertex; + + return previousProjectedPoint + (projectedUnitSegment * (minimumLength / util::mag(projectedUnitSegment))); + } optional placeGlyphAlongLine(const float offsetX, const float lineOffsetX, const float lineOffsetY, const bool flip, - Point anchorPoint, const uint16_t anchorSegment, const GeometryCoordinates& line, const mat4& labelPlaneMatrix) { + const Point& projectedAnchorPoint, const Point& tileAnchorPoint, const uint16_t anchorSegment, const GeometryCoordinates& line, const mat4& labelPlaneMatrix) { const float combinedOffsetX = flip ? offsetX - lineOffsetX : @@ -172,8 +185,8 @@ namespace mbgl { int32_t currentIndex = dir > 0 ? anchorSegment : anchorSegment + 1; - Point current = anchorPoint; - Point prev = anchorPoint; + Point current = projectedAnchorPoint; + Point prev = projectedAnchorPoint; float distanceToPrev = 0.0; float currentSegmentDistance = 0.0; const float absOffsetX = std::abs(combinedOffsetX); @@ -185,7 +198,18 @@ namespace mbgl { if (currentIndex < 0 || currentIndex >= static_cast(line.size())) return {}; prev = current; - current = project(convertPoint(line.at(currentIndex)), labelPlaneMatrix); + PointAndCameraDistance projection = project(convertPoint(line.at(currentIndex)), labelPlaneMatrix); + if (projection.second > 0) { + current = projection.first; + } else { + // The vertex is behind the plane of the camera, so we can't project it + // Instead, we'll create a vertex along the line that's far enough to include the glyph + const Point previousTilePoint = distanceToPrev == 0 ? + tileAnchorPoint : + convertPoint(line.at(currentIndex - dir)); + const Point currentTilePoint = convertPoint(line.at(currentIndex)); + current = projectTruncatedLineSegment(previousTilePoint, currentTilePoint, prev, absOffsetX - distanceToPrev + 1, labelPlaneMatrix); + } distanceToPrev += currentSegmentDistance; currentSegmentDistance = util::dist(prev, current); @@ -211,29 +235,28 @@ namespace mbgl { const mat4& posMatrix, const mat4& labelPlaneMatrix, const mat4& glCoordMatrix, - gl::VertexVector& dynamicVertexArray) { + gl::VertexVector& dynamicVertexArray, + const Point& projectedAnchorPoint) { const float fontScale = fontSize / 24.0; const float lineOffsetX = symbol.lineOffset[0] * fontSize; const float lineOffsetY = symbol.lineOffset[1] * fontSize; - const Point anchorPoint = project(symbol.anchorPoint, labelPlaneMatrix); - std::vector placedGlyphs; if (symbol.glyphOffsets.size() > 1) { const float firstGlyphOffset = symbol.glyphOffsets.front(); const float lastGlyphOffset = symbol.glyphOffsets.back(); - optional firstPlacedGlyph = placeGlyphAlongLine(fontScale * firstGlyphOffset, lineOffsetX, lineOffsetY, flip, anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); + optional firstPlacedGlyph = placeGlyphAlongLine(fontScale * firstGlyphOffset, lineOffsetX, lineOffsetY, flip, projectedAnchorPoint, symbol.anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); if (!firstPlacedGlyph) return PlacementResult::NotEnoughRoom; - optional lastPlacedGlyph = placeGlyphAlongLine(fontScale * lastGlyphOffset, lineOffsetX, lineOffsetY, flip, anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); + optional lastPlacedGlyph = placeGlyphAlongLine(fontScale * lastGlyphOffset, lineOffsetX, lineOffsetY, flip, projectedAnchorPoint, symbol.anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); if (!lastPlacedGlyph) return PlacementResult::NotEnoughRoom; - const Point firstPoint = project(firstPlacedGlyph->point, glCoordMatrix); - const Point lastPoint = project(lastPlacedGlyph->point, glCoordMatrix); + const Point firstPoint = project(firstPlacedGlyph->point, glCoordMatrix).first; + const Point lastPoint = project(lastPlacedGlyph->point, glCoordMatrix).first; if (keepUpright && !flip && (symbol.useVerticalMode ? firstPoint.y < lastPoint.y : firstPoint.x > lastPoint.x)) { @@ -244,7 +267,7 @@ namespace mbgl { for (size_t glyphIndex = 1; glyphIndex < symbol.glyphOffsets.size() - 1; glyphIndex++) { const float glyphOffsetX = symbol.glyphOffsets[glyphIndex]; // Since first and last glyph fit on the line, we're sure that the rest of the glyphs can be placed - auto placedGlyph = placeGlyphAlongLine(glyphOffsetX * fontScale, lineOffsetX, lineOffsetY, flip, anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); + auto placedGlyph = placeGlyphAlongLine(glyphOffsetX * fontScale, lineOffsetX, lineOffsetY, flip, projectedAnchorPoint, symbol.anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); placedGlyphs.push_back(*placedGlyph); } placedGlyphs.push_back(*lastPlacedGlyph); @@ -252,15 +275,23 @@ namespace mbgl { // Only a single glyph to place // So, determine whether to flip based on projected angle of the line segment it's on if (keepUpright && !flip) { - const Point a = project(convertPoint(symbol.line.at(symbol.segment)), posMatrix); - const Point b = project(convertPoint(symbol.line.at(symbol.segment + 1)), posMatrix); + const Point a = project(symbol.anchorPoint, posMatrix).first; + const Point tileSegmentEnd = convertPoint(symbol.line.at(symbol.segment + 1)); + const PointAndCameraDistance projectedVertex = project(tileSegmentEnd, posMatrix); + // We know the anchor will be in the viewport, but the end of the line segment may be + // behind the plane of the camera, in which case we can use a point at any arbitrary (closer) + // point on the segment. + const Point b = (projectedVertex.second > 0) ? + projectedVertex.first : + projectTruncatedLineSegment(symbol.anchorPoint,tileSegmentEnd, a, 1, posMatrix); + if (symbol.useVerticalMode ? b.y > a.y : b.x < a.x) { return PlacementResult::NeedsFlipping; } } assert(symbol.glyphOffsets.size() == 1); // We are relying on SymbolInstance.hasText filtering out symbols without any glyphs at all const float glyphOffsetX = symbol.glyphOffsets.front(); - optional singleGlyph = placeGlyphAlongLine(fontScale * glyphOffsetX, lineOffsetX, lineOffsetY, flip, anchorPoint, symbol.segment, + optional singleGlyph = placeGlyphAlongLine(fontScale * glyphOffsetX, lineOffsetX, lineOffsetY, flip, projectedAnchorPoint, symbol.anchorPoint, symbol.segment, symbol.line, labelPlaneMatrix); if (!singleGlyph) return PlacementResult::NotEnoughRoom; @@ -275,6 +306,7 @@ namespace mbgl { return PlacementResult::OK; } + void reprojectLineLabels(gl::VertexVector& dynamicVertexArray, const std::vector& placedSymbols, const mat4& posMatrix, const style::SymbolPropertyValues& values, const RenderTile& tile, const SymbolSizeBinder& sizeBinder, const TransformState& state, const FrameHistory& frameHistory) { @@ -311,12 +343,14 @@ namespace mbgl { const float pitchScaledFontSize = values.pitchAlignment == style::AlignmentType::Map ? fontSize * perspectiveRatio : fontSize / perspectiveRatio; + + const Point anchorPoint = project(placedSymbol.anchorPoint, labelPlaneMatrix).first; - PlacementResult placeUnflipped = placeGlyphsAlongLine(placedSymbol, pitchScaledFontSize, false /*unflipped*/, values.keepUpright, posMatrix, labelPlaneMatrix, glCoordMatrix, dynamicVertexArray); + PlacementResult placeUnflipped = placeGlyphsAlongLine(placedSymbol, pitchScaledFontSize, false /*unflipped*/, values.keepUpright, posMatrix, labelPlaneMatrix, glCoordMatrix, dynamicVertexArray, anchorPoint); if (placeUnflipped == PlacementResult::NotEnoughRoom || (placeUnflipped == PlacementResult::NeedsFlipping && - placeGlyphsAlongLine(placedSymbol, pitchScaledFontSize, true /*flipped*/, values.keepUpright, posMatrix, labelPlaneMatrix, glCoordMatrix, dynamicVertexArray) == PlacementResult::NotEnoughRoom)) { + placeGlyphsAlongLine(placedSymbol, pitchScaledFontSize, true /*flipped*/, values.keepUpright, posMatrix, labelPlaneMatrix, glCoordMatrix, dynamicVertexArray, anchorPoint) == PlacementResult::NotEnoughRoom)) { hideGlyphs(placedSymbol.glyphOffsets.size(), dynamicVertexArray); } }