-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Performance optimizations when animations are disabled #6710
Conversation
var me = this; | ||
if (animationsDisabled) { | ||
me._view = me._model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this works, but would probably better belong in transition
. My understanding is that the main point of pivot
is to reset _start
and that the point of transition
is to set _view
(we should probably document the functions)
pivot
and transition
both have a big of junk in them right now that are mainly cause by Tooltip
not properly initializing these variables in the way that other elements do. I looked a little bit at trying to untangle some of it earlier, but didn't have the time. I don't think pivot
should be initializing _view
(why does it only do it once? what about subsequent updates) and I don't think transition should have to initialize _view
and _start
(_view
should probably be initialized in a constructor and _start
is already set in pivot
, so we should make sure Tooltip
calls pivot
like everything else does)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning to take a deeper look at improving the animation system, in the near future (when I have enough time). But it will be another PR, if I ever get it done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I wouldn't try to make any of the other changes in this PR. But maybe we should at least pass the animationsDisabled
flag to transition
in this PR so that we don't drift further from the original intentions of the methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so how about breaking it a little then? :)
pivot
should work the same, animations disabled or not.
transition
is not called when animations are disabled (for dataset elements, for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. the code is a lot cleaner now!
i'm not sure I understood what you meant about breaking animations
"Break the animations" commit really broke those (and was not easily fixed). So rebased and dropped that commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for this. this will be a big performance win
Another ~100ms off from uPlot benchmark.
@benmccann consider this kind of optimization while working with elements