Defer cost of OSD tab font glyph render from tab initialization to time-of-use (cuts tab load time to roughly 1/3 of previous). #4495
Conversation
|
""" WalkthroughThe changes modify how and when font character images are generated and previewed. The drawing of font characters is now explicitly triggered during preview rendering or modal creation, rather than automatically on every character addition or font load. This centralizes and defers image generation to specific points in the workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal
participant FONT
participant LogoManager
User->>Modal: Open Font Manager modal
Modal->>FONT: onCreated callback
FONT->>FONT: preview()
loop For each character index
FONT->>FONT: draw(i)
end
FONT->>LogoManager: drawPreview()
loop For each character index
LogoManager->>LogoManager: font.draw(i)
end
Possibly related issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Fixes #4494 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/LogoManager.js (1)
293-294: LGTM! Good performance optimization with minor suggestions.The addition of
this.font.draw(i)to ensure character image URLs are generated on-demand aligns well with the PR objective to defer expensive rendering operations. This should effectively reduce initial tab load time.Consider these minor enhancements for robustness:
LogoManager.drawPreview = function () { const $el = this.elements.$preview.empty(); for (let i = this.logoStartIndex, I = this.font.constants.MAX_CHAR_COUNT; i < I; i++) { - // Call this.font.draw to force the URL to be generated if it hasn't already - this.font.draw(i); + // Call this.font.draw to force the URL to be generated if it hasn't already + if (this.font && typeof this.font.draw === 'function') { + this.font.draw(i); + } const url = this.font.data.character_image_urls[i]; + if (!url) { + console.warn(`Character image URL not available for index ${i}`); + continue; + } $el.append(`<img src="${url}" title="0x${i.toString(16)}"></img>`); } };This adds defensive checks for the font object and handles cases where URL generation might fail.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/LogoManager.js(1 hunks)src/js/tabs/osd.js(2 hunks)
🔇 Additional comments (2)
src/js/tabs/osd.js (2)
264-265: LGTM! Excellent performance optimization.The explicit call to
FONT.draw(i)ensures character image URLs are generated on-demand during preview rendering rather than upfront during font loading. This leverages the existing caching mechanism inFONT.draw(lines 226-230) which checks for cached URLs before generating new ones.This change should significantly reduce initial font loading time while maintaining the same functionality.
773-776: Good centralization of preview rendering to modal creation.Moving the
FONT.preview()andLogoManager.drawPreview()calls to the modal'sonCreatedcallback is a smart approach. This ensures:
- Font preview rendering only happens when the modal is actually opened
- Logo preview is also deferred until needed
- Both operations are centralized to a single event
This aligns perfectly with the PR objective of deferring expensive rendering operations to time-of-use rather than initialization.
|
Note that OSD tab load is faster, BUT the first load of the Font Manager is slower since it has to render all the glyphs then. |
|
FYI, my confidence level here is low to medium because of the lack of encapsulation around the font URL data. If people want to move forward with this PR, I would probably want to gate access behind a helper function that performs the extra draw operation. I also don't like the new delay opening font manager because it seems like nothing is happening for a little bit after clicking the button. (The current delay opening the OSD tab is at least clear that the tab is loading.) |
|
|
What I think I just realized is that the draw function IS the abstraction and everything should use the return value of that instead of accessing the array. That should simplify this PR. Let me do that and then you can try it and maybe decide if it really needs a loading message or not. |
…me-of-use (cuts tab load time to roughly 1/3 of previous).
|
|
Preview URL: https://36529588.betaflight-configurator.pages.dev |
|
@haslinghuis, please have a look at the latest commit. I feel better about it mostly, though I wish there were a little less duplication. If you are happy with this, I think it makes sense to merge regardless of whether the SVG idea goes anywhere. |
|
Well, I guess I still want to know if the SVG idea has potential before committing this PR. My change right now still feels a little fragile and I'm worried I may have missed a scenario. |
| @@ -3499,15 +3502,20 @@ osd.initialize = function (callback) { | |||
| // init structs once, also clears current font | |||
| FONT.initData(); | |||
|
|
|||
| let fontPreviewNeedsReload = false; | |||
There was a problem hiding this comment.
This is a hack as onCreated already did a run here ?
Reverting onCreated and fontPreviewNeedsReload the OSD tab loading time is as on master so there is definite an improvement.
Feel there is still some duplication - are we not draw() - ing two times now in LogoManager.drawPreview() and FONT.preview() ?
There was a problem hiding this comment.
onCreated runs when the Font Manager dialog opens - not as part of the opening of the OSD tab. Not sure that answers your question. But I agree it feels like this could still be simpler.
|
@haslinghuis, I've posted a PR for the SVG change and I feel better about it for almost the same benefit: #4497. This change could ALSO be applied, but I'm worried there may be subtleties that haven't been uncovered yet. That change seems easier to validate. |
I don't love this, but it's a sketch of one approach that seems to work.
Summary by CodeRabbit
Bug Fixes
Refactor