Skip to content

Fix KisAlgebra2D::intersectLineRect()

Before this commit, function intersectLineRect was both buggy and confusing to use. This commit fixes this behaviour to be more like what's expected, and extends it to be used in more cases more easily.

Bugs/problems before this commit:

  • if you put line(A, B) to the function, the returned line would often be (B', A') instead of (A', B')
  • it used bottom() etc. to determine the edges, which because it was QRect, not QRectF, was 1 unit smaller than the actual value. That made the resulting line incorrect.
  • it always extended the line to the edges of the rectangle. While it can sometimes be the desired behaviour, it's not explained in the documentation of the function.

After this commit, the first two problems are fixed, and the last one is resolved so that now the developer can specify if they want to extend the line to the edges of the rectangle or not.

Changes in the assistants and mirroring tool preview drawing code are necessary because of the fix for the issue #2, which reversed the order of the points in the returned line.

Note

Note: there is one more issue not addressed in this commit. The API of the function states that if the line doesn't intersects the rectangle, false is returned and the line is not changed. This is not necessary convenient, since now for example in case of "just draw the line inside the viewport" the developer cannot just draw the line, they need to first check the returned value and then decide whether to draw it or not. Note that presumably lots of assistants don't check the return value and draw the lines unnecessarily outside of the viewport.

Why I wrote a whole new algorithm instead of trying the old one?

  1. I started from fixing the most visible issues, that is using bottom() and right() and returning a line with wrong order of points.
  2. However I was getting more and more issues. One of the issue I've found was the extending of the line to the edges of the rectangle - at that point, I thought it's a bug.
  3. Code that I had to write to accomodate for all the edge cases was getting too long and complicated. I checked and Liang-Barsky's algorithm looked quick and easy.
  4. That was true, I spent much more time on writing unit tests than writing the algorithm ;) But later I noticed that the previews of assistants and mirroring are wrong, and it turned out the extending was an expected feature. So I spent a little more time there and now it all is working. I preferred to keep the new algorithm because it allowed me to implement the feature of not extending the line much easier. Also see the example #2 in Test Plan, the old algorithm had weird issues that looked like they're going to be hard to debug and fix in a clean and clear code, based on my previous experience (+ especially with Qt being super dumb about the whole bottom() thing, and forcing me to use other methods to get this value...).

Test Plan

  1. Check the unit tests in libs/image/tests/kis_algebra_2d_test . The ones added by me are testLineRectIntersectionsManual and testLineRectIntersectionsRandom and should be passing.
  2. Open Krita, make a new document and check previews for mirroring and for relevant assistants: parallel ruler, infinite ruler, perspective, vanishing point, 2pp. (Note: important in 2pp is to move the center point; it showed bugs with incomplete/buggy code that just looking at the static 2pp didn't show).
  3. This is a patch with unit tests to check on master to see the issues: unit_tests_for_kisalgebra2d_intersectLineRect_for_master.diff - I commented out the swapping of the points of the line since it seems to be more random than I thought. I'd suggest checking the Manual tests only, since they're much easier to understand and visualise.

One example (before my commit), shows the issue with using right() instead of x() + width() (and incorrect order of points at the end as well):

QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() FAILURE
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() intersects:  true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() intersects expected:  true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() Extend:  true true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual()
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() original:  QLineF(QPointF(9,12),QPointF(-6,-9))
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() rectangle was:  QRect(-1,-2 5x7)
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() expected:  QLineF(QPointF(4,5),QPointF(-1,-2))
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() result:  QLineF(QPointF(-1,-2),QPointF(3,3.6))

Another issue, showing a deeper problem in the algorithm - this is a line that goes exactly through the corners of the rectangle, so the resulting line should be between one corner and the other, but the result is just one point:

QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() FAILURE
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() intersects:  true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() intersects expected:  true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() Extend:  true true
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual()
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() original:  QLineF(QPointF(-6,-9),QPointF(9,12))
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() rectangle was:  QRect(-1,-2 5x7)
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() expected:  QLineF(QPointF(-1,-2),QPointF(4,5))
QSYSTEM: KisAlgebra2DTest::testLineRectIntersectionsManual() result:  QLineF(QPointF(-1,-2),QPointF(-1,-2))

Formalities Checklist

  • I confirmed this builds.
  • I confirmed Krita ran and the relevant functions work.
  • I tested the relevant unit tests and can confirm they are not broken. (If not possible, don't hesitate to ask for help!)
  • I made sure my commits build individually and have good descriptions as per KDE guidelines.
  • I made sure my code conforms to the standards set in the HACKING file.
  • I can confirm the code is licensed and attributed appropriately, and that unattributed code is mine, as per KDE Licensing Policy.
Edited by Agata Cacko

Merge request reports