Chore(TS): migrate Intersection #8121
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
ShaMan123
commented
Aug 5, 2022
| * @param B | ||
| * @returns true if `T` is contained | ||
| */ | ||
| const isContainedInInterval = (T: Point, A: Point, B: Point) => { |
There was a problem hiding this comment.
I am afraid future misuse of the method because it is meant to be used in a very specific context as mentioned in the comment
Any suggestions?
src/intersection.class.ts
Outdated
| * @chainable | ||
| */ | ||
| appendPoint(point) { | ||
| if (!this.contains(point)) { |
There was a problem hiding this comment.
append only unique points
| */ | ||
| appendPoints(points) { | ||
| this.points = this.points.concat(points.filter(point => { | ||
| return !this.contains(point); |
There was a problem hiding this comment.
append only unique points
There was a problem hiding this comment.
is this hot code? why does it matters? If this is used during render i don't really want to do it.
There was a problem hiding this comment.
hot code? What's that?
There was a problem hiding this comment.
How damaging is this loop?
we can make it a map instead (I think it is insignificant)
consider that most case points intersection is <=2
There was a problem hiding this comment.
only 2 polygons that are almost identical will have <= n-1
There was a problem hiding this comment.
hot code? What's that?
ahh code used by the dev?
yes, getting intersection points in crucial for ui displaying these points
There was a problem hiding this comment.
with hot code i mean code we use in tight loops during rendering. Goint to check how many intersections we use
There was a problem hiding this comment.
Ok is fine, the Intersection is just a fancy container for points, the math to find the point didn't change. I'm good with this.
| uaT = (b2.x - b1.x) * (a1.y - b1.y) - (b2.y - b1.y) * (a1.x - b1.x), | ||
| ubT = (a2.x - a1.x) * (a1.y - b1.y) - (a2.y - a1.y) * (a1.x - b1.x), | ||
| uB = (b2.y - b1.y) * (a2.x - a1.x) - (b2.x - b1.x) * (a2.y - a1.y); | ||
| static intersectLineLine(a1, a2, b1, b2, aIinfinite = true, bIinfinite = true) { |
There was a problem hiding this comment.
added option for infinite line check
| let result; | ||
| const uaT = (b2.x - b1.x) * (a1.y - b1.y) - (b2.y - b1.y) * (a1.x - b1.x), | ||
| ubT = (a2.x - a1.x) * (a1.y - b1.y) - (a2.y - a1.y) * (a1.x - b1.x), | ||
| uB = (b2.y - b1.y) * (a2.x - a1.x) - (b2.x - b1.x) * (a2.y - a1.y); |
There was a problem hiding this comment.
who ever wrote this knows their algerba
I can't understand it exactly, just the main concept
There was a problem hiding this comment.
some matrix det/cross clever calculation
well done who ever you are
There was a problem hiding this comment.
Some of those can be probably grouped in smaller const, i m not sure the extra const are worth the subtraction saved.
There was a problem hiding this comment.
I thought of it too.
Wasn't sure how to name what...
| const segmentsCoincide = aIinfinite || bIinfinite | ||
| || isContainedInInterval(a1, b1, b2) || isContainedInInterval(a2, b1, b2) | ||
| || isContainedInInterval(b1, a1, a2) || isContainedInInterval(b2, a1, a2); | ||
| result = new Intersection(segmentsCoincide ? 'Coincident' : undefined); |
There was a problem hiding this comment.
If segments intersection => check that coincident happens in range
fixed!
|
|
||
| inter = Intersection.intersectLineLine(a1, a2, b1, b2, infinite, false); | ||
| if (inter.status === 'Coincident') { | ||
| return inter; |
There was a problem hiding this comment.
fixed detection coincident (someone left a note about it)
|
|
||
| result.appendPoints(inter.points); | ||
| if (coincidents.length > 0 && coincidents.length === points1.length && coincidents.length === points2.length) { | ||
| return new Intersection('Coincident'); |
There was a problem hiding this comment.
same fix of TODO comment
ShaMan123
commented
Aug 5, 2022
| * @return {Intersection} thisArg | ||
| * @chainable | ||
| */ | ||
| private append(...points) { |
There was a problem hiding this comment.
refactored appendPoint appendPoints to append es6!
I don't consider it breaking as this method is private
Code Coverage Summary |
case when polygons are the same, one has more points
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
ShaMan123
commented
Aug 17, 2022
| @@ -124,10 +124,14 @@ export class Intersection { | |||
|
|
|||
| /** | |||
| * Checks if line intersects polygon | |||
| * | |||
| * @todo account for stroke | |||
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
Code Coverage Summary |
asturur
approved these changes
Aug 30, 2022
This was referenced Sep 7, 2022
frankrousseau
pushed a commit
to cgwire/fabric.js
that referenced
this pull request
Jan 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
first merge #8120 and update from mastercode remarks:
Should we create classes instead of the static methods?
new LinePolygonIntersection(...args).statusChanges
lineandsegment(lineis infinite) - BREAKING, older versions should renameLinetoSegment(what was namedLinewas a finite line which is now called a segment)??
Is
segmentthe best naming? Interval seems to be the correct math naming...