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: change canvas lib #2785

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

fix: change canvas lib #2785

wants to merge 7 commits into from

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Mar 4, 2024

I feel like this should go to 3.3, but it might be needed to fix some bugs in 3.2
This is now planned to land in 3.4 immediately after the release of 3.3

The skia-canvas lib we are using is problematic. There was a bug where loading the emoji font on windows could consume all available memory and crash #2714 , and another bug where it (also on windows) never returns when rendering certain unicode characters.

Also while at one point the library looked promising, it now looks to be abandoned. While there might be some viable forks of that library, this proposed change is definitely maintained better. (Long term, we may want to consider canvas, but that currently wont work due to not being node-api based yet).

This needs some more testing, to verify it doesn't have the same flaws on windows.
And some testing to verify the drawing in more scenarios. My quick testing shows it to be something like a pixel off sometimes (pixel rounding?), but is otherwise identical to 3.2.2

@dnmeid
Copy link
Member

dnmeid commented Mar 6, 2024

When I started the canvas stuff, I actually tried first with napi-rs/canvas, but unfortunately I couldn't get it to work by then. Also skia-canvas has some nice features on top of skia which napi-rs/canvas is missing. Mainly I was interested in the built in word-wrap/multiline support.
At the end I found that the word wrap didn't had the flexibility we need with some custom valid line breaking characters and I stayed with own solution. So if now skia-canvas is loosing maintanance and napi-rs/canvas does work, I'm fine with the change.

Regarding the pixel preciceness, with skia-canvas I had often to position lines at .5 positions to fit exactly one pixel. This is also the correct way like it is in skia.

I have now a Windows machine at hand and can give the PR a try in the next few days.

@Julusian
Copy link
Member Author

Julusian commented Mar 6, 2024

So far I haven't done any refinement other than fixing one very noticable issue with line heights, which was a simple swapping of some property names.
My main interest is whether it has the same 'fatal' bugs as the previous lib did, I will need to ask someone who had those issues previously to test and verify.

And I haven't confirmed that this works in the built files that we distribute yet, but I don't expect that to be impossible, just needs some config tweaking to get setup

@Julusian Julusian added this to the v3.3 milestone Mar 6, 2024
@Julusian Julusian modified the milestones: v3.3, v3.develop Apr 10, 2024
@CLAassistant
Copy link

CLAassistant commented Apr 10, 2024

CLA assistant check
All committers have signed the CLA.

@Julusian Julusian changed the base branch from stable-3.2 to main April 10, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants