Enable animation on inner nodes of the element and options

[stockiNail]

Fix #11172

[stockiNail]

@kurkle I have tried to implement the animation on properties of inner objects in element or options.

This should address the following #11129 (comment)

[stockiNail]

Set as draft to improve some checks

[stockiNail]

Set ready for review

[kurkle]

This should be checked for performance regressions with large datasets

[stockiNail]

This should be checked for performance regressions with large datasets

@kurkle Good point. What do you suggest to address it? A test case with many elements as fixture or as specs?

[stockiNail]

This should be checked for performance regressions with large datasets

Just for info. I did a test, locally, with and without this PR, using a scatter chart with 10000 points. I measured the time from beforeUpdate to afterRender, and both cases lasted about 2200ms.
It seems new code is not affecting the current performance. Of course this PR is not affecting the animation stuff because the Animation class is working as before. I'll do other tests.

[kurkle]

I'd try to find the breakking points, it used to be somewhere around million points on my old machine, but single dataset could not handle 500M points (if my memory serves). Anyway, with extreme numbers, possible regressions are easier to spot.

Having this in CI is not trivial, because performance of the runnen varies greatly. Would need to run both, master and pr benchmarks in the same runner to get a hint of the performance impact of a pull request.

For me the unknowns are: how to checkout and build both branches and how to report the results.

Also the benchmarks itself require a good amount of consideration for not taking too long but still having good results.

[stockiNail]

@kurkle I'll do additional tests (if I have time today). I'm not an expert on JS, you know, but I'm trying to use my knowledge on other languages to understand better where we could have a bottleneck based on the amount of data elements.

To be honest, the first implementation I did locally, I was using a Map (and a different flow) instead of an array but, having to check the start name of the property during the scan of the values, I preferred to use an array.

In the Animations constructor (and more in the configure method) I don't see any issue related to the amount of elements because if the properties are path based or not, I don't see any big computation (or many new objects, one for each path property).

Instead, in _createAnimations method, I see a possible issue based on the amount of data elements (even if path properties are not used).

      const matched = pathAnimatedProps.filter(item => item.prop.startsWith(prop));
      if (matched.length) {

With this code, for every animated property (also the normal ones) it returns an "array" (maybe new one, always empty if there is not any path property).
Therefore, for instance, having 100K data elements and 4 animated properties (not path ones, implemented by this PR), passing thru that code we are creating useless 400K "array".

To avoid it, I can change the PR going back to my first local impl where I had 2 cycles, 1 for path props and 1 for normal ones and where those "array" were not created. And I was using Map which should be better performed accessing by an indexed key.

@kurkle what do you think?

[stockiNail]

@kurkle have a look now. I did some checks and indeed Map is really more performed. The time spent to check if the animation property is flat one or not, is almost 0 (in my test with 10K). Now I try with more items (try to avoid to crash my FF).

[stockiNail]

Test with 100K data elements (with the same conditions, FF opened after clearing cache).

Branch	Elapsed rendering time
master	11207 ms
PR 11173	11552 ms

[stockiNail]

rebased

[kurkle]

Looks like each "path" prop would be managed multiple times if there are multiple "path" props?

I think most of the new helpers could be omitted(?):

Suggested change

	if (pathAnimatedProps.has(prop)) {
	pathAnimatedProps.forEach(function(item) {
	const newTarget = getInnerObject(target, item);
	const newValues = getInnerObject(values, item);
	if (newTarget && newValues) {
	manageItem(newTarget, newValues, item.prop, item.key);
	}
	});
	} else {
	manageItem(target, values, prop);
	}
	if (prop.includes('.')) {
	const newTarget = resolveObjectKey(target, prop);
	const newValues = newTarget && resolveObjectKey(values, prop);
	if (newValues) {
	manageItem(newTarget, newValues, prop, prop.split('.').pop());
	}
	} else {
	manageItem(target, values, prop);
	}

Did I miss something?

[stockiNail]

@kurkle I think the IF statement, you suggested, is wrong because the "prop" is not an item of "properties" animation configuration but one of the keys of the config object, therefore it will never have a dot.
The for cycle is on the config properties and not on the animation config one. That's why I used a map, reading the anim config, where the K is the fist part of the path (to check with the prop) and the V the whole path (an object with prop and whole path as array).

EDIT: I have merged your suggestion anyway because I didn't recall all logic of this PR... 4 months is to big period for my memory, getting old. 😃

[stockiNail]

Furthermore, I cannot use resolveObjectKey helper because this function is returning the value of the key and not the object where the key belongs to and therefore I cannot reuse the manageItem function for both normal and path anim options, where the function needs the object with key.

[stockiNail]

@kurkle I have improved a bit removing the for Each statemenet. Anyway forEach should be needed. Need to time to review. Apologize

[stockiNail]

@kurkle now I recall why we need to scan the map but was wrong. Now I fixed.

The map has:

K = string = the first item of the path (i.e. 'font' if animation properties config has 'font.size')
V = array = the array of objects with the animation properties config

The V is an array because you can configure more than 1 anim prop as path (i.e. ['font.size', 'font.weight'])