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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 7 commits into from
Feb 22, 2022
Merged

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

merged 7 commits into from
Feb 22, 2022

Conversation

ShaMan123
Copy link
Contributor

remove redundant else if.
I think this might fix bugs related to strokeUniform because getObjectScaling returns abs values and here it could have been negative...

@ShaMan123
Update object.class.js
132bfa4 
redundant else if
@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |     76.7 |   86.41 |   83.05 |                                               
 fabric.js |   83.32 |     76.7 |   86.41 |   83.05 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur changed the title chore(): remove redundant code fix(): ensure scaling factor is positive for strokeUniform Feb 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.26 |    76.63 |   86.41 |   82.98 |                                               
 fabric.js |   83.26 |    76.63 |   86.41 |   82.98 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Feb 22, 2022

I don't think is going to fix anything, but i agree that scaling for sizing issue is better always positive.
I added a visual test to try to cover the edge case, but there are no visual changes.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.66 |   86.41 |   82.99 |                                               
 fabric.js |   83.27 |    76.66 |   86.41 |   82.99 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Feb 22, 2022

let me change the test with dashes, maybe there something will change

@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.66 |   86.41 |   82.99 |                                               
 fabric.js |   83.27 |    76.66 |   86.41 |   82.99 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Feb 22, 2022

offff. there is a difference in rendering between node and browsers, well it means that least with dashes something is different.
I need to take the test image from the browser and commit that, and account for the test to be disabled under node.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.66 |   86.41 |   82.99 |                                               
 fabric.js |   83.27 |    76.66 |   86.41 |   82.99 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Feb 22, 2022 •
edited
Loading

BTW I am pretty sure that in some cases (e.g. shadow) the value returned from getObjectScaling should NOT be abs

@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022 •
edited
Loading

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.27 |    76.66 |   86.41 |   82.99 |                                               
 fabric.js |   83.27 |    76.66 |   86.41 |   82.99 | ...,29780,29905,29985-30050,30173,30272,30489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Feb 22, 2022

i do not know because i do not know how drawing shadow works on the canvas internally.
We can test it with plain js canvas api to see how it behaves when the context is scaled negatively.

@asturur
Copy link
Member

asturur commented Feb 22, 2022

The test i added doesn't really detect much, because the drawing is the same with or without the fix. You can merge if you are done

@ShaMan123 ShaMan123 merged commit 23ae1a7 into master Feb 22, 2022
@ShaMan123 ShaMan123 deleted the patch-3 branch February 22, 2022 22:43
ShaMan123 added a commit that referenced this pull request Mar 2, 2022
@ShaMan123
Squashed commit of the following:
dc335c1 
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
Reviewers

@asturur asturur asturur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShaMan123 @asturur