Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't re-add existing styles to SVGShapeElement #2983

Closed
wants to merge 3 commits into from

Conversation

geomaster
Copy link
Contributor

When calling SVGShapeElement.reloadShapes(), searchShapes() will correctly reconcile modified and added shapes and reuse their elements. However, it will call setElementStyles(), which will add all style elements unconditionally to this.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 (via addEffect()) for e.g. a tr element 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).

@bodymovin
Copy link
Collaborator

hey! addEffect and relaodShapes are internal methods that shouldn't be called manually.
I'm not sure about the consequences of calling it manually.
Having said that, the lottie-api takes advantage of those methods to add dynamic properties, so it might be worth looking into why in this scenario things don't refresh.
If you have a simple example to reproduce, I'd love to take a look before merging this PR.

@geomaster
Copy link
Contributor Author

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 (type: 'gr').

Additionally, pure reloadShapes() doesn't actually do the trick by itself unless the effect callback changes the value it's returning after the reloadShapes() call.

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 reloadShapes() does duplicate style elements sometimes, but this looks like a bug in the handling of dynamic properties, or I'm doing something quite wrong...

By the way, we are simulating what lottie-api is doing in userland, but with some customizations to better suit to our data model and the operations we need to perform. This is why we're hitting the addEffect() APIs.

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`.
@geomaster
Copy link
Contributor Author

Closing in favor of #3039, since this PR was created to merge lottielab:master, but that master branch now in turn has additional fixes outside the scope of this PR.

@geomaster geomaster closed this Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants