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.

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

Change rotation functions back to radians #743

Closed
binoculars opened this issue Jan 14, 2014 · 13 comments
Closed

Change rotation functions back to radians #743

binoculars opened this issue Jan 14, 2014 · 13 comments

Comments

@binoculars
Copy link
Contributor

It doesn't make sense to make the rotations in degrees since javascript accepts radians in the math functions (e.g. MATH.sin) and native canvas context rotation.

@ghost
Copy link

ghost commented Jan 15, 2014

I agree with binoculars. I'd prefer to set the rotation in radians instead of degrees. It is more natural in mathematical calculations.

@moeriki
Copy link
Contributor

moeriki commented Feb 10, 2014

Third that. I don't get the change really.

@samsieber
Copy link

I also agree. It seems like an abitrary change that a) breaks code and b) goes against the grain of the native javascript ecosystem. Can I ask why it was made?

@lavrton
Copy link
Contributor

lavrton commented Mar 5, 2014

👍 @ericdrowell, your thoughts?

@ericdrowell
Copy link
Owner

Hi guys. Believe it or not, there was alot of requests for this. In order to lighten the KineticJS API, I wanted to support a single unit for all attrs. This also greatly simplified lots of portions of the code base. Most people seemed to want degrees for rotation. I looked at a lot of people's code on stackoverflow and other places, and I saw a lot of people using rotateDeg() or rotationDeg(). It seemed natural to make the default unit in degrees to accommodate most people's use case.

There have been many other threads with people unhappy with this change. This is one of those topics where there is so much backlash that may have to go back and support both cases again :/

I'll add this to our todo list, but it's a sucky change

@dinther
Copy link

dinther commented Mar 17, 2014

Besides the fact that every library does rotation using radians by default. Your change just wasted a few hours of my time wading though my code to figure out why it would no longer work after updating the source from 4.something to 5.0.1 (Many changes, most of which make sense. Not this one)

My math calculates the angle in radians in an animation. Converting to degrees only to have it turned to radians somewhere internally is a waste of CPU time.

I would argue you do away with anything supporting degrees if you wish to simplify the API.

@dinther
Copy link

dinther commented Mar 17, 2014

Still messing about with this in my KineticJS based animated instrument library. Pretty pissed about this deg thing actually. Rotation data stored in databases, JSON based object definitions, everything is now wrong. Getters and setters. You should not have done this Eric. I am going to roll back to 4.5.5 till you make this right. It means changing all the new offset definitions back from {x:, y:} to [x,y] but so be it.

@jfollas
Copy link
Contributor

jfollas commented Mar 17, 2014

I can understand the frustration that comes from having an API change from underneath you, but it's not really fair to lay into Eric so hard about this. He puts so much time into producing a free and open source library to make it easier to do cool things with the canvas, and doesn't deserve to be chastised over a decision that he made based on feedback that he received.

Instead of going back to 4.5.5, it may be easier to just change your project's version of Kinetic 5.x to use radians, since Kinetic is fully open source. This would have a benefit of your code not needing changed yet again when support for radian angles are restored.

After a quick look through the code, there's only one line in node.js (1145, part of _getTransform) that would seem to be the culprit. Remove the conversion from degrees to radians there, and you can then supply all of your rotations in radians.

Test providing radian angle for rotation: http://jsfiddle.net/mjg6r/1/
Modified 5.0.1: http://pastebin.com/raw.php?i=WXaHyd1R

@dinther
Copy link

dinther commented Mar 17, 2014

You are right. I'm sorry. I had a bad weekend dealing with other library
updates as well for some commercial projects. To make programming fun again
I decided to spend today on my pet project and tripped up over this stuff.
I already reverted back as there were a lot of other issues with the new
library such as getScale returning {x:{x:1,y:1},y:{x:2,y:2}} surely that
should have been {x:1,y:2}?
But I do appreciate your email and efforts to get me going on the latest
kineticjs library.

I am close to release a free Google Maps based shipsimulator game that
makes extensive use of Kineticjs as I build an animated instrument library
on top of Kineticjs. Hopefully it will attract the same amount of attention
as my older Google Earth based ship simulator.

Yes, I do think Eric is doing an amazing job.

[image: Inline image 1]

On Mon, Mar 17, 2014 at 3:48 PM, jfollas notifications@github.com wrote:

I can understand the frustration that comes from having an API change from
underneath you, but it's not really fair to lay into Eric so hard about
this. He puts so much time into producing a free and open source library to
make it easier to do cool things with the canvas, and doesn't deserve to be
chastised over a decision that he made based on feedback that he received.

Instead of going back to 4.5.5, it may be easier to just change your
project's version of Kinetic 5.x to use radians, since Kinetic is fully
open source. This would have a benefit of your code not needing changed yet
again when support for radian angles are restored.

After a quick look through the code, there's only one line in node.js
(1145, part of _getTransform) that would seem to be the culprit. Remove the
conversion from degrees to radians there, and you can then supply all of
your rotations in radians.

Test providing radian angle for rotation: http://jsfiddle.net/mjg6r/
Modified 5.0.1: http://pastebin.com/raw.php?i=WXaHyd1R

Reply to this email directly or view it on GitHubhttps://github.com//issues/743#issuecomment-37781232
.

Regards

Paul van Dinther
Our Google Earth creations at www.planetinaction.com

@ericdrowell
Copy link
Owner

Thanks @jfollas . And to re-iterate, I've heard loud and clear that a lot of people are upset about the radian to degree change. But again, I think most people are frustrated by the change because it required lots of changes to their codebase, and not because it was necessarily a bad change. I'm still keeping a close eye on this topic, don't worry.

@dinther
Copy link

dinther commented Mar 17, 2014

No, it is definitely a bad change. The only bad one really. The changes to
setPosition, scale and offset also forced me to do a lot of code changes
but I can see why you did it.
As long as the javascript Math unit handles angles in radians you should do
the same. But if you feel very strongly about it and insist to go non
standard then I hope you provide a setRotationRad and rotateRad

@jfollas
Copy link
Contributor

jfollas commented Mar 17, 2014

For the record: I'm still in favor of the old/separate .rotation (for
radians) and .rotationDeg (for degrees), with one just being a convenience
method to set the other. Not sure what kind of impact this has on the
backend stuff, like tweens, etc...

On Mon, Mar 17, 2014 at 12:47 AM, Paul van Dinther <notifications@github.com

wrote:

No, it is definitely a bad change. The only bad one really. The changes to
setPosition, scale and offset also forced me to do a lot of code changes
but I can see why you did it.
As long as the javascript Math unit handles angles in radians you should do
the same. But if you feel very strongly about it and insist to go non
standard then I hope you provide a setRotationRad and rotateRad

Reply to this email directly or view it on GitHubhttps://github.com//issues/743#issuecomment-37784932
.

@ericdrowell
Copy link
Owner

@jfollas had a brilliant idea - we introduced a new config option that lets you choose which angle unit you'd like to use. the default is degrees, but you can switch to radians if you like. You can change to radians like this:

Kinetic.angleDeg = false;

here's the change: e5562f5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants