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

Enable data decimation plugins by separating parsed data into two arrays #8251

Closed
wants to merge 1 commit into from
Closed

Enable data decimation plugins by separating parsed data into two arrays #8251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/controllers/controller.doughnut.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export default class DoughnutController extends DatasetController {
const meta = this._cachedMeta;
let i, ilen;
for (i = start, ilen = start + count; i < ilen; ++i) {
meta._parsed[i] = +data[i];
meta._parsedRaw[i] = meta._parsed[i] = +data[i];
Copy link
Member

Choose a reason for hiding this comment

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

should do only one array and set the other to same instance after loop. we should make sure _parsedRaw is sufficient sized before parsing, so should set to that.

}
}

Expand Down
13 changes: 12 additions & 1 deletion src/core/core.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,11 +471,21 @@ class Chart {

// Make sure all dataset controllers have correct meta data counts
for (i = 0, ilen = me.data.datasets.length; i < ilen; i++) {
me.getDatasetMeta(i).controller.buildOrUpdateElements();
const {controller} = me.getDatasetMeta(i);
controller.parse(0, me.data.datasets[i].data.length);
}

me._updateLayout();

for (i = 0, ilen = me.data.datasets.length; i < ilen; i++) {
// After parse notification takes place after the layout pass
// to ensure that decimation plugins can take advantage of the
// scale dimensions
const controller = me.getDatasetMeta(i).controller;
controller._notifyAfterParse();
Copy link
Contributor

@benmccann benmccann Dec 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

should we call this _notifyAfterLayout?

Edit: I see now why this was called _notifyAfterParse. I think it could be called _notifyAfterLayout if we were not setting data attribute in the args per the suggestion in https://github.com/chartjs/Chart.js/pull/8251/files#r550253989

controller.buildOrUpdateElements();
}

// Only reset the controllers if we have animations
if (!animsDisabled) {
// Can only reset the new controllers after the scales have been updated
Expand Down Expand Up @@ -741,6 +751,7 @@ class Chart {
index: datasetIndex,
_dataset: dataset,
_parsed: [],
_parsedRaw: [],
_sorted: false
};
}
Expand Down
55 changes: 40 additions & 15 deletions src/core/core.datasetController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Animations from './core.animations';
import defaults from './core.defaults';
import {isObject, merge, _merger, isArray, valueOrDefault, mergeIf, resolveObjectKey, _capitalize} from '../helpers/helpers.core';
import {isObject, merge, _merger, isArray, valueOrDefault, mergeIf, resolveObjectKey, _capitalize, isNullOrUndef} from '../helpers/helpers.core';
import {listenArrayEvents, unlistenArrayEvents} from '../helpers/helpers.collection';
import {resolve} from '../helpers/helpers.options';
import {getHoverColor} from '../helpers/helpers.color';
Expand Down Expand Up @@ -346,18 +346,41 @@ export default class DatasetController {

me._dataCheck();

const data = me._data;
const metaData = meta.data = new Array(data.length);
// const data = me._data;
// const metaData = meta.data = new Array(data.length);

for (let i = 0, ilen = data.length; i < ilen; ++i) {
metaData[i] = new me.dataElementType();
}
// for (let i = 0, ilen = data.length; i < ilen; ++i) {
// metaData[i] = new me.dataElementType();
// }

if (me.datasetElementType) {
meta.dataset = new me.datasetElementType();
}
}

_notifyAfterParse() {
const me = this;
const {_cachedMeta: meta} = me;
const {iScale} = meta;

// We expect afterDataParse hooks to set the modified data
Copy link
Contributor

@benmccann benmccann Dec 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it'd be cleaner to always set meta._parsed = meta._parsedRaw and then have the plugin override meta._parsed. We could expose a setParsed method if that's preferable to setting the value of private _parsed variable. We might have to pass chart as an arg so that we can get the meta

// onto the args object under the data key
// If that key is not set by a plugin, then the parsed data is set to
// the raw parsed data
const args = {
indexScale: iScale,
parsed: meta._parsedRaw,
previousParsed: meta._parsed,
};
me.chart.notifyPlugins('afterDataParse', args);

if (!isNullOrUndef(args.data)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd reverse the cases to avoid the !

meta._parsed = args.data;
} else {
meta._parsed = meta._parsedRaw;
}
}

buildOrUpdateElements() {
const me = this;
const meta = me._cachedMeta;
Expand Down Expand Up @@ -421,11 +444,11 @@ export default class DatasetController {

if (start > 0) {
sorted = meta._sorted;
prev = meta._parsed[start - 1];
prev = meta._parsedRaw[start - 1];
}

if (me._parsing === false) {
meta._parsed = data;
meta._parsedRaw = data;
meta._sorted = true;
} else {
if (isArray(data[start])) {
Expand All @@ -438,7 +461,7 @@ export default class DatasetController {

const isNotInOrderComparedToPrev = () => isNaN(cur[iAxis]) || (prev && cur[iAxis] < prev[iAxis]);
for (i = 0; i < count; ++i) {
meta._parsed[i + start] = cur = parsed[i];
meta._parsedRaw[i + start] = cur = parsed[i];
if (sorted) {
if (isNotInOrderComparedToPrev()) {
sorted = false;
Expand Down Expand Up @@ -591,7 +614,8 @@ export default class DatasetController {
getMinMax(scale, canStack) {
const me = this;
const meta = me._cachedMeta;
const _parsed = meta._parsed;
// Use filtered parsed elements if the exist, otherwise the full elements
Copy link
Member

Choose a reason for hiding this comment

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

isn't the correct array always available in the meta._parsed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm storing the full array in meta._parsedRaw and the filtered one in meta._parsed

Copy link
Member

Choose a reason for hiding this comment

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

if there is not filtered array, meta._parsed === meta._parsedRaw?

const _parsed = meta._parsed && meta._parsed.length ? meta._parsed : meta._parsedRaw;
const sorted = meta._sorted && scale === meta.iScale;
const ilen = _parsed.length;
const otherScale = me._getOtherScale(scale);
Expand Down Expand Up @@ -631,7 +655,8 @@ export default class DatasetController {
}

getAllParsedValues(scale) {
const parsed = this._cachedMeta._parsed;
const {_parsed, _parsedRaw} = this._cachedMeta;
const parsed = _parsed && _parsed.length ? _parsed : _parsedRaw;
const values = [];
let i, ilen, value;

Expand Down Expand Up @@ -971,15 +996,15 @@ export default class DatasetController {
_resyncElements() {
const me = this;
const numMeta = me._cachedMeta.data.length;
const numData = me._data.length;
const numData = me._cachedMeta._parsed.length;

if (numData > numMeta) {
me._insertElements(numMeta, numData - numMeta);
} else if (numData < numMeta) {
me._removeElements(numData, numMeta - numData);
}
// Re-parse the old elements (new elements are parsed in _insertElements)
me.parse(0, Math.min(numData, numMeta));
// me.parse(0, Math.min(numData, numMeta));
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to do the parsing first and resync the element counts after that. So compare _data.length to _parsedRaw.length and if that differs parse. then compare _parsed.length to numMeta and add the elements. need to remove the parsing from the _insertElements.

}

/**
Expand All @@ -998,7 +1023,7 @@ export default class DatasetController {
data.splice(start, 0, ...elements);

if (me._parsing) {
meta._parsed.splice(start, 0, ...new Array(count));
meta._parsedRaw.splice(start, 0, ...new Array(count));
Copy link
Member

Choose a reason for hiding this comment

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

can't do the parsing here at all, because start and count refer to elements, no to the raw data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm realizing that this is causing a lot of tests to fail. A lot of code assumes these can be updated in sync at the same time.

I wonder if there is a simpler way to achieve this without touching the parsing, maybe even just a good guess at the size for estimating the number of points to keep

}
me.parse(start, count);

Expand All @@ -1014,7 +1039,7 @@ export default class DatasetController {
const me = this;
const meta = me._cachedMeta;
if (me._parsing) {
const removed = meta._parsed.splice(start, count);
const removed = meta._parsedRaw.splice(start, count);
Copy link
Member

Choose a reason for hiding this comment

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

this might work, but against _parsed

if (meta._stacked) {
clearStacks(meta, removed);
}
Expand Down