feat!: Remove integralSize property from class Svg, enforcing 'always true' behaviour - #3969
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the Svg.integralSize API from flame_svg and makes Svg always use an “integral” cache-key strategy, aiming to prevent cache thrashing caused by floating-point rounding when rendering SVGs at varying scales/zooms.
Changes:
- Removed the
integralSizeconstructor/loader parameter and the corresponding public getter/setter fromSvg. - Updated
Svg.load,Svg.loadFromString, andGame.loadSvgto no longer accept/forwardintegralSize. - Simplified
_getImageto always build the cache key from ceiled dimensions (now unconditional).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Image _getImage(Size size, double widthRatio, double heightRatio) { | ||
| final width = size.width * widthRatio; | ||
| final height = size.height * heightRatio; | ||
| Size cacheKey; | ||
| if (fixedRatio || integralSize) { | ||
| cacheKey = Size(width.ceilToDouble(), height.ceilToDouble()); | ||
| } else { | ||
| cacheKey = Size(width, height); | ||
| } | ||
| final cacheKey = Size(width.ceilToDouble(), height.ceilToDouble()); | ||
| final image = _imageCache.getValue(cacheKey); |
| /// Creates an [Svg] with the received [pictureInfo]. | ||
| /// Default [pixelRatio] is the device pixel ratio. | ||
| /// Setting [integralSize] to `true` uses integer dimensions for cache keys. | ||
| /// Setting [fixedRatio] to `true` ensures the cache uses a single entry. | ||
| /// Default [cacheSize] is [defaultCacheSize], which is 10 as previously; |
Thank you. :-) |
@spydon — regarding this one, I've updated the dartdoc. This one, while obviously valid, as expected costs quite a bit... the testbed shows ~35% increased cache usage (~170 instead of ~110 entries): after all, Running image diffs on a few rendering results hasn't revealed any rasterisation improvements; the deltas all appear to be < 1%, and they mostly occur in the alpha channel. Anyways, the cache changes would formally align the keys with the contents' dimensions, so... please let me know whether I should add them to the PR. (From a 'field usage' POV, the suggested approach would be what the testbed does: when |
|
@adario hmm, I'll let you decided, both are fine for me. |
Thanks again: I think we could merge the PR, and revisit the Copilot changes at a later time... I've stashed them just in case. |
Description
The
Svg.integralSizeproperty was added in PR #3956: whentrue, it ensures that the keys forSvg._imageCacheare always integer-sized. The property, however, would never be useful with its current default offalse, since this would re-introduce floating-point rounding errors in the image cache keys.Resolution
Remove the
integralSizeproperty, always following itstruecodepath.Testing
An updated testbed for the changes is available — this displays 200+1 SvgComponent objects, all with different rotation/scale values; it now features the Svg button, which cycles through a few SVG assets.
For comparison purposes, the testbed starts with the default implementation; the Mode and Size buttons cycle through several combinations of the
fixedRatioandcacheSizeproperties..Checklist
docsand added dartdoc comments with///.examplesordocs.Breaking Change?
Migration instructions
This PR does change the internal caching behaviour in class
Svg, for all rendering operations. However, any existing use cases of classSvgshould build as previously , since the property changes from PR #3956 haven't been published yet. All parameters/properties from PR #3956 have default values, so no build should break.At runtime, the PR should not break class
Svgclients either: the actualImageobjects in theMemoryCacheshould maintain the previous dimensions and contents, since these were already integral prior to PR #3956. A 3-way comparison for themelos test --scope=flame_svgresults reveals no differences (all tests fail in the same way)...I apologise if I've missed something, but at present I wouldn't know what migration steps to provide.
Related Issues