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

Make hover animation work #6129

Merged
merged 4 commits into from
Mar 21, 2019
Merged

Make hover animation work #6129

merged 4 commits into from
Mar 21, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Mar 11, 2019

Element._model has to be cloned to Element._view when Element.transition(1) is called. Otherwise, when _model.* is set on hover, _view.* is also set to the same value, and an animation doesn't occur.

This PR makes makes hover animation work.

Master: https://jsfiddle.net/nagix/gL0sebrh/
This PR: https://jsfiddle.net/nagix/bjhp163w/

Fixes #5691

kurkle
kurkle previously approved these changes Mar 11, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

src/core/core.element.js Outdated Show resolved Hide resolved
@simonbrunel simonbrunel added this to the Version 2.9 milestone Mar 12, 2019
simonbrunel
simonbrunel previously approved these changes Mar 12, 2019
@simonbrunel
Copy link
Member

@nagix good catch. I think it's negligible but did you profile the performance hit of this final clone?

etimberg
etimberg previously approved these changes Mar 12, 2019
@etimberg
Copy link
Member

Looks great @nagix. Great catch!

@nagix
Copy link
Contributor Author

nagix commented Mar 13, 2019

This clone with 47 properties always finishes in less than 0.2ms and is called once per animation, so I don't think it impacts the performance.

@nagix
Copy link
Contributor Author

nagix commented Mar 13, 2019

...But let me check a chart with more data points as this could impact all the elements.

benmccann
benmccann previously approved these changes Mar 13, 2019
@nagix
Copy link
Contributor Author

nagix commented Mar 13, 2019

This is the draw(1) call duration at the end of an animation in the scatter chart with 50,000 points.

Browser Master (ms) This PR (ms) Change
Safari 12.0.3 198.903 470.219 +136%
Firefox 65.0.1 841.555 1195.500 +42%
Chrome 73.0.3683.75 314.002 399.518 +27%

@nagix
Copy link
Contributor Author

nagix commented Mar 13, 2019

The overhead of the clone was not small as expected. Another option would be to clone _model in setHoverStyle and removeHoverStyle. This works because _model.* is updated only in those functions while a new object is assigned to _model in other update calls, and this clones active elements only.

The downside is that it is not guaranteed that _view and _model are pointing to different objects, so this could cause similar issues when someone adds code that update _model properties.

@nagix
Copy link
Contributor Author

nagix commented Mar 14, 2019

The duration of draw(0.5) is added. It seems that the performance overhead comes from helpers.clone which is relatively heavy. If we can replace me._view = helpers.clone(model) with interpolate({}, view, model, 1), the duration of draw(1) drops to the same level as draw(0.5).

Browser draw(0.5) draw(1) Master draw(1) This PR
Safari 12.0.3 247.746 198.903 470.219
Firefox 65.0.1 880.300 841.555 1195.500
Chrome 73.0.3683.75 343.968 314.002 399.518

@nagix nagix dismissed stale reviews from benmccann, etimberg, simonbrunel, and kurkle via 570633c March 14, 2019 04:46
@nagix
Copy link
Contributor Author

nagix commented Mar 14, 2019

Now it just uses interpolate for the final transition with ease === 1 instead of helpers.clone.

@nagix
Copy link
Contributor Author

nagix commented Mar 14, 2019

Reverted the previous change as interpolate doesn't copy the properties that start with _ and one test failed.

@simonbrunel
Copy link
Member

Would helpers.extend(me._view, model) work and be faster?

@benmccann
Copy link
Contributor

Or would me._view = Object.assign({}, model); be any faster?

@simonbrunel
Copy link
Member

Object.assign() isn't compatible with IE.

@nagix
Copy link
Contributor Author

nagix commented Mar 15, 2019

Here is another run. me._view = helpers.extend({}, model) was faster than me._view = helper.clone(model).

Browser draw(0.5) draw(1) with = draw(1) with extend draw(1) withclone
Safari 12.0.3 247.746 198.903 287.740 470.219
Firefox 65.0.1 880.300 841.555 949.100 1195.500
Chrome 73.0.3683.75 343.968 314.002 350.298 399.518

As far as I know, _model currently does neither have nested properties nor support transition of them, me._view = helpers.extend({}, model) could be used in pivot as well (shallow vs deep copy). What do you think?

@benmccann
Copy link
Contributor

I'm guessing that helpers.extend could be optimized further. The current code is:

	extend: function(target) {
		var setFn = function(value, key) {
			target[key] = value;
		};
		for (var i = 1, ilen = arguments.length; i < ilen; ++i) {
			helpers.each(arguments[i], setFn);
		}
		return target;
	},

That loops over multiple inputs does a function call per key. I'm guessing it'd be much faster to do something like:

	shallowClone: function(source) {
		var target = {};
		var keys = Object.keys(loopable);
		var key;
		for (var i = 1, ilen = keys.length; i < ilen; i++) {
			key = keys[i];
			target[key] = source[key];
		}
		return target;
	},

Or potentially faster still:

	shallowClone: function(source) {
		var target = {};
		if (Object.assign) {
			Object.assign(target, source);
			return target;
		}
		var keys = Object.keys(loopable);
		var key;
		for (var i = 1, ilen = keys.length; i < ilen; i++) {
			key = keys[i];
			target[key] = source[key];
		}
		return target;
	},

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants