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

feat(util): transform utils #7614

Merged
merged 55 commits into from Feb 24, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jan 16, 2022

Making transformations easier and accessible
Added the following utils:
2. sendPointToPlane
3. transformPointRelativeToCanvas
4. sendObjectToPlane

https://codesandbox.io/s/fabric-app-forked-bipwk

TODO

  • tests

ezgif com-gif-maker (9)

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 16, 2022

Is this normal?
image

click to expand test results
ok 98 fabric.util > transformPoint
not ok 99 fabric.util > getTransformMatrixByObject
  ---
  message: failed
  severity: failed
  actual  : [
  3.000000000000001,
  6,
  6,
  7.999999999999999,
  27,
  42
]
  expected: [
  3,
  6,
  6,
  8,
  27,
  42
]
  stack: |
        at Object.<anonymous> (C:\Users\DELL\Desktop\DEV\fabric.js\test\unit\util.js:864:12)
        at runTest (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2411:35)
        at Test.run (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2394:9)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2645:16
        at processTaskQueue (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1966:26)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1970:13
  ...
  ---
  message: failed
  severity: failed
  actual  : [
  1.0000000000000002,
  2,
  3,
  3.9999999999999996,
  5,
  6
]
  expected: [
  1,
  2,
  3,
  4,
  5,
  6
]
  stack: |
        at Object.<anonymous> (C:\Users\DELL\Desktop\DEV\fabric.js\test\unit\util.js:866:12)
        at runTest (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2411:35)
        at Test.run (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2394:9)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2645:16
        at processTaskQueue (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1966:26)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1970:13
  ...
not ok 100 fabric.util > calcTransformationBetweenObjectPlanes
  ---
  message: failed
  severity: failed
  actual  : [
  3.0000000000000004,
  4,
  9,
  7.999999999999999,
  25,
  16
]
  expected: [
  3.000000000000001,
  4,
  9,
  7.999999999999999,
  25,
  16
]
  stack: |
        at Object.<anonymous> (C:\Users\DELL\Desktop\DEV\fabric.js\test\unit\util.js:938:12)
        at runTest (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2411:35)
        at Test.run (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2394:9)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2645:16
        at processTaskQueue (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1966:26)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1970:13
  ...
ok 101 fabric.util > transformPointBetweenObjectPlanes
ok 102 fabric.util > transformPointRelativeToCanvas
not ok 103 fabric.util > sendObjectToPlane
  ---
  message: failed
  severity: failed
  actual  : [
  -20.99999999999999,
  13.000000000000005,
  -14.999999999999988,
  9,
  -21.999999999999993,
  11.000000000000002
]
  expected: [
  -20.999999999999993,
  13,
  -14.999999999999996,
  9,
  -21.999999999999996,
  11
]
  stack: |
        at Object.<anonymous> (C:\Users\DELL\Desktop\DEV\fabric.js\test\unit\util.js:1061:12)
        at runTest (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2411:35)
        at Test.run (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2394:9)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2645:16
        at processTaskQueue (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1966:26)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1970:13
  ...
  ---
  message: failed
  severity: failed
  actual  : [
  18.00000000000002,
  10.000000000000036,
  12.000000000000009,
  6.000000000000018,
  16.00000000000001,
  6.000000000000014
]
  expected: [
  18,
  10,
  12,
  6,
  16,
  6.0000000000000036
]
  stack: |
        at Object.<anonymous> (C:\Users\DELL\Desktop\DEV\fabric.js\test\unit\util.js:1063:12)
        at runTest (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2411:35)
        at Test.run (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2394:9)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:2645:16
        at processTaskQueue (C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1966:26)
        at C:\Users\DELL\Desktop\DEV\fabric.js\node_modules\qunit\qunit\qunit.js:1970:13
  ...
ok 104 fabric.util > makeBoundingBoxFromPoints

@ShaMan123 ShaMan123 changed the title feat(util): transform point utils feat(util): transform utils Jan 16, 2022
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 16, 2022

This is ready and tested!
We simply need to understand the small diff in outcomes

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Jan 16, 2022

I am a bit confused regarding transformPoint util.
I'll run a test to make sure I didn't confuse the order of transformations

EDIT
I was confused. fixed now.

@asturur
Copy link
Member

asturur commented Feb 23, 2022

Merging this and fixing the unit test with a commit later.

asturur
asturur previously approved these changes Feb 23, 2022
@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 23, 2022

Now I think sendObjectToPlane and sendChildToPlane are pretty much the same.
For the sake of consistency I think of removing sendObjectToPlane (the clipPath issue) and renaming sendChildToPlane to sendObjectToPlane

@asturur
Copy link
Member

asturur commented Feb 23, 2022

do you want to make the change now or later?

@ShaMan123
Copy link
Contributor Author

now, I'm refactoring

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 23, 2022

I have refactored this PR heavily because of the clip path (and other orphans) not aware of the fact they have parents.
It's sad.
It was a good util but clip path broke it.
It still has a vulnerability getPlaneMatrix that I don't like, see my comment.
I was thinking to fix it by adding a parent property to clip path but then I remembered you said there is an option for objects to share a clip path so I backed off.
Now it is more generic and is basically the minions demo into a util.

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 23, 2022

Maybe we can mark an object as a clipPath so then we can throw an error from calcPlaneMatrix
Here for example

