Enable arbitrary rotation of datapoints by joel-hamilton · Pull Request #5319 · chartjs/Chart.js

joel-hamilton · 2018-03-06T01:59:30Z

Thought I'd take a stab at #5213, it seems like a really useful feature. I added dataset.pointRotation, and point.pointRotation options that accept positive or negative angle (in degrees). The canvas is translated to (0,0) and then rotated before drawing the point, so x & y should now be zero for the tests.

Here is a link to a quick example

I'm new to the codebase so I might have missed something. Happy for any input or suggestions!

joel-hamilton · 2018-03-06T02:17:06Z

CodeClimate is chirping me about similar code blocks. I was trying to follow the existing conventions, but we may have reached a point where it's getting too repetitive. It might be worth refactoring all the getPointXxxxx(point, index) functions to a single getPointValue(point, index, propertyName) function. I can do that in this PR if desired.

etimberg

@joelhamilton5 this is looking good. Just the one comment in the line controller code

etimberg · 2018-03-07T02:16:34Z

src/controllers/controller.line.js

+				pointRotation = custom.pointRotation;
+			} else if (!isNaN(dataset.pointRotation) || helpers.isArray(dataset.pointRotation)) {
+				pointRotation = helpers.valueAtIndexOrDefault(dataset.pointRotation, index, pointRotation);
+			} else if (!isNaN(dataset.pointRotation)) {


I don't think this case will ever be hit. If !isNaN(dataset.pointRotation) is true, the previous else if will run.

Oops, right you are! Fixed.

benmccann · 2018-04-05T02:26:00Z

Looks good to me. Do you have any thoughts on this one @simonbrunel?

benmccann · 2018-04-07T20:50:16Z

@joelhamilton5 could you rebase this? thanks!

simonbrunel

Looks good, I would rename the element option for elements.point.rotation, pointRotation being redundant in this case (at some point we should deprecate element.point.pointStyle for element.point.style).

simonbrunel · 2018-04-10T07:47:02Z

src/controllers/controller.line.js

 		},

