BREAKING refactor(Geometry): rm line coords #9373

ShaMan123 · 2023-09-24T08:40:42Z

Motivation

It it something we are talking about for a long time and now it is possible.

Description

First merge #9372 (removes the need for lineCoords)
Removes lineCoords from geometry and the absolute concept/arg as well as the calculate arg.
Meaning that in this PR all calculation are done in the scene plane, point args are expected to be in that plane.
This is incorrect because of stroke uniform, padding etc. but is a big step forward. I made this PR incremental and minimal as possible.
This should be considered part of the group rewrite because it make Geometry 100% safe to use with groups as opposed to before there were a number of valunrabilities (_getCoords returning own coords regardless of group, containsPoint doing something still unclear to me etc.)
Also if I am not mistaken, lineCoords were wrong when under group because they applied the viewportTransform before the group transform, it should have been vice versa.

Changes

oCoords => controlCoords
aCoords => ownCoords
calcOCoords => protected calcControlCoords
calcACoords => protected calcCornerCoords
rm _getCoords, _containsCenterOfCanvas, calcLineCoords
rm absolute, calculate for all geometry methods
rm scaleToWidth, scaleToHeight because it is not supported under group and expections are unclear regarding usage + it is a fairly simple method to write

Gist

In Action

github-actions · 2023-09-24T08:42:47Z

Build Stats

file / KB (diff)	bundled	minified
fabric	904.234 (-9.034)	303.834 (-1.638)

ShaMan123

oCoords => controlCoords
aCoords => ownCoords
progress

ShaMan123 · 2023-09-24T08:45:00Z

src/shapes/Object/Object.ts

@@ -1375,9 +1375,10 @@ export class FabricObject<
      sendObjectToPlane(this, this.getViewportTransform());
    }

+    // skip canvas zoom and calculate with setCoords now.
+    this.setCoords();


i think there is aredundant call to setCoords in this method

github-actions · 2023-09-24T09:37:14Z

Coverage after merging rm-lineCoords into master will be

82.87%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 35, 38, 41
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	9
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.76%	85.29%	87.50%	89.58%	134–135, 160–161, 32–33, 41, 50, 78, 89
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
src/Pattern
Pattern.ts	92.31%	91.89%	90%	93.55%	119, 130, 139, 32, 95
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.06%	82.35%	100%	93.81%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 149, 149, 149–152, 154–157, 161–162, 164, 166–169, 17, 172, 179, 18, 180, 182, 184–185, 187, 194–195, 197–198, 20, 201, 201, 208, 208, 21, 212, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
Canvas.ts	79.12%	77.68%	83.05%	79.61%	1000, 1003–1004, 1004, 1004, 1006, 1014, 1014, 1014–1016, 1016, 1016, 1023–1024, 1032–1033, 1033, 1033–1034, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1517, 157, 182, 292–293, 296–300, 305, 328–329, 334–339, 359, 359, 359–360, 360, 360–361, 369, 37, 374–375, 375, 375–376, 378, 387, 393–394, 394, 394, 41, 437, 445, 449, 449, 449–450, 452, 535–536, 536, 536–537, 543, 543, 543–545, 565, 567, 567, 567–568, 568, 568, 571, 571, 571–572, 575, 584–585, 587–588, 590, 590–591, 593–594, 606–607, 607, 607–608, 610–615, 621, 628, 665, 665, 665, 667, 669–674, 680, 686, 686, 686–687, 689, 692, 697, 710, 738, 738, 796–797, 797, 797–798, 800, 803–804, 804, 804–805, 807–808, 811, 811–813, 816–817, 887, 899, 906, 927, 959, 980–981, 997–998, 998, 998–999
CanvasOptions.ts	100%	100%	100%	100%
SelectableCanvas.ts	94.40%	92.21%	94.23%	96.14%	1088, 1090, 1092–1093, 301, 478–479, 481–482, 482, 482, 531–532, 593–594, 647–649, 681, 686–687, 714–715, 888, 888–889, 892, 912, 912, 961, 969
StaticCanvas.ts	96.78%	93.09%	100%	98.53%	1028, 1038, 1090–1091, 1094, 1129–1130, 1206, 1215, 1215, 1219, 1219, 1266–1267, 187–188, 204, 568, 580–581, 911–912, 912, 912–913
StaticCanvasOptions.ts	100%	100%	100%	100%
TextEditingManager.ts	84.31%	71.43%	91.67%	88%	17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
CanvasDOMManager.ts	95.52%	70%	100%	100%	21–22, 29
StaticCanvasDOMManager.ts	97.50%	88.89%	100%	100%	33
util.ts	86.67%	80.56%	83.33%	93.94%	14, 26, 63–64, 67, 67, 74, 93–94
src/color
Color.ts	94.96%	91.67%	96.30%	96.05%	233, 258–259, 267–268, 48
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	85.71%	76.92%	100%	89.74%	55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
Control.ts	93.48%	88.24%	91.67%	97.83%	181, 247, 334, 334, 369
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
fireEvent.ts	88.89%	75%	100%	100%	13
polyControl.ts	6.06%	0%	0%	11.43%	103, 117, 117, 117, 117, 117, 119–122, 122, 125, 132, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–84, 84, 86, 89–90, 90, 90, 90, 90, 92, 97
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.57%	92.94%	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%	131–132, 163–164, 171, 177, 179
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env<

