Resolve FontSources on changes #24362
Conversation
…to the font assets. When FontSource resolution failed because an asset wasn't yet loaded, `update_editable_text_styles` didn't resolve the FontSource again the next frame.
| } | ||
|
|
||
| if text_font.is_changed() { | ||
| if text_font.is_changed() || fonts.is_changed() { |
There was a problem hiding this comment.
This means that whenever any font asset is added/removed/updated this will update the style of every EditableText (and likely trigger a bunch of work? ... I don't know too much about the editing stuff). At the very least this would trigger a "relayout" for every editable text. That doesn't seem like an efficient way to go about this.
It seems like attacking this from a different angle would be better. Ex: we know when text_font has changed, and we can also know if the font family couldn't be resolved on a given frame due to an asset not being present. We can track EditableText entities whose fonts haven't loaded yet and either listen for those asset load events, or poll for load state.
There was a problem hiding this comment.
Yep it's not ideal, but the whole way we manage fonts as assets is broken atm (#22415, needs to be updated for Parley but most of the same problems remain).
We could add some more complex update logic I suppose, but if you think about this practically:
- font asset updates are rare
- there won't be many text inputs entities alive at once
- if there are it'll almost always be something like a transform widget with multiple fields
- short numeric values will be extremely cheap to relayout
- and all the fields will use the same font
There was a problem hiding this comment.
I don't think any of those things are things we should bank on. I don't think the added complexity of checking and waiting merits being O(n) slow in those cases.
There was a problem hiding this comment.
but the whole way we manage fonts as assets is broken atm
Any text rendering system that can't accommodate loading and unloading fonts at arbitrary times (via something like the Bevy asset system) probably needs some work. Someone should be able to do things like "build a web browser" in bevy that doesn't leak each time it loads a website.
There was a problem hiding this comment.
Someone should be able to do things like "build a web browser" in bevy that doesn't leak each time it loads a website.
FWIW (as someone who is actually building a web browser - not on Bevy but on Parley), the expected mechanism for this in Parley would currently be to create a new FontCollection and drop the old one: system fonts would then be retained but custom fonts would be reset. There is also an unregister font API but in actual webpage use case it would be very difficult to avoid webpages clobbering each other (or accidentally matching against each others' fonts) with such an API.
There are definitely some rough edges around this now. Notably that the only way to avoid reloading system fonts from scratch is to clone an existing FontCollection. So we'd currently recommend retaining a global "blank" FontCollection with only system fonts that you can use as a source to clone new FontCollections from. The plan is probably to move towards storing a static Arc<Mutex<SystemFonts>> or similar internally in Parley to avoid this problem.
I don't think any of those things are things we should bank on. I don't think the added complexity of checking and waiting merits being O(n) slow in those cases.
(I am currently doing this - it's not ideal for me either, but I haven't found it to be a huge problem in practice)
Medium/long term I'm hoping to support having "lazy-loaded fonts" in the font database where the metadata is loaded but the font itself isn't (help welcome). I think that might make it easier to track exactly what is impacted by loading a font.
(on the web this corresponds to an @font-face declaration where the actual font file hasn't been loaded yet).
update_editable_text_styles, reresolve FontSources on changes …FontSources on changes
…he loaded collection, instead of the asset path. Store the changed family ids and asset paths, and set the any TextFonts that refer to them as changed.
…ditable_text_styles`. Only mutably access the editor when updates need to be made.
asset changes in `update_editable_text_content_size`. The `text_font.is_changed()` is sufficient by itself as `load_font_assets_into_font_collection` sets `TextFont` components to changed when new font assets can affect font resolution.
|
The CI failure doesn't seem like it's related to this PR. |
|
@ickshonpe have you addressed @cart's critique in #24362 (comment) ? |
Objective
When
FontSourceresolution fails because an asset isn't yet loaded, it doesn't attempt to reresolve theFontSourceagain the next frame.Fixes #24356
Solution
load_font_assets_into_font_collectionset the family name from the loaded font collection, instead of the asset path.TextFonts that refer to them changed.update_editable_text_content_sizeon font asset changes. Instead rely on the narrowerTextFontchange detection, which is sufficient now becauseload_font_assets_into_font_collectionmarks affectedTextFontcomponents as changed when newly loaded font assets can affect font resolution.EditableTextat the start ofupdate_editable_text_styles. The editor is only mutable accessed if updates to the styles need to be made.Testing