-
-
Notifications
You must be signed in to change notification settings - Fork 529
LunaSVG: add custom font support #4620
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
base: master
Are you sure you want to change the base?
Conversation
Lua API: svgRegisterFont(string fontFamily, string path) svgUnregisterFont(string fontFamily)
This reverts commit 969dfec.
This reverts commit 7c29995.
| CResource* pResource = pLuaMain->GetResource(); | ||
| if (!pResource) | ||
| return false; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should emit a warning when the font is registered already? I think right now it just returns false.
Bonus points if the warning message includes the name of the resource that registered the font
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
| // Allocate memory for font data that will be owned by LunaSVG | ||
| size_t dataSize = fontData.size(); | ||
| char* pFontData = new char[dataSize]; | ||
| memcpy(pFontData, fontData.data(), dataSize); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we can avoid the memcpy? e.g. FileLoad straight into a char* or cast SString to a char*?
I don't know what the recommended approach is here, but avoiding the memcpy seems preferable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileLoad uses SString which manages its own memory, so we can't pass this to LunaSVG for it to take ownership (and there is no way to 'release' the buffer provided by FileLoad)...
However, we can use the SharedUtil file functions for this to avoid the memcpy, which I've changed.
| std::vector<SString> fonts; | ||
| fonts.reserve(m_RegisteredFonts.size()); | ||
|
|
||
| for (const auto& pair : m_RegisteredFonts) | ||
| fonts.push_back(pair.first); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Non-blocking feedback) The code you have is good because it's very clear what it does. Apparently in C++20 there's now a two-liner for this, up to you whether you want to try it: https://stackoverflow.com/a/68094571/1517394
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented
|
|
||
| bool CSVGFontManager::IsFontRegistered(const SString& strFontFamily) const | ||
| { | ||
| return m_RegisteredFonts.find(strFontFamily) != m_RegisteredFonts.end(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this invalid?
| return m_RegisteredFonts.find(strFontFamily) != m_RegisteredFonts.end(); | |
| return m_RegisteredFonts.contains(strFontFamily); |
https://en.cppreference.com/w/cpp/container/map/contains.html
If it's valid, I think there's opportunity to use this function elsewhere in your PR too (RegisterFont)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed valid - updated!
| // LunaSVG takes ownership of the data and will call our destroy callback when done | ||
| if (!lunasvg_add_font_face_from_data(strFontFamily.c_str(), false, false, pFontData, dataSize, FontDataDestroyCallback, pFontData)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relationship between resource lifetime and fonts in m_RegisteredFonts are clear so far, but it looks like there could be some sort of desync between what's in m_RegisteredFonts and what's actually in lunasvg.
my questions are:
- In what scenario is
FontDataDestroyCallbackcalled? - Does the callback also need to remove the font from
m_RegisteredFonts? (I don't know if it's possible for lunasvg to eagerly unload a font?) UnregisterFont(on resource unload) doesn't communicate with lunasvg. Should it?
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into this further, it doesn't actually work how I thought. Once we register a font with lunasvg/plutovg, the reference count on that side will always remain above 0, and therefore our callback won't ever be fired (see plutovg_font_face_destroy)
There's no way to request removal of font faces from cache using the public API (i.e no such lunasvg_remove_font_face), the only font related public APIs are lunasvg_add_font_face_from_file and lunasvg_add_font_face_from_data. Therefore once registered, a font will remain in the lunasvg/plutovg cache until application shutdown.
We'll need to defer this PR until the public API for lifecycle of font faces is improved, particularly around explicit destruction (I'm considering opening a PR @ lunasvg repo to speed this up).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qaisjp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super cool and really well written, thanks. Some minor questions
| UnregisterFont(fontFamily); | ||
| } | ||
|
|
||
| std::vector<SString> CSVGFontManager::GetRegisteredFonts() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetRegisteredFonts is never called
Bumps LunaSVG from 3.2.0 to [0dd60d1](sammycage/lunasvg@0dd60d1). See [releases for LunaSVG](https://github.com/sammycage/lunasvg/releases) for changes in previous versions. Testing resource with presets/test cases: [svg.zip](https://github.com/user-attachments/files/24606373/svg.zip) Additional PR for custom font support #4620
…- PR #4615 See [releases for LunaSVG](https://github.com/sammycage/lunasvg/releases) for changes in previous versions. Testing resource with presets/test cases: [svg.zip](https://github.com/user-attachments/files/24606373/svg.zip) Additional PR for custom font support #4620
Makes use of newly added font support with a new Lua API
Fonts must be registered before attempting to createSvg referencing the font-family.
Fonts are cached in lunasvg for resource lifetime or until manually unregistered.
Loading existing system fonts is also supported (so can load a wider range of default system fonts now) - some discussion here: https://discord.com/channels/801330706252038164/801411628600000522/1458564145984176168
Note: LunaSVG already loads system fonts in our current version, but the latest version improves this
This was tested using a new SVG testing suite resource: svg.zip (say thanks to Claude Opus 4.5, he worked hard on this resource). Just run it and you'll see the UI with various testing presets and tools
All seems to be working
Based on #4615 - must be merged after that
Awaiting resolution of sammycage/lunasvg#258