fix(Intersection): bug causing selection edge case #8735

ShaMan123 · 2023-02-23T15:36:48Z

Motivation

I started experiencing a bug with selection
It collected objects if I clicked really fast on the canvas.

Description

The bug lies in isContainedInInterval
I am to blame.
I wrote that assuming a state that apparently was incorrect.

Changes

Gist

In Action

github-actions · 2023-02-23T15:38:35Z

Build Stats

file / KB (diff)	bundled	minified
fabric	890.624 (+1.468)	305.211 (+0.305)

github-actions · 2023-02-23T15:39:55Z

Coverage after merging fix-intersection-edge-case into master will be

83.17%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	18, 21, 24, 30, 33, 36
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	96.28%	96.43%	90%	98.04%	206–207, 232–233
CommonMethods.ts	100%	100%	100%	100%
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Pattern.ts	95.77%	94.29%	100%	96.43%	124, 135, 144
Point.ts	100%	100%	100%	100%
Shadow.ts	98.48%	96%	100%	100%	156
cache.ts	97.06%	90%	100%	100%	56
config.ts	75%	64.29%	66.67%	82.76%	138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
typedefs.ts	100%	100%	100%	100%
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
Canvas.ts	78.93%	77.41%	81.67%	79.60%	1000, 1000, 1000–1002, 1004–1005, 1005, 1005, 1007, 1015, 1015, 1015–1017, 1017, 1017, 1023–1024, 1032–1033, 1033, 1033–1034, 1039, 1041, 1072–1074, 1077–1078, 1082–1083, 1197–1199, 1202–1203, 1276, 1396, 1519, 1589, 161, 186, 295–296, 299–303, 308, 331–332, 337–342, 36, 362, 362, 362–363, 363, 363–364, 372, 377–378, 378, 378–379, 381, 390, 396–397, 397, 397, 40, 440, 448, 452, 452, 452–453, 455, 537–538, 538, 538–539, 545, 545, 545–547, 567, 569, 569, 569–570, 570, 570, 573, 573, 573–574, 577, 586–587, 589–590, 592, 592–593, 595–596, 608–609, 609, 609–610, 612–616, 622, 629, 669, 669, 669, 671, 673–677, 683, 689, 689, 689–690, 692, 695, 700, 713, 740, 740, 798–799, 799, 799–800, 802, 805–806, 806, 806–807, 809–810, 813, 813–815, 818–819, 889, 901, 908, 929, 961, 982–983, 999
SelectableCanvas.ts	94.51%	91.63%	94.34%	96.59%	1102, 1102–1103, 1106, 1126, 1126, 1185, 1235–1236, 1257, 1265, 1390, 1392, 1394–1395, 681–682, 684–685, 685, 685, 734–735, 796–797, 850–852, 884, 889–890, 917–918
StaticCanvas.ts	95.96%	91.29%	100%	97.92%	1083–1084, 1084, 1084–1085, 1205, 1215, 1269–1270, 1273, 1308–1309, 1386, 1395, 1395, 1399, 1399, 1446–1447, 1661–1662, 282–283, 300, 380–381, 383–384, 745, 757–758, 843
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.ts	92.16%	86.49%	100%	94.29%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
Control.ts	93.90%	88.89%	90.91%	97.73%	235, 319, 319, 354
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	87.50%	100%	66.67%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
defaultControls.ts	77.78%	66.67%	100%	100%	10, 17
drag.ts	100%	100%	100%	100%
polyControl.ts	6.15%	0%	0%	11.11%	100, 105, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–28, 30, 30, 30, 30, 30, 30, 30, 30, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.49%	92.77%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	91.67%	50%	100%	100%	20
node.ts	76.92%	50%	66.67%	88.24%	27, 31–32, 32, 32
src/filters
BaseFilter.ts	19.77%	20%	30%	17.65%	100–101, 101, 101–102, 109–112, 112, 112–113, 119, 119, 119–122, 140, 170–175, 179–180, 180, 180–183, 183,

This reverts commit eef83a6.

ShaMan123

ready

src/Intersection.ts

ShaMan123 · 2023-02-24T05:30:55Z

