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(): fix modify polygon points with zero sized polygons ( particular case of axis oriented lines ) #9575

Merged
merged 13 commits into from
Jan 7, 2024

Conversation

asturur
Copy link
Member

@asturur asturur commented Jan 6, 2024

Description

Since the polygon points are by design ignoring padding, we do not need to use the standard getLocalPoint and then deal with calculations that involve width/height directly.

We can use a plain matrix that should take care of placing the new point correctly.

This PR fixes the bug mentioned in #9556 and also fixes the displacement of the polygon when skew is involved

closes #9556

In Action

Copy link
Contributor

github-actions bot commented Jan 6, 2024

Build Stats

file / KB (diff) bundled minified
fabric 907.774 (-0.703) 305.109 (-0.264)

const mouseLocalPosition = sendPointToPlane(
new Point(x, y),
undefined,
poly.calcTransformMatrix()
Copy link
Member Author

Choose a reason for hiding this comment

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

@ShaMan123 i do remember that x,y are always in the scene coordinates, but i m not sure that this is true also for when we run an action inside an interactive group. Do you remember if that was changed to accomodate interactive groups?

If those are in parent coordinates i have to change this with calcOwnTransform()

Copy link
Contributor

Choose a reason for hiding this comment

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

I am very surprised to see x,y in the parent plane and not in the scene plane
bad decision I made at that point to limit the amount of changes

Screenshot 2024-01-06 at 20 04 17

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah we talked about this, but i wanted to be sure. So i ll use calcOwnMatrix

);
const diff = newAnchorPointInParentPlane.subtract(anchorPointInParentPlane);
poly.left -= diff.x;
poly.top -= diff.y;
Copy link
Member Author

Choose a reason for hiding this comment

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

@ShaMan123 this approach with diffing the position of the anchor point in a reference plance, before and after the action works. Now also skew works fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@asturur asturur requested a review from ShaMan123 January 6, 2024 23:54
ShaMan123
ShaMan123 previously approved these changes Jan 7, 2024
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

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

This looks great
I would do a small syntax change in createPolyPositionHandler to match the rest
Screenshot 2024-01-07 at 8 08 32

Copy link
Contributor

github-actions bot commented Jan 7, 2024

Coverage after merging ISSUE-9556-polycontrols into master will be

82.83%

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.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   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.ts74.14%65%66.67%82.76%131, 139, 139, 139, 139, 139–141, 152–154, 30
   constants.ts100%100%100%100%
src/LayoutManager
   LayoutManager.ts88.05%89.83%76.92%90.54%136–137, 139–140, 140, 140, 140, 140, 234, 297–298, 309, 47
   constants.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts82.05%72.22%100%88.89%30–31, 42, 54, 57–58, 65
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts20%0%0%40%20–21, 23, 23, 23, 23, 23
   LayoutStrategy.ts86.54%64.71%100%96.30%36, 43, 43, 43, 51, 78–79
   utils.ts91.67%85.71%100%100%28, 34
src/Pattern
   Pattern.ts90.54%93.94%80%90.32%119, 130, 139, 32, 36
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, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79%76.99%83.05%79.84%1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.21%91.60%94.44%94.22%1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
   StaticCanvas.ts96.53%93.09%98.92%98.29%1019, 1029, 1080–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts86%71.43%91.67%91.67%17, 17, 17–18, 18, 18
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, 354
   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.ts10.87%0%0%16.13%114, 114, 114, 114, 114, 116–119, 119, 122, 129, 24–26, 50–52, 58–59, 61, 71, 77–79, 79, 81, 84, 86, 90–92, 94, 99
   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<

@asturur
Copy link
Member Author

asturur commented Jan 7, 2024

I made the test case larger and included all variation of combination of settings of polygons ( 4 ) all with skewX/skewY/angle/scale and it works fine now.

@asturur
Copy link
Member Author

asturur commented Jan 7, 2024

Test cases for me are fundamental, not having a ready test case sometimes stops me from even trying to address a bug

@asturur asturur merged commit add96d3 into master Jan 7, 2024
22 checks passed
@asturur asturur deleted the ISSUE-9556-polycontrols branch January 7, 2024 13:39
@ShaMan123
Copy link
Contributor

ShaMan123 commented Jan 7, 2024

I agree but we should stick to playwright and use what you did in the vanilla app

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.

[Bug]: createPolyControls with two points (Line) has locked axis
2 participants