+		getPointRotation: function(point, index) {
+			var pointRotation = this.chart.options.elements.point.pointRotation;


var rotation = this.chart.options.elements.point.rotation;

This also needs to be added to the element documentation as rotation.

simonbrunel · 2018-04-10T07:47:33Z

src/controllers/controller.radar.js

 					borderColor: custom.borderColor ? custom.borderColor : helpers.valueAtIndexOrDefault(dataset.pointBorderColor, index, pointElementOptions.borderColor),
 					borderWidth: custom.borderWidth ? custom.borderWidth : helpers.valueAtIndexOrDefault(dataset.pointBorderWidth, index, pointElementOptions.borderWidth),
 					pointStyle: custom.pointStyle ? custom.pointStyle : helpers.valueAtIndexOrDefault(dataset.pointStyle, index, pointElementOptions.pointStyle),
+					pointRotation: custom.pointRotation ? custom.pointRotation : helpers.valueAtIndexOrDefault(dataset.pointRotation, index, pointElementOptions.pointRotation),


rotation: custom.pointRotation ? ...

simonbrunel · 2018-04-10T07:51:46Z

src/helpers/helpers.canvas.js


+		ctx.save();
+		ctx.translate(x, y);
+		ctx.rotate(rotation * Math.PI / 180);


I don't see how we can do differently right now but I'm a bit worry of these extra save / translate / rotate / restore because drawing point is already pretty slow. Anyway, if we are translating we should remove the use of x & y everywhere in the switch (and remove x = 0 and y = 0).

Sounds good!

simonbrunel · 2018-04-10T07:56:06Z

src/controllers/controller.line.js

 				// Appearance
 				radius: custom.radius || helpers.valueAtIndexOrDefault(dataset.pointRadius, index, pointOptions.radius),
 				pointStyle: custom.pointStyle || helpers.valueAtIndexOrDefault(dataset.pointStyle, index, pointOptions.pointStyle),
+				pointRotation: me.getPointRotation(point, index),


rotation: me.getPointRotation(point, index),

simonbrunel · 2018-04-10T08:20:13Z

src/elements/element.point.js

 		var model = this._model;
 		var ctx = this._chart.ctx;
 		var pointStyle = vm.pointStyle;
+		var rotation = vm.pointRotation;


var rotation = vm.rotation;

…10 (#5275) * Bugfix: Improve polyfill function of log10 to return whole powers of 10 as integer values, as it caused endless loop in IE11 in the tick creation loop. * Compare floating-point numbers directly instead of using unnecessary division. rotate works added docs delete line

docs/charts/bubble.md

 | `borderColor` | bubble border color
 | `borderWidth` | bubble border width (in pixels)
 | `pointStyle` | bubble [shape style](../configuration/elements#point-styles)
+| `rotation` | bubble rotation (in degrees)


src/controllers/controller.bubble.js

 				'hitRadius',
-				'pointStyle'
+				'pointStyle',
+				'rotation'


simonbrunel · 2018-06-16T15:40:12Z

@joelhamilton5 I'm deeply sorry but I'm realizing that for the bubble controller other options are not prefixed (except pointStyle) and thus I think it should be rotation. For the line controller, pointRotation is the correct name.

@etimberg @benmccann what do you think: should we keep it consistent and simple by using rotation for the bubble controller only (basically revert da23599 and fix the documentation)?

Side note: pointStyle should actually be style but that's an error introduced at the element level element.point.pointStyle (instead of element.point.style). In 3.0, we should certainly scope those options, for example: dataset.elements.point.rotation (easier to merge options).

benmccann · 2018-06-16T16:50:36Z

rotation for bubble chart sounds fine to me

I filed an issue to track the cleanup for 3.0: #5572

joel-hamilton · 2018-06-20T22:28:40Z

@simonbrunel No problem, it's all fixed up now!

simonbrunel

@joelhamilton5 I think you missed a few changes, right?

simonbrunel · 2018-06-24T11:21:54Z

docs/charts/bubble.md

 | `borderColor` | bubble border color
 | `borderWidth` | bubble border width (in pixels)
 | `pointStyle` | bubble [shape style](../configuration/elements#point-styles)
+| `pointRotation` | bubble rotation (in degrees)


pointRotation -> rotation

simonbrunel · 2018-06-24T11:22:13Z

src/controllers/controller.bubble.js

 				'hitRadius',
-				'pointStyle'
+				'pointStyle',
+				'pointRotation'


pointRotation -> rotation

simonbrunel · 2018-07-07T15:54:13Z

Thanks @joelhamilton5

- Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius` - Remove `translate()` and `rotate()` to fix the regression introduced by chartjs#5319

- Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius` - Remove `translate()` and `rotate()` to fix the regression introduced by chartjs#5319 - Refactor `rectRounded` for better performance

- Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius` - Remove `translate()` and `rotate()` to fix the regression introduced by #5319 - Refactor `rectRounded` for better performance

…tjs#5858) - Calculate the vertices of the shapes so that they are inscribed in the circle that has the radius of `pointRadius` - Remove `translate()` and `rotate()` to fix the regression introduced by chartjs#5319 - Refactor `rectRounded` for better performance

etimberg reviewed Mar 7, 2018

View reviewed changes

etimberg requested a review from simonbrunel March 7, 2018 02:18

etimberg previously approved these changes Apr 5, 2018

View reviewed changes

joel-hamilton dismissed etimberg’s stale review via 7a74ce8 April 10, 2018 00:45

simonbrunel requested changes Apr 10, 2018

View reviewed changes

simonbrunel added the type: enhancement label Apr 10, 2018

simonbrunel added this to the Version 2.8 milestone Apr 10, 2018

jcopperfield and others added 6 commits April 13, 2018 08:28

added ability to rotate points

8cf7910

remove redundant elseif

d47c278

fix merge conflict

b2cb8b1

change pointRotation to rotation, add docs

d22d7ce

fix draw point

d182d34

simonbrunel reviewed May 22, 2018

View reviewed changes

simonbrunel requested a review from etimberg May 22, 2018 20:41

etimberg reviewed May 23, 2018

View reviewed changes

src/controllers/controller.bubble.js

'hitRadius',

'pointStyle'

'pointStyle',

'rotation'

This comment was marked as resolved.

Sign in to view

rotation -> pointRotation

da23599

rename bubble options for point rotation

6b9dfaf

simonbrunel requested changes Jun 24, 2018

View reviewed changes

change pointRotation -> rotation

71ae83a

simonbrunel approved these changes Jul 7, 2018

View reviewed changes

simonbrunel merged commit 0ddd0ee into chartjs:master Jul 7, 2018

simonbrunel mentioned this pull request Jul 7, 2018

Support ability to rotate data points #5213

Closed

simonbrunel mentioned this pull request Oct 24, 2018

[BUG] dataset pointRotation option not working #5785

Closed

nagix mentioned this pull request Nov 19, 2018

[BUG] Release v2.7.3 scatter plot CanvasGradient #5839

Closed

nagix mentioned this pull request Nov 23, 2018

Adjust the size of rectRounded/rectRot points to fit the circle with pointRadius #5858

Merged

vpetersson mentioned this pull request Mar 7, 2020

[Snyk] Upgrade chart.js from 2.7.3 to 2.9.3 WoTTsecurity/api#764

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Enable arbitrary rotation of datapoints (chartjs#5319)

d4c7d81

Conversation

joel-hamilton commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joel-hamilton commented Mar 6, 2018

Uh oh!

etimberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented Apr 5, 2018

Uh oh!

benmccann commented Apr 7, 2018

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as resolved.

Uh oh!

simonbrunel commented Jun 16, 2018

Uh oh!

benmccann commented Jun 16, 2018

Uh oh!

joel-hamilton commented Jun 20, 2018

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Jul 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joel-hamilton commented Mar 6, 2018 •

edited

Loading