if (this.clipPath) {

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 23, 2022

and I'm not sure about transformPointRelativeToCanvas as well, isn't Canvas.getPointer the right thing to use?

I looked around and encountered _normalizePointer which is a combination of transformPointRelativeToCanvas and sendPointToPlane and restorePointerVpt which is exactly what transformPointRelativeToCanvas does
Then I saw isTargetTransparent accepts a global pointer which is very confusing IMO and should conform to canvas coordinate system.

So I think what I want is a reversed function for Canvas.getPointer

@ShaMan123
Copy link
Contributor Author

so all in all I think to remove transformPointRelativeToCanvas and standardize Canvas.getPointer
Thoughts?

@asturur
Copy link
Member

asturur commented Feb 24, 2022

imho you are aiming at a moving target.
clipPath is full of edge cases, and probably should be simplified and not drive our efforts.

We should examine, does forbidding to share clipPaths block real use cases or make them extremely complicated?
if yes, can it be solved adding the clipPath to a group?

I have experienced many time following a refactor idea and the get stopped at the end by something it couldn't work because of requirement x.

Are you satisfied with this PR? should i merge it?
I would like to start looking at the group myself and extract the first parts to merge.

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 24, 2022

I am pretty satisified, perhaps I'll rework this in the future

@ShaMan123 ShaMan123 merged commit 109efe5 into fabricjs:master Feb 24, 2022
@ShaMan123 ShaMan123 deleted the transform-point-util branch February 24, 2022 09:38
ShaMan123 added a commit that referenced this pull request Mar 2, 2022
commit 109efe5
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Thu Feb 24 11:38:26 2022 +0200

    feat(util): transform utils (#7614)

    * Update misc.js

    * Update misc.js

    * rename

    * better JSDOC

    * Update misc.js

    * Update util.js

    * ci(): lint

    * better JSDOC

    * `sendObjectToPlane`

    * Update misc.js

    * Update util.js

    * Update util.js

    * Update misc.js

    * rename

    * Update misc.js

    * Update util.js

    * Update misc.js

    * remove redundant tests

    * Update util.js

    * Update misc.js

    * Update misc.js

    * Update util.js

    * Update util.js

    * fix(): reversed transform order

    * allow passing null for `sourceObject`

    * Update util.js

    * lint

    * Update misc.js

    * Update misc.js

    * ci: adjust tests to accept error

    * Update misc.js

    * Update util.js

    * build

    * Revert "Update misc.js"

    This reverts commit fb83a71.

    * Update misc.js

    * checkout

    * Update util.js

    * Update misc.js

    * Update util.js

    * typo

    * Update misc.js

    * optional parent

    * refactor around orphan objects

    * Update misc.js

    * add warning

    * Update object_geometry.mixin.js

    * lint

    * rename

    * JSDOC

    * Revert "JSDOC"

    This reverts commit a88b0ab.

    * remove unsafe `calcPlaneMatrix`

    * typo

    * Update misc.js

    * Update misc.js

commit 1d447b0
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Thu Feb 24 11:16:43 2022 +0200

    chore/fix(v6): prerequisites for Group (#7728)

commit f13075c
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Thu Feb 24 10:03:22 2022 +0100

    tests() adding an extra controls test where the group are transformed (#7736)

commit bae062c
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Thu Feb 24 10:10:20 2022 +0200

    chore(): Group prerequisite minor refactor object_origin

commit 23ae1a7
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Wed Feb 23 00:43:36 2022 +0200

    fix(): ensure scaling factor is positive for strokeUniform (#7729)

    * Update object.class.js

    redundant else if

    * added a visual test

    * removed useless file

    * prefer dashes

    * prefer dashes

    * modified golden

    * more tolerance

    Co-authored-by: Andrea Bogazzi <andreabogazzi79@gmail.com>

commit 84e405d
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Mon Feb 21 14:52:07 2022 +0200

    MAJOR chore(v6): neutral prerequisites for fabric.Group rework (#7726)

commit 0c98cee
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Mon Feb 21 08:25:42 2022 +0100

    chore(): update CHANGELOG

commit 6565db1
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Mon Feb 21 07:30:19 2022 +0200

    fix(): add `eraser` to Object state/cache props (#7720)

    * Update eraser_brush.mixin.js

    * Update eraser_brush.mixin.js

commit fd1b0c3
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 17:18:19 2022 +0200

    feat(Object.isType): accept multiple `type` (#7715)

commit 4e80e77
Author: Andrea Bogazzi <andreabogazzi79@gmail.com>
Date:   Sun Feb 20 16:12:36 2022 +0100

    updated package.json

commit fc902cb
Author: CommanderRoot <CommanderRoot@users.noreply.github.com>
Date:   Sun Feb 20 13:40:29 2022 +0100

    chore(): Replace deprecated String.prototype.substr() with Array.prototype.slice() (#7696)

    String.prototype.substr() is deprecated (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr) so we replace it with slice() which works similarily but isn't deprecated.
    Signed-off-by: Tobias Speicher <rootcommander@gmail.com>

commit 30c0c19
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 14:39:15 2022 +0200

    MAJOR feat(fabric.Point): divide, scalarDivide, scalarDivideEquals (#7716)

    * **BREAKING**: divide, scalarDivide, scalarDivideEquals

commit 2d922e1
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 14:37:52 2022 +0200

    chore(): use Array.isArray instead of ie6+ workarounds (#7718)

commit 88b425c
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 14:24:21 2022 +0200

    MAJOR feat(): Reuse fabric.Point logic for scaling and naming consistency (#7710)

    * fix(Object): object scaling inconsisteny

    **MAJOR**
     **BREAKING** `Object.getObjectScaling`, `Object.getTotalObjectScaling`

commit 7e563c7
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 02:19:13 2022 +0200

    chore(): BREAKING Cleanup fabric.Point for v6 (#7709)

    * Update misc.js

    * point **BREAKS** `multiply`

    * Update point.js

    * revert adding error check

commit 4c2e9a6
Author: Shachar <34343793+ShaMan123@users.noreply.github.com>
Date:   Sun Feb 20 01:52:47 2022 +0200

    tests(fabric.animation): fix test reliability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants