feat(graphics-overhaul): graphics elements caching #3905
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughCentralizes entity-list change signaling into a single reportChange API, adds a per-element ElementConversionCache with content-hash and expression-aware graphics conversion, and threads cache/forceNewIds through preset/preview call sites while updating control/entity wiring to use the new change reporting. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
companion/lib/Controls/Entities/EntityList.ts (1)
212-219: Code structure is safe but could be clearer for maintainability.The method is protected in practice—
oldIndexalways comes fromfindParentAndIndex()which guarantees a valid index in [0, length−1], and the caller validates withif (!oldInfo) return false. ThenewIndex -= 1adjustment handles the removal correctly.However, clamping
newIndextothis.#entities.length(rather thanlength − 1) and relying on implicit assumptions about index validity makes the code fragile. Consider documenting that both indices are expected to be valid (or adding a validation guard) to make the contract explicit and prevent future bugs if this method is called differently.companion/lib/Graphics/ConvertGraphicsElements/Helper.ts (1)
216-277: Consider aligningcreateHelpersignature with actual usage.
ParseElementsContext.createHelperadvertisespropOverrides?: VariableValues, but the implementation ignores it. If overrides must be scoped viawithPropOverrides, dropping the param avoids accidental misuse. Based on learnings, this keeps composite override scoping explicit.companion/lib/Graphics/ConvertGraphicsElements/Util.ts (1)
14-46: Minor consideration: JSON.stringify key ordering for nested objectsThe top-level keys are sorted for determinism (line 20), which is great! However, when
typeof value === 'object'is true (line 33-35),JSON.stringify(value)is called on nested objects. JavaScript'sJSON.stringifydoesn't guarantee key ordering for nested objects by default.For most practical cases this should be fine since the same object structure will serialize the same way within a single runtime. But if you ever need cross-session cache key stability (e.g., for persistent caching), you might want to use a deterministic JSON serializer.
Not blocking - just something to keep in mind if cache behavior ever seems inconsistent! 🙂
companion/lib/Controls/ControlTypes/ExpressionVariable.ts (1)
132-142: Small copy-paste artifact in commentThe comment on line 137 says "Elements are not relevant for triggers", but this is the
ExpressionVariablecontrol, not a trigger. Looks like a copy-paste fromTrigger.ts(see lines 195-205 in that file from the relevant snippets).Not a big deal, but a quick fix would help future readers! 😊
📝 Suggested comment fix
`#entityListReportChange`(options: ControlEntityListChangeProps): void { if (!options.noSave) { this.commitChange(false) } - // Elements are not relevant for triggers + // Elements are not relevant for expression variables if (options.redraw) { this.triggerRedraw() } }companion/lib/Graphics/ConvertGraphicsElements.ts (1)
98-199: Well-structured caching logic inconvertElementsA few observations and a minor suggestion:
The cache-hit path is clean - check cache, use if present, otherwise compute and store.
The reference merging (lines 158-163) correctly aggregates
usedVariablesandcompositeElementsinto global references.Minor suggestion (line 162-163): The braces-less
ifwith the complex expression reads a bit dense. Consider adding braces for clarity:- if (cacheEntry.compositeElement?.elementId) - context.globalReferences.compositeElements.add(cacheEntry.compositeElement.elementId) + if (cacheEntry.compositeElement?.elementId) { + context.globalReferences.compositeElements.add(cacheEntry.compositeElement.elementId) + }This is purely a readability nit - feel free to keep as-is if you prefer the current style! 😊
Julusian
force-pushed
the
feat/graphics-element-caching
branch
from
ff81923 to
8dae3a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
companion/lib/Graphics/Controller.ts (1)
303-348: Cache key dropslocationwhen topbar is actually shown.
On Line 314–321 the condition removeslocationwhendecorationisFollowDefaultand topbar is enabled — the common case. That makes the cache ignore location precisely when topbar rendering is location-sensitive. Suggest computingshowTopBarand only deletinglocationwhen it’s false.🛠 Suggested fix
- if (canvasElement?.decoration !== ButtonGraphicsDecorationType.TopBar && globalShowTopBar) { - // Location is not needed in the cache key if topbar is not shown - delete cacheKeyObj.location - } + const showTopBar = + canvasElement?.decoration === ButtonGraphicsDecorationType.TopBar || globalShowTopBar + if (!showTopBar) { + // Location is not needed in the cache key if topbar is not shown + delete cacheKeyObj.location + }
🧹 Nitpick comments (2)
companion/lib/Graphics/ElementConversionCache.ts (1)
23-27: Minor documentation suggestion: "thread-safe" terminology.Since JavaScript is single-threaded, the "thread-safe" terminology in the comments (lines 26, 68-69, 100) might be slightly misleading. The pattern is really about batching invalidations to process them in a controlled manner, avoiding mid-operation inconsistencies.
Feel free to keep it as-is if it's clear enough in context, just wanted to mention it! 😊
companion/test/Graphics/ConvertGraphicsElements.test.ts (1)
1660-1727: Avoid hard‑coding the composite hash to reduce test brittleness.
Tying the test to an exact hash makes refactors to the hashing algorithm noisy. Consider asserting the prefix/suffix pattern instead.♻️ Suggested adjustment
- expect(groupElement.children[0].id).toBe( - `comp1-44136fa355b3678a1146ad16f7e8649e94fb4fc21fe77e8310c060f61caaff8a/inner1` - ) + expect(groupElement.children[0].id).toMatch(/^comp1-[^/]+\/inner1$/)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webui/src/Buttons/EditButton/LayeredButtonEditor/Preview/LayeredButtonPreviewRenderer.tsx (1)
164-166: Optional: hoist the cache size to a named constant for easier tuning.
Keeps the knob visible and makes later adjustments more straightforward.♻️ Suggested tweak
const PAD_X = 10 const PAD_Y = 10 +const TEXT_LAYOUT_CACHE_SIZE = 200 @@ - const textLayoutCache: TextLayoutCache = new QuickLRU({ maxSize: 200 }) + const textLayoutCache: TextLayoutCache = new QuickLRU({ maxSize: TEXT_LAYOUT_CACHE_SIZE })
This adds a layer of caching to the new graphics rendering.
This layer is intended to avoid re-executing stable expressions, and to reduce the cost of computing a cache key for the graphics properties.
Because of the unlimited number of image elements, the json drawing object could easily reach multiple mb, making it a little costly to stringify and hash as a cache key.
Instead, this is computing each element separately (minus the children) and computing a hash for each element whenever it changes. The overall hash is then computed from these individual hashes.
No attempt is made at this stage to add any caching to the drawing process, that could be a follow up if there appears to be a benefit to doing so.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.