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

Load dynamic fonts to memory on all platforms, to avoid locked files. #44117

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Dec 5, 2020

Fully load dynamic fonts to memory instead of streaming from the open file.

Should fix #23773.

Bugsquad edit: Likely closes #36971 as well.

@bruvzg bruvzg added this to the 4.0 milestone Dec 5, 2020
@akien-mga
Copy link
Member

For the reference, @reduz seems to agree with the technical change:

14:54 <Akien> reduz: WDYT about #44117 ? Was there technical reasons to go with streaming from opened files instead of loading into memory for DynamicFont? The latter could solve Windows users' pain with not being able to move fonts.
14:56 <Calinou> there was also another issue reported with a large amount of file handles being created and never freed
14:56 <Calinou> (on Linux)
14:56 <Calinou> each font size had its own file handle
14:57 <reduz> Akien: nowadays it probably makes no sense to stream them, to be honest
14:57 <reduz> everything has way too much memory

@bruvzg
Copy link
Member Author

bruvzg commented Dec 5, 2020

each font size had its own file handle

This is still true, each size use it's own FT_Face instance and multiple FT_Streams with the same file handle do not work reliably.
With the changes in this PR all font sizes will share the same memory buffer.

@Calinou
Copy link
Member

Calinou commented Dec 5, 2020

This is still true, each size use it's own FT_Face instance and multiple FT_Streams with the same file handle do not work reliably.
With the changes in this PR all font sizes will share the same memory buffer.

I guess we can close #36971 as "not resolvable" then.

@bruvzg
Copy link
Member Author

bruvzg commented Dec 5, 2020

I guess we can close #36971 as "not resolvable" then.

Why? Loading fonts to memory should work for 3.2 in the same way (I'll make separate PR after testing this one), and will resolve it (file opened once per font and closed after loading).

@Calinou
Copy link
Member

Calinou commented Dec 6, 2020

@bruvzg I see, I misunderstood your previous comment.

@bruvzg bruvzg changed the title [WIP] Load dynamic fonts to memory on all platforms, to avoid locked files. Load dynamic fonts to memory on all platforms, to avoid locked files. Dec 7, 2020
@bruvzg bruvzg marked this pull request as ready for review December 7, 2020 08:27
@bruvzg
Copy link
Member Author

bruvzg commented Dec 7, 2020

3.2 PR - #44160

@akien-mga
Copy link
Member

The commit message still says "[WIP]", it might be good to amend it.

@bruvzg
Copy link
Member Author

bruvzg commented Dec 7, 2020

it might be good to amend it.

Done.

@akien-mga akien-mga merged commit 73eb8d5 into godotengine:master Dec 7, 2020
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants