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(Geometry): containsPoint - BREAKING Canvas#_checkTarget #9372

Merged
merged 17 commits into from
Sep 26, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Sep 24, 2023

Motivation

containsPoint is not user freinly and that is a shame.
It accepts points in different planes depending on use cases and is to me at least unintuitive.

This is actually a part of #8767 - Since you have freezed it I have decided to extract this important part immediately.

Description

First merge #9333

Changes

BREAKING

  • containsPoint signature containsPoint(point, lines, absolute, calculate) => containsPoint(point, absolute, calculate)
  • _checkTarget signature _checkTarget(pointer, target, globalPointer) => _checkTarget(target, globalPointer = pointerFromViewport)

Gist

I believe it is now possible to remove lineCoords

In Action

@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2023

Build Stats

file / KB (diff) bundled minified
fabric 911.252 (-0.877) 305.214 (-0.164)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 24, 2023

Coverage after merging contains-point into master will be

82.92%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.71%94.64%86.67%97.09%101, 104, 207–208, 233–234
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%119, 130, 139, 32, 95
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%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.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%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.ts78.99%77.35%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, 906, 906, 927, 959, 980–981, 997–998, 998, 998–999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts94.38%92.21%94.12%96.13%1075, 1077, 1079–1080, 301, 466–467, 469–470, 470, 470, 519–520, 581–582, 635–637, 669, 674–675, 702–703, 875, 875–876, 879, 899, 899, 948, 956
   StaticCanvas.ts96.78%93.09%100%98.53%1031, 1041, 1093–1094, 1097, 1132–1133, 1209, 1218, 1218, 1222, 1222, 1269–1270, 187–188, 204, 571, 583–584, 914–915, 915, 915–916
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts84.31%71.43%91.67%88%17–18, 18, 18–19, 19, 19
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 353
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts6.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.ts19.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.ts93.57%92.94%100%93.67%129–130, 132–134, 148–149, 181–183, 42
   scaleSkew.ts78.79%64.29%100%85.71%27, 29, 29, 29, 31, 33, 35
   skew.ts91.03%79.31%100%97.67%131–132, 163–164, 171, 177, 179
   util.ts100%100%100%100%
   wrapWithFireEvent.ts100%100%100%100%
   wrapWithFixedAnchor.ts100%100%100%100%
src/env</

ShaMan123 and others added 3 commits September 24, 2023 19:05
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 Object.spec.ts
@ShaMan123 ShaMan123 changed the title BREAKING refactor(): containsPoint fix(Geometry): containsPoint - BREAKING Canvas#_checkTarget Sep 26, 2023
@zhe-he
Copy link
Contributor

zhe-he commented Sep 26, 2023

Excuse me for interrupting. Is this case overwritten?

const rect = new Rect({ left: 0, top: 0, width: 100, height: 100 });
const g = new Group([rect], { left: 0, top: 0 });
const g2 = new Group([g], { left: 10, top 10 });
canvas.add(g2);
const p = new Point(5, 5);

const flag = g.containsPoint(p);
console.log(flag);

Or do I need to manually remove the child elements from the group and then evaluate, and once evaluated, restore them, like this?

const rect = new Rect({ left: 0, top: 0, width: 100, height: 100 });
const g = new Group([rect], { left: 0, top: 0 });
const g2 = new Group([g], { left: 10, top 10 });
canvas.add(g2);
const p = new Point(5, 5);

+ const gg = g.group;
+ gg.remove(g);

const flag = g.containsPoint(p);

+ gg.add(g);
console.log(flag); // Is this the correct way to achieve the desired result?

// Do not check for currently grouped objects, since we check the parent group itself.
// until we call this function specifically to search inside the activeGroup
while (i--) {
const objToCheck = objects[i];
const pointerToUse = objToCheck.group
? this._normalizePointer(objToCheck.group, pointer)
Copy link
Member

Choose a reason for hiding this comment

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

_normalizePointer can go away now. It was used only for this and was explicitly private

Copy link
Member

Choose a reason for hiding this comment

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

i removed it

// http://idav.ucdavis.edu/~okreylos/TAship/Spring2000/PointInPolygon.html
obj.containsPoint(pointer)
obj.containsPoint(
sendPointToPlane(pointer, undefined, this.viewportTransform),
Copy link
Member

Choose a reason for hiding this comment

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

can we use this.restorePointerVpt(pointer) here?

We are taking the pointer in html coordinates and we are transitioning it to scene coordinates correct?

Copy link
Member

Choose a reason for hiding this comment

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

this change and looking at possibly adding tests is the only thing i need to merge this

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 deliberately removed this usage
You removed restoreVpt so why leave this?
It is removed in the getPointer renaming pr, is it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is yet another method to explain what it does with 1 or 2 usages abd as confusung as getPointer

@asturur
Copy link
Member

asturur commented Sep 26, 2023

#9333 has 4000 lines to read and requires 2 screens to do it comfortably, on the small notebook screen is a pain to compare old and new tests.
I think the dependency of #9333 has been removed yesterday when i fixed the failing tests on the qunit side.

@asturur
Copy link
Member

asturur commented Sep 26, 2023

i ll add specific tests for the new functionality in a following PR.
We need some small test that can cover nested stuff properly for containsPoint and checkTarget

@asturur asturur merged commit f7ff31d into master Sep 26, 2023
24 checks passed
@asturur asturur deleted the contains-point branch September 26, 2023 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants