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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Commits on Oct 31, 2023

  1. Don't re-add existing styles to SVGShapeElement 

    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.
    @geomaster
    geomaster committed Oct 31, 2023
    Configuration menu
    Copy the full SHA
    ca93f98 View commit details
    Browse the repository at this point in the history

  2. Fix ESLint error

    @geomaster
    geomaster committed Oct 31, 2023
    Configuration menu
    Copy the full SHA
    7615026 View commit details
    Browse the repository at this point in the history

  3. Ensure _localMatMdf is reset to false 

    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
    geomaster committed Oct 31, 2023
    Configuration menu
    Copy the full SHA
    5e6164b View commit details
    Browse the repository at this point in the history