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

prevent render canvas with quality less than 100% #7537

Merged
merged 2 commits into from Jan 9, 2022
Merged

prevent render canvas with quality less than 100% #7537

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
699ac68
prevent render canvas with quality less than 100%
vitonsky Dec 5, 2021
0f8236d
return from getRetinaScaling number greater or equal to 1
vitonsky Dec 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/static_canvas.class.js
View file
Open in desktop
Expand Up @@ -232,7 +232,7 @@
* @private
*/
_isRetinaScaling: function() {
return (fabric.devicePixelRatio !== 1 && this.enableRetinaScaling);
return (fabric.devicePixelRatio > 1 && this.enableRetinaScaling);
Copy link
Member

Choose a reason for hiding this comment

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

you also need to change getRetinaScaling to return a number that is at minimum 1 i think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

Choose a reason for hiding this comment

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

Maybe make sense to switch places of these parts for performance? Checking a boolean variable should be faster than comparing two values. Like this: this.enableRetinaScaling && fabric.devicePixelRatio > 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Dok11 i don't sure. JS engine probably is don't know whats variable contain, so will not check variable as boolean directly. But symbol > may suggest that it's just math operation which is instant

Copy link

Choose a reason for hiding this comment

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

@vitonsky I even tested it, and it seems like it doesn't care :D
https://jsbench.me/zkkxq3rjv0/1

image

Copy link
Member

Choose a reason for hiding this comment

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

i m sure performance is not an issue here, but in the JS bench seems like Math.random and toFixed are actually the 2 things that will matter for performance. no?

},

/**
Expand Down