Polar area: align start angle with angleLines

[kurkle]

Polar area treats startAngle as radians and 0 at right. However radar chart and radialLinear scale expects startAngle to be degrees and 0 to be at top.

https://codepen.io/kurkle/pen/RERWEz

Note: I did not botrher changing values displayed to radians for this gif. They were just converted to radians for config.

Fixes #5907

[simonbrunel]

I think that's a breaking change, right?

[kurkle]

@simonbrunel Yes, its breaking change. Also a bug fix 🙈

[simonbrunel]

I don't think we can/should change the unit of startAngle (radians -> degrees), neither where the angle 0 is positioned (right -> top), because all projects that explicitly set this option will be impacted by major visual changes.

[kurkle]

Should not have it both ways either 2.7.3
2.7.3 projects would probably have ticks hidden always.
Both default to start at top, only 2.7.3 does it by setting startAngle to - 0.5PI (so by default ticks are -1.57° off)

[simonbrunel]

I updated the PR description with a before / after screenshots.

The problem is when there is no angle lines, the current (2.7.3) behavior is not bugged but startAngle is expected to be in radian. I agree that startAngle in not consistent between radar and polar charts, but should we break existing project that don't use angle lines (display: false by default in polar)?

Another question: why startAngle doesn't apply when angleLines.display: false?

[kurkle]

Another question: why startAngle doesn't apply when angleLines.display: false?

getValueCount returns 0

	function getValueCount(scale) {
		var opts = scale.options;
		return opts.angleLines.display || opts.pointLabels.display ? scale.chart.data.labels.length : 0;
	}

