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

BREAKING changing objectCaching #5567

Merged
merged 10 commits into from
Mar 16, 2019
Merged

BREAKING changing objectCaching #5567

merged 10 commits into from
Mar 16, 2019

Conversation

asturur
Copy link
Member

@asturur asturur commented Mar 9, 2019

Change the meaning of Object.objectCaching.

ObjectCaching can now deactivate cache unless the cache is needed for clipPath or other funcionalities ( future ).

@asturur
Copy link
Member Author

asturur commented Mar 11, 2019

needs a unit test to verify that works as intended.
one for Object, one for Image at least.

@melchiar
Copy link
Member

Do you think conditional caching should apply in cases where caching is preferred to be disabled, but may be required for objects with both stroke and shadow? I recently had to implement this for text since caching can cause text to appear blurry at small font sizes, but it's required to make stroke and shadows work properly together.

@asturur
Copy link
Member Author

asturur commented Mar 13, 2019

Before adding cache and requestAnimationFrame, fabric was way slower.

Now with cache everything is faster. Cache blurryness has been mostly solved last year but some things cannot be solved because are part of the caching.

Said so, i agree sometimes is better not having cache, especially for final renderings where you want best possible picture, while during animations or interaction you do not really care.

You can conditionally avoid caching per object type, if you want, assigning objectCaching false only to text for example.

I also created a different rendering method that does not cache, allow for clipping, and gives best resolution.

But for now i digged it.

I think is fine to disable caching for some objects if you want maximum clarity at expense of frame time.

i was thinking of a rendering engine that can finally update just the change objects rather than full canvas, that would give the best of both world

@melchiar
Copy link
Member

melchiar commented Mar 14, 2019

For most object types caching renders the everything flawlessly. It's only with text that I've noticed a decrease in clarity in certain cases which is why I generally disable caching with text when possible.
image

@asturur
Copy link
Member Author

asturur commented Mar 14, 2019

So for how the code is write, cached text should look perfectly fine when displayed at postion of 1px at time. If it has no stroke width for example,
Or there is a but in calculation.
If you try to set strokeWidth to 0 or to 2, with or without a stroke color, to text and leave cache on, you should have a fine result. This because with strokeWidth of 1 or 3 or 5, the text is aliased and wrote with half pixel of difference and everything is drawn across pixels.
So all the calculation i do to place the text on in the correct position are thrown off.

I cannot compensate them, because compensating would mean moving the text of 0.5px left or right and that is not right.

@melchiar
Copy link
Member

Makes sense. The issue is also practically non-existent on higher dpi displays which most devices seem to have these days so the number of people that would actually notice this is likely minimal.

A big thanks for your amazing work on this project. 👍 I only started using fabricjs a year and a half ago and have been amazed at the frequency and quality of improvements you've put into it in that short amount of time.

@asturur asturur merged commit 21fb0c3 into master Mar 16, 2019
@asturur asturur mentioned this pull request May 19, 2019
@asturur asturur deleted the change-objectCaching branch May 19, 2019 19:30
thiagocunha pushed a commit to thiagocunha/fabric.js that referenced this pull request Nov 18, 2019
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