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(): use sendObjectToPlane in mergeClipPaths #8247

Merged
merged 12 commits into from
Sep 12, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Sep 4, 2022

I win 😋


That is why the following is correct.

- sendObjectToPlane(b, b.calcTransformMatrix(), a.calcTransformMatrix())
+ sendObjectToPlane(b, b.group?.calcTransformMatrix(), a.calcTransformMatrix())

Taking b's containing plane matrix and multiplying with b's own matrix gives us b.calcTransformMatrix()

I don't think so, i think you are inverting a multiplcation order.
If there is no group, you send null ( let's say is conditionally an IMatrix ).
In the case the group exist you are going to multiply forst for the group matrix and then for the object one,
in the case as it is today, you are multiplying for the ( OBJECT x CONTAINER ).

I ll write a failing unit test, and then you'll owe me a cocacola first time we ever meet.

Originally posted by @asturur in #8230 (comment)

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2022

Coverage after merging patch-merge-clip-paths into master will be

82.45%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
HEADER.js54.90%48.15%0%65.22%105, 105, 105, 12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31
src
   cache.ts97.06%90%100%100%57
   canvas.class.ts93.44%90.36%94.23%95.58%1077, 1077–1078, 1081, 1101, 1101, 1136, 1169–1170, 1198–1199, 1232, 1240, 1350–1351, 1353–1354, 1374–1375, 1533, 1538, 1548, 1552, 485–486, 491, 500, 649–651, 696–697, 761–762, 765, 767, 814–816, 858, 863–864, 892–893
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%110, 116, 127, 136, 88
   point.class.ts100%100%100%100%
   shadow.class.ts95.95%90%100%100%172, 239, 8
   static_canvas.class.ts89.69%82.87%96.70%92.57%1152–1153, 1153, 1153–1154, 1288, 1354–1355, 1358, 1407–1408, 1501, 1516, 1520, 1546–1547, 1576–1577, 1610–1611, 1652–1653, 1656, 1658, 1658, 1658, 1658, 1662, 1662, 1662–1664, 1686–1687, 1728–1729, 1732, 1734, 1734, 1734, 1734, 1738, 1738, 1738–1740, 1809, 1809–1810, 1813, 1813–1814, 1873, 1875, 1875, 1875, 1875, 1875–1876, 1879–1880, 1880, 1880–1881, 1884, 1884, 1884, 1886, 1889, 1895, 1897–1898, 1898, 1898, 1901–1902, 1902, 1902, 1905, 278–279, 281–282, 284–285, 298–299, 301–302, 616, 642, 698–701, 881, 901
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts2.99%0%0%3.92%100–101, 103, 105–107, 116, 116, 116, 118, 120, 122–124, 126–129, 137, 144, 146, 26, 31–32, 40–44, 48–52, 59–62, 70–74, 76, 84, 84, 84, 84, 84–85, 87, 87, 87–90, 92
   pattern_brush.class.ts5.26%0%0%8.33%16, 20–23, 25–26, 26, 26–29, 37–38, 40, 44, 55, 55, 55, 63–65, 65, 65, 72–73, 75–76, 76, 80
   pencil_brush.class.ts91.95%85.42%100%93.69%125–126, 155, 155–157, 279, 283, 288–289, 71–72, 87–88
   spray_brush.class.ts2.30%0%0%3.08%102–104, 106–107, 115, 115, 115, 115, 115–116, 118–119, 126–127, 129, 131–135, 144, 148–149, 149, 157, 157, 157–160, 162–165, 169–170, 172, 174–177, 180, 187–188, 190, 192–193, 195, 202–203, 205–206, 209, 209, 216, 216, 220, 25–26, 28–30, 30, 30–32, 36, 45, 52, 59, 66, 73, 80, 92–94
src/color
   color.class.ts91.67%84.51%100%94.44%325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   index.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   control.class.ts90.24%81.48%86.67%97.50%210, 304, 304, 348, 372, 6
   controls.actions.ts76.80%69.20%93.75%80.67%161, 163, 163, 163, 165, 167, 26, 28, 28, 28, 290–293, 321, 323, 330–331, 333, 333–334, 336–337, 341, 373, 375, 382–383, 385, 385–386, 388–389, 393, 421–422, 424, 426, 431, 434, 434, 434–435, 435, 435, 437, 437, 437–438, 438, 438, 441, 441, 441–442, 442, 442, 475–476, 478, 480, 485, 488, 488, 488–489, 489, 489, 491, 491, 491–492, 492, 492, 495, 495, 495–496, 496, 496, 520–522, 528, 528, 528–529, 532–535, 537, 537, 537–539, 539, 539–541, 543, 543, 543–545, 545, 545–546, 551, 551, 551–552, 554, 556–558, 57, 589–590, 592–594, 641–643, 8, 841
   controls.render.ts85.11%84.78%100%84.78%23, 27, 42–49, 58–59, 82, 86
   default_controls.ts94.29%50%100%100%115, 83
src/filters
   2d_backend.class.ts96.43%83.33%100%100%78
   WebGLProbe.ts60%80%57.14%52.17%37–38, 48–52, 66–69, 71, 77
   base_filter.class.ts28.90%31.82%36.36%26.17%100–102, 102, 102–103, 112–116, 116, 116–117, 124–125, 125, 125–128, 143, 159, 169–174, 178, 181, 181, 181–184, 184, 184–185, 185, 188–189, 195, 204–205, 210–214, 257–260, 273, 273, 273–274, 276, 292–294, 294, 294, 294, 294–295, 297, 299–300, 306–307, 309–311, 315–316, 318, 322–324, 328, 332, 352, 352, 352–356, 393, 78, 78, 78–79, 79, 79–80, 80, 80–81, 86–89, 89, 89–90, 99
   blendcolor_filter.class.ts10%4.76%28.57%9.72%104,

@ShaMan123 ShaMan123 requested a review from asturur September 4, 2022 04:52
Copy link
Contributor Author

@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.

Refactored src/util/misc/planeChange.ts a bit, to make more readable

// we are actually looking for the transformation from the destination plane to the source plane (which is a linear mapping)
// the object will exist on the destination plane and we want it to seem unchanged by it so we reverse the destination matrix (to) and then apply the source matrix (from)
const t = multiplyTransformMatrices(invertTransform(to), from);
export const sendObjectToPlane = (object: TObject, from: TMat2D = iMatrix, to: TMat2D = iMatrix): TMat2D => {
Copy link
Contributor Author

@ShaMan123 ShaMan123 Sep 4, 2022

Choose a reason for hiding this comment

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

I want to add a signature that accepts objects once we migrate Object so I can do instanceof if it won't overkill imports or make it strange

Copy link
Contributor Author

@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.

Naming: calcChangePlaneMatrix or calcPlaneChangeMatrix?

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Sep 11, 2022

fixes passing null as matrix

fabric.util.sendPointToPlane(pointer, null, target.group.calcTransformMatrix()) :

@asturur
Copy link
Member

asturur commented Sep 11, 2022

Yes you win, and now i see where i m wrong.

the multiplication of calcTransformMatrix is from the outside to the inside, while i was sure it was from the inside to the outside!!

@ShaMan123 ShaMan123 requested a review from asturur September 11, 2022 23:04
Copy link
Contributor Author

@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.

READY
If you are satisfied with naming merge it

@asturur asturur merged commit 9abe2fd into master Sep 12, 2022
@asturur
Copy link
Member

asturur commented Sep 12, 2022

maybe this wasn't a fix(), but only an optimization or a refactor

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Sep 12, 2022

it fixed the null arg usage in src/mixins/canvas_events.mixin.ts

@ShaMan123 ShaMan123 deleted the patch-merge-clip-paths branch September 12, 2022 15:08
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>
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.

2 participants