ShaMan123

calcOCoords => protected calcControlCoords
calcACoords => protected calcCornerCoords
rm _getCoords, _containsCenterOfCanvas, calcLineCoords
rm absolute, calculate for all geometry methods
rm scaleToWidth, scaleToHeight because it is not supported under group and expections are unclear regarding usage + it is a fairly simple method to write

ShaMan123

good to go

ShaMan123 · 2023-09-24T10:36:16Z

test/unit/object_geometry.js

@@ -218,7 +214,7 @@
    assert.equal(object.containsPoint(point5), false);
  });

-  QUnit.test('containsPoint with padding', function(assert) {
+  QUnit.test.skip('containsPoint with padding', function(assert) {


we should discuss this.
It is not a problem respecting padding in containsPoint
I think it should be an option though.

ShaMan123 · 2023-09-24T10:38:11Z

test/unit/object_geometry.js

moved all the getCoords tests to src/shapes/Object/Object.spec.ts

restore restore restore Update Intersection.ts fix Create Intersection.spec.ts isPointInPolygon Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts update CHANGELOG.md Update CHANGELOG.md

restore restore restore

Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts

Update Object.spec.ts

ShaMan123

u

restore restore restore isPointInPolygon Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts

Update resize_filter.js

fix imports comments m comments

asturur · 2023-09-26T14:17:03Z

i would like to approach this differently.
Is this urgent for your work?

ShaMan123 · 2023-09-26T16:16:22Z

Nope
Go ahead

asturur · 2023-09-26T16:38:24Z

i would like to remove the absolute parameter in the codebase without any rename first.
Then deciding how the things should be named looking at how they can be used differently now.
I want to do that for the pleasure of coding a bit because i can do just reviews and tests and i m not having any fun

asturur · 2023-09-28T10:36:17Z

i made a different branch where i tackle the entirity of the absolute arguments in the functions ( at lesat all the one i found ), but i didn't rename anything and i still didn't test it properly.

But that is how i would like to tackle the removal of a layer of coordinates that we shouldn't have.
As soon as i manage to get all uts passing i ll open a pr
branch is: remove-absolute-globally if you are curious but is not ready for review/comments

ShaMan123 commented Sep 24, 2023

View reviewed changes

ShaMan123 requested a review from asturur September 24, 2023 10:28

ShaMan123 changed the title ~~Rm line coords~~ BREAKING: refactor(Geometry): rm line coords Sep 24, 2023

ShaMan123 changed the title ~~BREAKING: refactor(Geometry): rm line coords~~ BREAKING refactor(Geometry): rm line coords Sep 24, 2023

ShaMan123 commented Sep 24, 2023

View reviewed changes

ShaMan123 and others added 10 commits September 24, 2023 19:05

isPointInPolygon

af81347

restore restore restore Update Intersection.ts fix Create Intersection.spec.ts isPointInPolygon Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts update CHANGELOG.md Update CHANGELOG.md

Update findCrossPoint.ts

d9f87d9

Merge branch 'master' into controls-point-in-poly

b267690

benchmark

dcb8982

Merge branch 'master' into controls-point-in-poly

412e4f5

control

2deb663

restore restore restore

update CHANGELOG.md

36ea1a0

isPointInPolygon

cb8cf26

Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts

BREAKING: refactor(): containsPoint, _checkTarget

cf28bea

better breaking

2185e87

ShaMan123 force-pushed the rm-lineCoords branch from 5bb3bcb to 4293b84 Compare September 25, 2023 00:48

Delete findCrossPoint.ts

6bc9aec

Update Object.spec.ts

ShaMan123 force-pushed the rm-lineCoords branch from 4293b84 to baac40c Compare September 25, 2023 00:51

Merge branch 'master' into contains-point

bc4f89d

ShaMan123 force-pushed the rm-lineCoords branch from baac40c to 29d4cbf Compare September 26, 2023 03:36

ShaMan123 commented Sep 26, 2023

View reviewed changes

ShaMan123 added 6 commits September 26, 2023 09:13

Update CHANGELOG.md

6f40dae

control

617ddf6

restore restore restore isPointInPolygon Update Intersection.ts Update Intersection.ts Update Intersection.ts Update Intersection.ts

rm lineCoords,

0e4eac4

rename oCoords

5f8610c

rm scaleToWidth/Height

9559d07

Update resize_filter.js

cleanup

01785ae

fix imports comments m comments

restore test

4d91730

ShaMan123 force-pushed the rm-lineCoords branch from 29d4cbf to 4d91730 Compare September 26, 2023 03:45

Update CHANGELOG.md

6367485

asturur mentioned this pull request Oct 1, 2023

BREAKING: remove absolute true/false from the api. #9395

Merged

asturur closed this Oct 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING refactor(Geometry): rm line coords #9373

BREAKING refactor(Geometry): rm line coords #9373

ShaMan123 commented Sep 24, 2023 •

edited

Loading

github-actions bot commented Sep 24, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 Sep 24, 2023

github-actions bot commented Sep 24, 2023 •

edited

Loading

ShaMan123 left a comment

ShaMan123 left a comment

ShaMan123 Sep 24, 2023

ShaMan123 Sep 24, 2023

ShaMan123 left a comment

asturur commented Sep 26, 2023

ShaMan123 commented Sep 26, 2023

asturur commented Sep 26, 2023

asturur commented Sep 28, 2023

BREAKING refactor(Geometry): rm line coords #9373

BREAKING refactor(Geometry): rm line coords #9373

Conversation

ShaMan123 commented Sep 24, 2023 • edited Loading

Motivation

Description

Changes

Gist

In Action

github-actions bot commented Sep 24, 2023 • edited Loading

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Sep 24, 2023

Choose a reason for hiding this comment

github-actions bot commented Sep 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

ShaMan123 Sep 24, 2023

Choose a reason for hiding this comment

ShaMan123 Sep 24, 2023

Choose a reason for hiding this comment

ShaMan123 left a comment

Choose a reason for hiding this comment

asturur commented Sep 26, 2023

ShaMan123 commented Sep 26, 2023

asturur commented Sep 26, 2023

asturur commented Sep 28, 2023

ShaMan123 commented Sep 24, 2023 •

edited

Loading

github-actions bot commented Sep 24, 2023 •

edited

Loading

github-actions bot commented Sep 24, 2023 •

edited

Loading