Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 30 commits into from
Feb 25, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
## [next]

- fix(Intersection): bug causing selection edge case [#8735](https://github.com/fabricjs/fabric.js/pull/8735)
- chore(TS): class interface for options/brevity [#8674](https://github.com/fabricjs/fabric.js/issues/8674)
- ci(): fix import autocomplete in dev mode #8725
- chore(): remove deprecated class util [#8731](https://github.com/fabricjs/fabric.js/pull/8731)
- lint(): fix eslint errors [#8729](https://github.com/fabricjs/fabric.js/pull/8729)
- fix(TS): `this.constructor` types [#8675](https://github.com/fabricjs/fabric.js/issues/8675)
- fix(DraggableText): drag image blur [#8712](https://github.com/fabricjs/fabric.js/pull/8712)
Expand Down
69 changes: 46 additions & 23 deletions src/Intersection.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Point } from './Point';
import { createVector, slope } from './util/misc/vectors';
import { createVector } from './util/misc/vectors';

/* Adaptation of work of Kevin Lindsey (kevin@kevlindev.com) */

Expand Down Expand Up @@ -40,27 +40,50 @@ export class Intersection {
}

/**
* check if `T` is contained in `[A, B]` by comparing the direction of the vectors from `T` to `A` and `B`
* @param T
* @param A
* @param B
* check if point T is on the segment or line defined between A and B
*
* @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`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

no, I put it I think to centralize logic

* @returns true if `T` is contained
*/
static isContainedInInterval(T: Point, A: Point, B: Point) {
if (T.eq(A) || T.eq(B)) {
return true;
}
const AB = createVector(A, B);
if (!AB.eq(new Point())) {
const s = slope(AB);
return s === slope(createVector(T, B)) && s === slope(createVector(T, B));
static isPointContained(T: Point, A: Point, B: Point, extendToLine = false) {
if (A.eq(B)) {
// Edge case: the segment is a point, we check for coincidence,
// extendToLine has no meaning because there are infinite lines to consider
return T.eq(A);
} else if (A.x === B.x) {
// Edge case: horizontal line.
// we first check if T.x is on the same value, and then if T.y is contained between A.y and B.y
return (
T.x === A.x &&
(extendToLine || (T.y >= Math.min(A.y, B.y) && T.y <= Math.max(A.y, B.y)))
);
} else if (A.y === B.y) {
// Edge case: vertical line.
// we first check if T.y is on the same value, and then if T.x is contained between A.x and B.x
return (
T.y === A.y &&
(extendToLine || (T.x >= Math.min(A.x, B.x) && T.x <= Math.max(A.x, B.x)))
);
} else {
return false;
// generic case: we normalize AT over AB.
// then for the line case we just verify that AB has the same inclination than AT
// for the segment case we need the same direction and the length of AT needs to be less than AB
const AB = createVector(A, B);
const AT = createVector(A, T);
const s = AT.divide(AB);
return infinite
? Math.abs(s.x) === Math.abs(s.y)
: s.x === s.y && s.x >= 0 && s.x <= 1;
}
}

/**
* Checks if a line intersects another
* @see {@link https://en.wikipedia.org/wiki/Line%E2%80%93line_intersection line intersection}
* @see {@link https://en.wikipedia.org/wiki/Cramer%27s_rule Cramer's rule}
* @static
* @param {Point} a1
* @param {Point} a2
Expand All @@ -78,12 +101,12 @@ export class Intersection {
aInfinite = true,
bInfinite = true
): Intersection {
const b2xb1x = b2.x - b1.x,
a1yb1y = a1.y - b1.y,
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,
Comment on lines +107 to +112
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed order to be less confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and found the algebra behind it and documented it

uaT = b2xb1x * a1yb1y - b2yb1y * a1xb1x,
ubT = a2xa1x * a1yb1y - a2ya1y * a1xb1x,
uB = b2yb1y * a2xa1x - b2xb1x * a2ya1y;
Expand All @@ -105,11 +128,11 @@ export class Intersection {
const segmentsCoincide =
aInfinite ||
bInfinite ||
Intersection.isContainedInInterval(a1, b1, b2) ||
Intersection.isContainedInInterval(a2, b1, b2) ||
Intersection.isContainedInInterval(b1, a1, a2) ||
Intersection.isContainedInInterval(b2, a1, a2);
return new this(segmentsCoincide ? 'Coincident' : undefined);
Intersection.isPointContained(a1, b1, b2) ||
Intersection.isPointContained(a2, b1, b2) ||
Intersection.isPointContained(b1, a1, a2) ||
Intersection.isPointContained(b2, a1, a2);
return new Intersection(segmentsCoincide ? 'Coincident' : undefined);
} else {
return new Intersection('Parallel');
}
Expand Down
7 changes: 3 additions & 4 deletions src/Point.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ export interface IPoint {
*/
export class Point implements IPoint {
declare x: number;

declare y: number;

static ZERO = new Point();

constructor();
constructor(x: number, y: number);
constructor(point: IPoint);
Expand Down Expand Up @@ -356,7 +357,7 @@ export class Point implements IPoint {
* @param {TRadian} radians The radians of the angle for the rotation
* @return {Point} The new rotated point
*/
rotate(radians: TRadian, origin: IPoint = originZero): Point {
rotate(radians: TRadian, origin: IPoint = Point.ZERO): Point {
// TODO benchmark and verify the add and subtract how much cost
// and then in case early return if no origin is passed
const sinus = sin(radians),
Expand Down Expand Up @@ -384,5 +385,3 @@ export class Point implements IPoint {
);
}
}

const originZero = new Point(0, 0);
ShaMan123 marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 0 additions & 7 deletions src/util/misc/vectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,6 @@ export const calcAngleBetweenVectors = (a: Point, b: Point): TRadian => {
return Math.atan2(det, dot) as TRadian;
};

export const slope = (v: Point) =>
v.x === 0 && v.y === 0
? 0
: v.x === 0
? Infinity * Math.sign(v.y)
: v.y / v.x;

/**
* Calculates the angle between the x axis and the vector
* @param {Point} v
Expand Down
Loading