Fix rendering of cross-hatched areas

[lpechacek]

See issue #2101 for details.

[dl3sdo]

The fix looks fine to me. I just tested it and it provides the expected results.

[dg0yt]

Thanks to both of you for picking up this issue.

setTopLeft was really wrong because it "may change the size". I think it should have been moveTopLeft.
IMO includeRect does too much to the extent which is an initial, invalid QRectF at this point.
(There is also safeIncludeRect, just for cases where the rect maybe initially invalid. So many helpers which even aren't pure functions...)

[lpechacek]

Answering out of order...

IMO includeRect does too much to the extent which is an initial, invalid QRectF at this point. (There is also safeIncludeRect, just for cases where the rect maybe initially invalid. So many helpers which even aren't pure functions...)

Thanks, Kai, for the review. includeRect is plain wrong in my patch as it causes too large extents for the lines. includeRectSafe does the right job in the freshly constructed extent.

setTopLeft was really wrong because it "may change the size". I think it should have been moveTopLeft.

The use of moveTopLeft would be a solution. Anyway, we would have to determine the minimum on the x and y coordinates, including the line width, or blindly set the rectangle origin and then call includeRect:

	extent.moveTopLeft(first);
	rectInclude(extent, first - right);
	rectInclude(extent, first + right);
	rectInclude(extent, second - right);
	rectInclude(extent, second + right);

My approach would be completely different, however. We've got a straightforward calculation here - two line endpoints, determine the bounding rectangle. My choice would be a complete rewrite here for the sake of comprehension and efficiency. The only disadvantage I can see is that we are working with individual coordinates rather than with vectors which is error prone. I don't see a vector-based solution at the moment though.

diff --git a/src/core/renderables/renderable_implementation.cpp b/src/core/renderables/renderable_implementation.cpp
index 32f6f78d9201..ab849f4fc977 100644
--- a/src/core/renderables/renderable_implementation.cpp
+++ b/src/core/renderables/renderable_implementation.cpp
@@ -281,17 +281,15 @@ LineRenderable::LineRenderable(const LineSymbol* symbol, QPointF first, QPointF
  , cap_style(Qt::FlatCap)
  , join_style(Qt::MiterJoin)
 {
-	qreal half_line_width = (color_priority < 0) ? 0 : line_width/2;
-	
 	auto right = MapCoordF(second - first).perpRight();
 	right.normalize();
-	right *= half_line_width;
-	
-	extent.setTopLeft(first + right);
-	rectInclude(extent, first - right);
-	rectInclude(extent, second - right);
-	rectInclude(extent, second + right);
-	
+	right *= (color_priority < 0) ? 0 : line_width;
+	auto const origin = QPointF { std::min(first.x(), second.x()) - right.x()/2,
+	                              std::min(first.y(), second.y()) - right.y()/2 };
+	auto const size = QSizeF { std::abs(first.x() - second.x()) + right.x()/2,
+	                           std::abs(first.y() - second.y()) + right.y()/2 };
+
+	extent = QRectF { origin, size };
 	path.moveTo(first);
 	path.lineTo(second);
 }

[lpechacek]

My choice would be a complete rewrite here for the sake of comprehension and efficiency.

LibreMapper/mapper@c3f86a7