src/Intersection.ts

+    const a2xa1x = a2.x - a1.x,
+      a2ya1y = a2.y - a1.y,
+      b2xb1x = b2.x - b1.x,
      b2yb1y = b2.y - b1.y,
      a1xb1x = a1.x - b1.x,
-      a2ya1y = a2.y - a1.y,
-      a2xa1x = a2.x - a1.x,
+      a1yb1y = a1.y - b1.y,


changed order to be less confusing

and found the algebra behind it and documented it

ShaMan123

ready after cleanup and JSDOC

ShaMan123 · 2023-02-24T07:23:44Z

I want to tweak logic a bit more
DO NOT MERGE

asturur · 2023-02-24T09:55:07Z

I want to read this slowly because i want to understand what the bug is and how gets fixed since the code looks fine, but reading the Math require me to dedicate a bit of time to that.
Of course if you could explain the math bug....

ShaMan123 · 2023-02-24T15:20:16Z

isContainedInInterval assumed that T, A, B are points on the same line.
That assumption was wrong. I thought so because I didn't fully understand the algebra in intersectsLineLine (not that know I do, but a lot more).
You can see the test I added (intersectPolygonRectangle line edge case) that fails on master that proves the assumption was wrong.

T = (2, 2) 
A = (10, 3)
B = (30, 3)

isContainedInInterval(T, A, B)

const isContainedInInterval = (T: Point, A: Point, B: Point) => {
  const TA = new Point(T).subtract(A); === (-8, -1)
  const TB = new Point(T).subtract(B); === (28, 1)
  return (
    Math.sign(TA.x) !== Math.sign(TB.x) || Math.sign(TA.y) !== Math.sign(TB.y) // true but should be false
  );
};

The entire impl was 💩, I don't know how it was uncovered for so long. I am guessing the selection PR surfaced it.
Using vector angles is accurate but I wanted to stick to the same logic as the rest of the intersection tests.

ShaMan123

Now it is ready for sure

ShaMan123

finally renamed and support infinite to match the rest

asturur · 2023-02-25T17:28:06Z

Is not that i have any issues with a new implementation, but at this point i m curious to understand what in 5.x this was doing and what was its scope.

src/Intersection.ts

src/Point.ts

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

ShaMan123 · 2023-02-25T19:00:54Z

Is not that i have any issues with a new implementation, but at this point i m curious to understand what in 5.x this was doing and what was its scope.

same here.

ShaMan123

just the naming thing left, I don't mind to much

ShaMan123 · 2023-02-25T18:44:19Z

src/Intersection.ts

+   * @param {Point} T the point we are checking for
+   * @param {Point} A one extremity of the segment
+   * @param {Point} B the other extremity of the segment
+   * @param [extendToLine] if true checks if `T` is on the line defined by `A` and `B`


I am not sure about thte name extendToLine
I chose infinite same as the rest of the methods, but maybe that isn't great for non math users

took me a while to understand that infinite wasn't Math.inifinite, and i was telling myself, infinite is always true.
So i tried with a different name.
Was this infinite here before we restructured the class from 5.x?

I see infinite i used in all the class and i think we should come up with a better name for it
considerSegmentAsLine, or something that. can be read and let not wonder

i ll put back infinite for consistency, and then we can rename all together

Was this infinite here before we restructured the class from 5.x?

no, I put it I think to centralize logic

src/Point.ts

ShaMan123 added 2 commits February 23, 2023 17:27

fix

6863ed8

proof

b3184e6

ShaMan123 requested a review from asturur February 23, 2023 15:36

ShaMan123 changed the title ~~fix(intersection): edge case~~ fix(Intersection): edge case Feb 23, 2023

Update CHANGELOG.md

e039a3a

ShaMan123 changed the title ~~fix(Intersection): edge case~~ fix(Intersection): bug causing selection edge case Feb 23, 2023

ShaMan123 added 2 commits February 23, 2023 17:49

Update intersection.js

3c6cfc0

slope

eef83a6

ShaMan123 force-pushed the fix-intersection-edge-case branch from 4d34101 to eef83a6 Compare February 23, 2023 15:55

Revert "slope"

efbf857