getIndexAngle divides by that 0

		getIndexAngle: function(index) {
			var angleMultiplier = (Math.PI * 2) / getValueCount(this);

[kurkle]

The problem is when there is no angle lines, the current (2.7.3) behavior is not bugged but startAngle is expected to be in radian. I agree that startAngle in not consistent between radar and polar charts, but should we break existing project that don't use angle lines (display: false by default in polar)?

I think not. Something should still be done. Couple of options come in mind;

• Apply this fix only if angleLines (or pointLabels) are shown.
• Apply this fix only if abs(startAngle)>2PI. If its > 0 and <= 2PI warn about deprecation
• Introduce new option to work around this issue
• Postpone this change to 3.0

[nagix]

I would suggest to

• not change startAngle (-0.5 * Math.PI by default) for polarArea
• add startAngle (-0.5 * Math.PI by default) for radar
• keep startAngle in radians
• make getIndexAngle return radians staring from the right and staying between -PI and PI
• modify the related code (min and max arguments for determineLimits, getTextAlignForAngle and adjustPointPositionForLabelHeight)
• add Math.PI / 2 to

  Chart.js/src/scales/scale.radialLinear.js

  Line 537 in 52b9793

  ctx.rotate(startAngle);

This won't break existing projects.

[nagix]

Maybe getIndexAngle can return radians from the top and leave other parts in radialLinear.js as is.

[kurkle]

I would suggest to

• keep startAngle in radians

This was my initial approach.
However that test broke and made me think it was supposed to be degrees.
IMO configuration should be degrees and immediately converted to radians when parsing (so code is all radians)
--snip--

This won't break existing projects.

What about radar charts with startAngle defined in degrees? Custom chart types using radialLinear axis?

[nagix]

I notice that startAngle was first introduced to polarArea charts in a452094 representing radians while offsetAngle was first introduced to radar charts in #2984 representing degrees, then renamed to startAngle in #2947. Unfortunately, the description of startAngle for radar charts was removed from the document when v2.6 was released.

[benmccann]

It seems to me like the fundamental problem here is that all the radialLinear scale methods like getIndexAngle (used by the aforementioned test) utilize degrees while the polarArea chart uses radians. This is a big problem since the polarArea chart uses the radialLinear scale.

Here's from the radialLinear scale:

	getIndexAngle: function(index) {
		var angleMultiplier = (Math.PI * 2) / getValueCount(this);
		var startAngle = this.chart.options && this.chart.options.startAngle ?
			this.chart.options.startAngle :
			0;

		var startAngleRadians = startAngle * Math.PI * 2 / 360;

		// Start from the top instead of right, so remove a quarter of the circle
		return index * angleMultiplier + startAngleRadians;
	},

The problem here is that the scale doesn't know whether this.chart.options.startAngle is defined as radians or degrees since the radar chart and polarArea chart have defined them differently.

I'm not really sure how to fix this while maintaining backwards compatibility. I think I agree with @kurkle when he calls it a bug in the polarArea chart. It may be a bit painful to change it, but it's also a bit painful to leave this bug in place. I would vote for fixing the bug which I think means we need to change to degrees

@simonbrunel @nagix thoughts?

[nagix]

This problem appears only when scale.angleLines.display is explicitly set to true in a polarArea chart, so I guess not many projects are suffering from this. As discussed in #5279, changing to degrees in v3.0 seems to be way to go. Until then, why don't we use the following workaround?

var startAngleRadians = (this.chart.config.type === 'polarArea') ?
    startAngle + Math.PI * 0.5 :
    startAngle * Math.PI * 2 / 360;

[kurkle]

How about this? (it does not work with decimal angles between -6.28 and 6.28 degrees (about), excluding 0. But who needs those in a chart?):

	helpers.autoRadians = function(angle) {
		var TAU = 2 * Math.PI;
		if (angle >= -TAU && angle <= TAU && Math.floor(angle) !== angle) {
			console.warn('angles defined in radians are deprecated and will not be supported in 3.0.');
			return angle;
		}
		return helpers.toRadians(angle);
	};

• and rotate canvas 90 degrees when angleLines are displayed (to match with scale)

Demonstrated in this pen: https://codepen.io/kurkle/pen/ZVvrqg

Added "&& Math.floor(angle) !== angle" to allow whole degrees of 1..6 (this leaves tiny holes in radian support)
Holes in radians approximated in degrees:

1/180*PI = 57.29577951308232
2/180*PI = 114.59155902616465
3/180*PI = 171.88733853924697
4/180*PI = 229.1831180523293
5/180*PI = 286.4788975654116
6/180*PI = 343.77467707849394

[benmccann]

Another idea: we could introduce an option for degrees vs radians. Then the scale doesn't need to know which chart is being used and users can specify which ever they prefer

[benmccann]

This works, but it'd be nicer if the scale didn't have to know about the chart type. What if we instead introduce an option for degrees vs radians? Then the scale doesn't need to know which chart is being used and users can specify which ever they prefer

[kurkle]

Trying to fix a bug here, I'd rather not introduce a new option to work around it.
If this was fixed using a new option, then would have to specify conflicting defaults to chart types. IMO that would be a new confusing public API to keep backward compatible.

[benmccann]

I just remembered this PR. It might be a good idea to standardize on degrees vs radians now that we're working on 3.0, so I'll reopen this one. @kurkle you can close it again if it's not something you want to eventually pursue

[kurkle]

Rebased

[benmccann]

The polar area behavior is changed? It's now in degrees with 0 at the top, which wasn't the case before? If that's right, then it'd be good to add something to migration guide about that

[benmccann]

It's a bit surprising to me that this is returning radians now that all the options are specified in degrees. Should we just standardize on degrees everywhere?

[kurkle]

CanvasRenderingContext2d is going to need radians as input, so need to convert at some point.
IMO its clear that options are degrees, while everything internal (API included) should be radians.

There are only couple of things that contribute to that opinion:

• developers are usually familiar with radians
• people in general (users) are more likely familiar with degrees.
• most functions require radians as input

[benmccann]

Ok. That makes sense. Should we convert determineLimits to use radians and remove the toDegrees function in that case?

[benmccann]

@kurkle this one will need to be rebased

[kurkle]

Ok, this is going nowhere.

[benmccann]

I'd rather merge as is than let perfect be the enemy of good