LunaSVG: add custom font support#4620
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.
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
| // 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.
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.
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.
(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
|
|
||
| bool CSVGFontManager::IsFontRegistered(const SString& strFontFamily) const | ||
| { | ||
| return m_RegisteredFonts.find(strFontFamily) != m_RegisteredFonts.end(); |
There was a problem hiding this comment.
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.
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.
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.
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).
qaisjp
left a comment
There was a problem hiding this comment.
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.
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