This reverts commit eef83a6.

ShaMan123 commented Feb 23, 2023

View reviewed changes

src/Intersection.ts Outdated Show resolved Hide resolved

ShaMan123 added 3 commits February 24, 2023 07:02

Update intersection.js

705ce6a

zero

ab5add3

reorder

a7a02b8

ShaMan123 commented Feb 24, 2023

View reviewed changes

ShaMan123 added 7 commits February 24, 2023 07:32

Update intersection.js

122608f

Update Intersection.ts

1e6c57c

wikis

dd644b9

jsdocify

275a9cd

Update intersection.js

05af7ca

Update intersection.js

7d72d85

cleanup

7845627

ShaMan123 commented Feb 24, 2023

View reviewed changes

ShaMan123 added 2 commits February 24, 2023 17:01

pure linear algebra

8282094

Merge branch 'master' into fix-intersection-edge-case

adc3b24

ShaMan123 added 2 commits February 24, 2023 17:23

Update Intersection.ts

0df947b

Update Intersection.ts

f4fb162

ShaMan123 commented Feb 24, 2023

View reviewed changes

ShaMan123 added 2 commits February 25, 2023 06:17

rename + infinite

7214d87

coverage

9df5095

ShaMan123 commented Feb 25, 2023

View reviewed changes

Merge branch 'master' into fix-intersection-edge-case

370b175

asturur reviewed Feb 25, 2023

View reviewed changes

src/Intersection.ts Outdated Show resolved Hide resolved

asturur reviewed Feb 25, 2023

View reviewed changes

src/Point.ts Outdated Show resolved Hide resolved

ShaMan123 and others added 6 commits February 25, 2023 20:42

useful comments

74b3db7

Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

fix commit

8937960

Update Intersection.ts

387279a

Update Intersection.ts

52d7b43

Update Intersection.ts

633ec84

checkout

1c68531

ShaMan123 requested a review from asturur February 25, 2023 18:58

ShaMan123 commented Feb 25, 2023

View reviewed changes

Update Intersection.ts

acda592

asturur approved these changes Feb 25, 2023

View reviewed changes

asturur merged commit 196bea1 into master Feb 25, 2023

asturur deleted the fix-intersection-edge-case branch February 25, 2023 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Intersection): bug causing selection edge case #8735

fix(Intersection): bug causing selection edge case #8735

ShaMan123 commented Feb 23, 2023

github-actions bot commented Feb 23, 2023 •

edited

Loading

github-actions bot commented Feb 23, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Feb 24, 2023

ShaMan123 Feb 24, 2023

ShaMan123 left a comment

ShaMan123 commented Feb 24, 2023 •

edited

Loading

asturur commented Feb 24, 2023

ShaMan123 commented Feb 24, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 left a comment

asturur commented Feb 25, 2023

ShaMan123 commented Feb 25, 2023

ShaMan123 left a comment

ShaMan123 Feb 25, 2023

asturur Feb 25, 2023

asturur Feb 25, 2023

asturur Feb 25, 2023

ShaMan123 Feb 25, 2023

fix(Intersection): bug causing selection edge case #8735

fix(Intersection): bug causing selection edge case #8735

Conversation

ShaMan123 commented Feb 23, 2023

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Feb 23, 2023 • edited Loading

github-actions bot commented Feb 23, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Feb 24, 2023

Choose a reason for hiding this comment

ShaMan123 Feb 24, 2023

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 commented Feb 24, 2023 • edited Loading

asturur commented Feb 24, 2023

ShaMan123 commented Feb 24, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Feb 25, 2023

ShaMan123 commented Feb 25, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Feb 25, 2023

Choose a reason for hiding this comment

asturur Feb 25, 2023

Choose a reason for hiding this comment

asturur Feb 25, 2023

Choose a reason for hiding this comment

asturur Feb 25, 2023

Choose a reason for hiding this comment

ShaMan123 Feb 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2023 •

edited

Loading

github-actions bot commented Feb 23, 2023 •

edited

Loading

ShaMan123 commented Feb 24, 2023 •

edited

Loading

ShaMan123 commented Feb 24, 2023 •

edited

Loading