Don't re-add existing styles to SVGShapeElement #2983
Conversation
|
hey! addEffect and relaodShapes are internal methods that shouldn't be called manually. |
|
Hey, thanks for the reply! I did some investigation and there is definitely something strange going on. It seems that not all dynamic properties are affected, as I can reproduce only with transform elements inside a group ( Additionally, pure Please take a look at this repro - hopefully it'll be clearer what I'm trying to say: https://gist.github.com/geomaster/d1ca626f3467b539181df6c043a9aa63 I still think this PR is valid, as By the way, we are simulating what |
When calling `SVGShapeElement.reloadShapes()`, `searchShapes()` will correctly reconciliate modified and added shapes and reuse their elements. However, it will call `setElementStyles()`, which will add all styles unconditionally to `this.stylesList`, even if the styles were already in there. Modify `setElementStyles()` to check for the existence of a particular style before adding it.
With efeb109, support for transform effects has been added, which required introducing another matrix on `TransformElement` to represent the final transform matrix including the effects. This matrix is denoted as `localMat` If there are no transform effects, `localMat` is the same (referentially) as the existing transform matrix (`mat`). Otherwise, it's calculated by composing the transform effects and then composing that with the "old" transform matrix. Alongside `localMat`, a `_localMatMdf` member is introduced, which tracks whether the local matrix was modified since its last commit to the DOM, analogously to `_matMdf`. Unfortunately, a logic bug introduced by efeb109 meant that, in the absence of transform effects, `_localMatMdf` was latched to `true`, i.e. no code path would ever be able to reset it to `false` after it was once set to `true` (e.g. as a result of the matrix being animated). This causes a serious performance regression, since all matrices that once had `_matMdf` set to `true` will keep being updated in the DOM every frame, causing costly full style recalculations. Fix by forcing `_matMdf` and `_localMatMdf` to be equivalent in the case of no transform effects, just as `mat` and `localMat` are referentially equal. In the case of >0 transform effects, the custom logic in `renderLocalTransform()` will take over and compute `_localMatMdf` separately from and based on `_matMdf`.
|
Closing in favor of #3039, since this PR was created to merge |
When calling
SVGShapeElement.reloadShapes(),searchShapes()will correctly reconcile modified and added shapes and reuse their elements. However, it will callsetElementStyles(), which will add all style elements unconditionally tothis.stylesList, even if the styles were already in there, making it grow indefinitely and causing other problems down the line.Modify
setElementStyles()to check for the existence of a particular style before adding it.On a semi-related note, I'm calling
reloadShapes()because dynamic properties (viaaddEffect()) for e.g. atrelement in a shape group (gr) will not refresh otherwise. If this is not intended, there is possibly a deeper issue (I can provide a repro if that's the case).