Fix Sequential Iterator assert on invalid rectangles
ClosedPublic

Authored by tymond on Mar 19 2019, 12:07 PM.

Details

Summary

Before Quick Brush engine (roundmarker) was crashing
because Sequential Iterator throwed out asserts
when the rectangle was not "empty" according to Qt
but still not valid (for example has intmin value on
width or height). This commit fixes that behaviour by
providing additional checks.
Commit includes also benchmarks for roundmarker.

BUG:404179

Test Plan
  • benchmarks included in the commit
  • painting with Quick Brush (i.e. b) Basic-1)

with size <1 px

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tymond created this revision.Mar 19 2019, 12:07 PM
Restricted Application added a reviewer: Krita. · View Herald TranscriptMar 19 2019, 12:07 PM
Restricted Application added a project: Krita. · View Herald Transcript
tymond requested review of this revision.Mar 19 2019, 12:07 PM
rempt added a subscriber: rempt.Mar 19 2019, 12:38 PM
rempt added inline comments.
libs/image/kis_sequential_iterator.h
49 ↗(On Diff #54306)

Why is this commented out?

tymond added inline comments.Mar 19 2019, 2:19 PM
libs/image/kis_sequential_iterator.h
49 ↗(On Diff #54306)

The true answer is "I forgot about it and it's not needed for Rectangle Tool + Quick Brush to work (and I tried to get rid of all checks I don't need)". Probably I should either delete it or uncomment it.

rempt added a comment.Mar 19 2019, 4:40 PM

Right. One other nitpick, otherwise it looks all very sane to me.

libs/image/kis_sequential_iterator.h
38 ↗(On Diff #54306)

Functions need to smart with small letter -- isNumberSane, isRectSane

I will check the patch today

dkazakov requested changes to this revision.Mar 20 2019, 1:47 PM

Hi, @tymond!

I have checked the code and I'm not sure if it okay to make such manual checks for the rects to workaround the problem. It would be better to fix the original problem and find out why such invalid rect is generated at all?

I've added this debugging info:

1diff --git a/libs/image/kis_marker_painter.cpp b/libs/image/kis_marker_painter.cpp
2index ff2f756..a38d234 100644
3--- a/libs/image/kis_marker_painter.cpp
4+++ b/libs/image/kis_marker_painter.cpp
5@@ -60,9 +60,14 @@ void KisMarkerPainter::fillHalfBrushDiff(const QPointF &p1, const QPointF &p2, c
6 KisAlgebra2D::RightHalfPlane plane2(p2, p3);
7 KisAlgebra2D::OuterCircle outer(center, radius);
8
9+ const QRectF originalRect = boundRect;
10+
11 boundRect = KisAlgebra2D::cutOffRect(boundRect, plane1);
12 boundRect = KisAlgebra2D::cutOffRect(boundRect, plane2);
13
14+ ENTER_FUNCTION() << boundRect.isEmpty() << ppVar(boundRect) << ppVar(originalRect);
15+ ENTER_FUNCTION() << ppVar(p1) << ppVar(p2) << ppVar(p3);
16+
17 KisSequentialIterator it(m_d->device, boundRect.toAlignedRect());
18 while (it.nextPixel()) {
19 QPoint pt(it.x(), it.y());
20@@ -126,6 +131,9 @@ void KisMarkerPainter::fillCirclesDiff(const QPointF &c1, qreal r1,
21 {
22 QVector<QPointF> n = KisAlgebra2D::intersectTwoCircles(c1, r1, c2, r2);
23
24+ ENTER_FUNCTION() << c1 << r1 << c2 << r2 << n;
25+
26+
27 if (n.size() < 2) {
28 fillFullCircle(c2, r2);
29 } else {

And it looks like the problem is in the function KisAlgebra2D::intersectTwoCircles(). When we pass two circles:

  1. QPointF(401,214.5) r = 0.25
  2. QPointF(401,215) r = 0.25

Then it generates two points that coincide:
QVector(QPointF(401,214.75), QPointF(401,214.75))

And after that we try to draw a line through these two points, which, basically, gives undefined results. So, the real fix should go to KisAlgebra2D::intersectTwoCircles()...

This revision now requires changes to proceed.Mar 20 2019, 1:47 PM

I think you need to check quadraticEquation() function. It might be that the check for discriminant is too severe and you might need to change that to D <= 1e-14 or something. If it helps, then just change it and move your rectangle check into fillHalfBrushDiff() as a sanity check.

tymond updated this revision to Diff 54481.Mar 21 2019, 12:25 PM

Move sanity checks to KisMarkerPainter

It changes the location of sanity checks and add an assert on them.
It also makes the epsilon in checking delta=0 in quadraticEquation
bigger to make sure all nearly-zero values are caught.

I moved the conditional and add qAbs to make sure negative near-zero values are caught as well.

dkazakov accepted this revision.Mon, Mar 25, 12:09 PM

The patch looks perfectly fine now, thank you! :)

This revision is now accepted and ready to land.Mon, Mar 25, 12:09 PM
This revision was automatically updated to reflect the committed changes.