Improve offset calculation for scale.offset option #4658

nagix · 2017-08-15T03:59:18Z

The offsets of time scales are currently calculated based on tick intervals, but it causes too much margin or short of margin depending on scale configuration or datasets. The proposed code calculates the offsets based on timestamps of labels and data. It also fixes offset calculation in case that a chart has a single data point.

Current version: https://jsfiddle.net/q4fo65aa/
Proposed version: https://jsfiddle.net/sfhwd1by/

Fixes #5312

etimberg · 2017-08-15T11:18:18Z

src/scales/scale.time.js

+
+	if (options.offset) {
+		[ts.labels, ts.data].forEach(function(data) {
+			if (!timestamps.length) {


Will this work as expected if ts.data and ts.labels both have items?

It looks what will happen is that the first iteration of the forEach will run and timestamps.length will be 0 so the items in ts.labels are considered. However, the items in ts.data will be skipped because on the second iteration, timestamps.length may be non-zero due to the first iteration of the loop.

If this is as intended, I think there should be a comment explaining why. I can see it accidentally getting lost/changed later

This is as intended. First, it processes ts.labels to pick up timestamps of labels, which are primarily used for offset calculation because bars are usually on labels. But if there are no labels, it should add offsets based on ts.data because we support bars with {x, y} data points as well. That is the second path.

I will add a comment on it.

etimberg · 2017-08-15T11:22:16Z

src/scales/scale.time.js

+		if (length) {
+			divisor = length > 1 ? 2 : 1;
+			if (!options.time.min) {
+				upper = length > 1 ? interpolate(table, 'time', timestamps[1], 'pos') : 1;


is timestamps guaranteed to be sorted here?

Yes, it is deduped and sorted at

Chart.js/src/scales/scale.time.js

Line 515 in b9afeaf

labels = arrayUnique(labels).sort(sorter);

and

Chart.js/src/scales/scale.time.js

Line 521 in b9afeaf

timestamps = arrayUnique(timestamps).sort(sorter);

simonbrunel · 2017-08-25T20:01:59Z

src/scales/scale.time.js

+		// primarily used for offset calculation because bars are usually on labels.
+		// But if there are no labels, it should add offsets based on `ts.data` because
+		// we support bars with {x, y} data points as well. That is the second path.
+		[ts.labels, ts.data].forEach(function(data) {


Would ts.data be enough since most of the time it contains ts.labels?

@simonbrunel I was considering the case of a mixed chart with bars on labels and line with {x, y} points. If the label positions and {x, y} points are not aligned on a timeline, bar width calculation could go wrong. But, as you say, that may be an edge case, and I agree that the calculation based on ts.data works in most cases.

nagix · 2017-08-29T02:13:27Z

Removed check for label values, and now it only uses scale._timestamps.data for offset calculation.

benmccann · 2017-10-07T18:17:42Z

@nagix the fiddles in the description don't seem to be working for me

benmccann · 2017-10-31T23:39:37Z

@nagix could you fix the fiddles so that we can review this PR? They're giving me the error below:

Refused to execute script from 'https://raw.githubusercontent.com/chartjs/chartjs.github.io/master/dist/master/Chart.js' because its MIME type ('text/plain') is not executable, and strict MIME type checking is enabled.

benmccann · 2017-11-08T05:59:15Z

@nagix just a reminder to fix the fiddles when you get a chance. I'd love to get this change merged

benmccann · 2017-11-23T16:50:28Z

@nagix have you abandoned this PR?

I'd would really like to see it get checked assuming it fixes an issue, but I can't tell what the issue is since the fiddles are broken

I may close this PR due to inactivity if you're no longer interested in seeing it merged

etimberg · 2017-12-31T15:02:18Z

Closing due to inactivity

nagix · 2018-03-02T03:05:54Z

@benmccann @etimberg @simonbrunel Sorry for not responding earlier. I fixed the fiddles and confirmed that the problems still remain. Can you reopen this if possible?

- Offset is calulated based on the intervals between the first two data points and the last two data points (barThickness: 'flex') or the minimum interval of all data (barThickness: undefined). - Add test for both cases of barThickness: 'flex' and undefined.

nagix · 2018-06-29T14:29:29Z

As equally sized bars were introduced in 2.7.2, I made some changes to support it.

Offsets are calulated based on the intervals between the first two data points and the last two data points (barThickness: 'flex') or the minimum interval of all data (barThickness: undefined).
Add test for both cases of barThickness: 'flex' and undefined.

@benmccann @etimberg @simonbrunel Can you review this again?

benmccann · 2018-08-04T17:08:00Z

Documentation for these features added in 119a86f and awaiting deployment

benmccann · 2018-08-04T17:30:35Z

src/controllers/controller.bar.js

@@ -164,7 +164,7 @@ function computeFlexCategoryTraits(index, ruler, options) {
 	if (prev === null) {
 		// first data: its size is double based on the next point or,
 		// if it's also the last data, we use the scale end extremity.
-		prev = curr - (next === null ? ruler.end - curr : next - curr);
+		prev = curr - (next === null ? Math.max(curr - ruler.start, ruler.end - curr) * 2 : next - curr);


This is getting a bit tricky to follow. I feel like we're trying to hack prev and next to get the desired results for start and size. I wonder if it would be easier if we just computed those directly like:

if (prev == null && next == null) { start = ruler.start * (1 - percent / 2); size = ruler.end - ruler.start; } else if (prev == null) { start = curr - ((next - curr) / 2) * percent; size = ((next - curr) / 2) * percent; } else if (next == null) { start = curr - ((curr - prev) / 2) * percent; size = (curr - prev) * percent; } else { start = curr - ((curr - prev) / 2) * percent; size = ((next - prev) / 2) * percent; }

Note that these formulas probably aren't at all correct. This is just to demonstrate an alternate way of laying out the code

@benmccann your proposal introduces lot of code duplication and actually doesn't look clearer to me since it's hard to follow all the if / else. The current implementation is not a "hack" of prev / next but an adjustment when they are not defined. Comments explain pretty well what's going on so please don't reformat this part of the code.

benmccann · 2018-08-04T17:39:30Z

src/scales/scale.time.js

+		});
+
+		if (!barThickness) {
+			[data, ticks].forEach(function(timestamps) {


can you add a comment here:

// Calculate minInterval

Yes, I will add this.

benmccann · 2018-08-04T17:43:05Z

src/scales/scale.time.js

-			) / 2;
+
+		length = pos.length;
+		if (length) {


Can this be combined with the enclosing if statement? I.e. if (options.offset && data.length)?

Even if data is not empty, we need to use the subset of data, which only contains timestamps between min and max, so checking pos.length is needed. But, we can move this if (length) statement before the calculation of minInterval.

Ah, you're right. Maybe we could also invert the check. E.g.:

data.forEach(function(timestamp) { if (timestamp >= min && timestamp <= max) { pos.push(interpolate(table, 'time', timestamp, 'pos')); } }); length = pos.length; if (!length) { return; }

As this function needs to return an object, I will just move the check.

…he range

benmccann · 2018-08-09T15:38:31Z

src/scales/scale.time.js

+
+		length = pos.length;
+		if (length) {
+			// Calculate minInterval


This is a bit nit picky, but I would actually move this comment down a line (i.e. right above [data, ticks].forEach(function(timestamps) {) because right now it's not clear if it's referring to the entire block of code inside the if (length) statement of just to the if (!barThickness) section

Ok, agreed.

benmccann · 2018-08-09T15:42:28Z

src/scales/scale.time.js

+			}
+
+			if (!timeOpts.min) {
+				if (length === 1) {


What if length === 0 ?

This is the case that there is only one data point within the range. length === 0 is already filtered out here.

Thanks. I missed that it was filtered out up above

benmccann · 2018-08-09T15:53:36Z

src/scales/scale.time.js

@@ -361,27 +361,55 @@ function generate(min, max, capacity, options) {
 * Returns the right and left offsets from edges in the form of {left, right}.


Can you clarify more what this offset it. What is it that gets offset and why does it need to be offset?

Does this make sense?

/** * Returns the right and left offsets from edges in the form of {left, right} * where each values is a relative width to the scale and ranges between 0 and 1. * They add extra margins on the both sides by scaling down the original scale. * Offsets are typically used for bar charts. They are calculated based on intervals * between the data points to keep the leftmost and rightmost bars from being cut. * Offsets are added when the `offset` option is true. */

Super helpful!

To clarify:
Are these offsets for each bar or only the leftmost and rightmost bar? I.e. can you update "edges" to specify the edge of the chart or the edge of the category

Ok, it's "the edges of the chart".

benmccann · 2018-08-10T03:23:38Z

src/scales/scale.time.js

+		});
+
+		length = pos.length;
+		if (length) {


It could make the code slightly easier to read and remove a lot of indentation if we reversed this if statement:

if (!length) { return {left: left, right: right}; }

You could do the same just above with the options.offset check as well which would remove the need for two levels of indentation

Agreed. Will update it.

benmccann · 2018-08-12T14:38:25Z

src/scales/scale.time.js

+	}
+
+	if (!timeOpts.min) {
+		if (length === 1) {


actually I just thought of one last thing. can we move setting width outside to occur just before the if statement? I think that would allow removing the duplicate code here and in the other if statement just below

I'm not sure what you mean. width for the left and right offset needs to be calculated separately, and the calculation depends on the length of pos and barThickness.

Yes, you're right. Nevermind

simonbrunel

IMO, we should find a better way to fix that issue without relying on the barThickness from the time scale.

simonbrunel · 2018-09-16T08:51:22Z

src/scales/scale.time.js

+	var pos = [];
+	var minInterval = 1;
+	var timeOpts = options.time;
+	var barThickness = options.barThickness;


Accessing barThickness from the time scale doesn't seem ideal because it correlates the bar controller and this scale, which is something we are trying to remove / avoid. Actually, the barThickness should not be part of the scale options.

benmccann · 2019-01-01T18:42:40Z

@nagix this PR will need to be rebased. There's also an outstanding comment from Simon

nagix · 2019-01-02T15:50:06Z

Closing this for now because accessing barThickness from the time scale code is not a good design as @simonbrunel pointed out. Note that the improvement of the offset calculation for a single data point has been added in #5933.

nagix force-pushed the offset-calc branch from bc4bc9e to 7a8f8f5 Compare August 15, 2017 04:12

etimberg reviewed Aug 15, 2017

View reviewed changes

nagix force-pushed the offset-calc branch 3 times, most recently from b05b0f8 to 7eabbf0 Compare August 17, 2017 03:32

simonbrunel reviewed Aug 25, 2017

View reviewed changes

simonbrunel added the type: enhancement label Aug 25, 2017

simonbrunel added this to the Version 2.8 milestone Aug 26, 2017

benmccann mentioned this pull request Oct 7, 2017

Bug in calculateBarIndexPixels #4825

Closed

etimberg closed this Dec 31, 2017

simonbrunel removed this from the Version 2.8 milestone Jan 13, 2018

simonbrunel mentioned this pull request Mar 2, 2018

[BUG] Barchart: setting xaxes 'type: time' is causing bars to be drawn off canvas. #5312

Closed

simonbrunel reopened this Mar 2, 2018

nagix force-pushed the offset-calc branch from afa314e to 55bfeba Compare March 2, 2018 19:57

nagix force-pushed the offset-calc branch from 55bfeba to a2ec063 Compare June 29, 2018 13:38

nagix force-pushed the offset-calc branch from a2ec063 to adaeffa Compare June 29, 2018 14:23

nagix mentioned this pull request Jun 29, 2018

Fix offsetGridLine behavior with a single data point #5609

Merged

benmccann mentioned this pull request Aug 4, 2018

Flex mode in bar chart not documented #5659

Closed

benmccann reviewed Aug 4, 2018

View reviewed changes

benmccann added the status: stale label Aug 4, 2018

Calculate minInterval only when there is one or more data points in t…

839a1d8

…he range

benmccann reviewed Aug 9, 2018

View reviewed changes

benmccann reviewed Aug 10, 2018

View reviewed changes

Improve readability and add more comments

dc9edba

benmccann approved these changes Aug 12, 2018

View reviewed changes

benmccann removed the status: stale label Aug 12, 2018

benmccann reviewed Aug 12, 2018

View reviewed changes

benmccann added the status: awaiting review label Aug 12, 2018

simonbrunel requested changes Sep 16, 2018

View reviewed changes

simonbrunel removed the status: awaiting review label Sep 16, 2018

benmccann mentioned this pull request Jan 1, 2019

Align radialLinear coding style #5941

Closed

nagix closed this Jan 2, 2019

		@@ -361,27 +361,55 @@ function generate(min, max, capacity, options) {
		* Returns the right and left offsets from edges in the form of {left, right}.

Improve offset calculation for scale.offset option #4658

Improve offset calculation for scale.offset option #4658

Conversation

nagix commented Aug 15, 2017 • edited

Choose a reason for hiding this comment

nagix Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagix commented Aug 29, 2017

benmccann commented Oct 7, 2017

benmccann commented Oct 31, 2017 • edited

benmccann commented Nov 8, 2017

benmccann commented Nov 23, 2017 • edited

etimberg commented Dec 31, 2017

nagix commented Mar 2, 2018

nagix commented Jun 29, 2018 • edited by benmccann

benmccann commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmccann commented Jan 1, 2019 • edited

nagix commented Jan 2, 2019

nagix commented Aug 15, 2017 •

edited

nagix Aug 15, 2017 •

edited

benmccann commented Oct 31, 2017 •

edited

benmccann commented Nov 23, 2017 •

edited

nagix commented Jun 29, 2018 •

edited by benmccann

benmccann commented Jan 1, 2019 •

edited