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

To radians function #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

To radians function #47

wants to merge 3 commits into from

Conversation

webocs
Copy link

@webocs webocs commented Jun 5, 2019

Description

Added a toRadians function and a corresponding demo for this Feature Request #34.

Changes

  • Added toRadians function in Boilerplate.js
  • Added a demo for the function
  • Added new dist using makefile script

+ Added toRadians function in Boilerplate.js
+ Added a demo for the function
+ Added new dist using makefile script
@desandro
Copy link
Member

desandro commented Jun 5, 2019

Thanks for this contribution! I'm still evaluating if this feature is worth adding to Zdog and how. I think I may prefer Chris Gannon's suggestion of setting a Zdog-wide property like useDegrees. Otherwise, doing this conversion can be done on developer-side, with either your own helper function or inline:

rotate: { y: 45/360 * Zdog.TAU }

@webocs
Copy link
Author

webocs commented Jun 5, 2019

I see! no problem, this was pretty straightforward so I figured I could PR it even if it wasn't used!. Are you planning on converting to radians for each operation if the variable is set, thus leaving the engine as it is now. Or are you planning to change the math of the rotateProperty function?

function rotateProperty( vec, angle, propA, propB ) {
  if ( !angle || angle % TAU === 0 ) {
    return;
  }
  // It could be as simple as this??.  unless you want to change the math
  angle =  useDegrees? angle/360 * Zdog.TAU : angle;

  var cos = Math.cos( angle );
  var sin = Math.sin( angle );
  var a = vec[ propA ];
  var b = vec[ propB ];
  vec[ propA ] = a*cos - b*sin;
  vec[ propB ] = b*cos + a*sin;
}

@desandro
Copy link
Member

desandro commented Jun 5, 2019

Yeah, I was thinking a library-wide change in rotateProperty. I'm waffling on this feature as it feels un-Zdog-like in that its extra stuff for personal preference. I'm now thinking a good approach is to make rotateProperty a proper Vector method so you could duck punch it.

// if Vector.prototype.rotateProperty were a thing
Zdog.Vector.prototype.rotateProperty = function ( angle, propA, propB ) {
  if ( !angle || angle % 360 === 0 ) {
    return;
  }
  angle = angle / 360 * Zdog.TAU;
  var cos = Math.cos( angle );
  var sin = Math.sin( angle );
  var a = vec[ propA ];
  var b = vec[ propB ];
  this[ propA ] = a*cos - b*sin;
  this[ propB ] = b*cos + a*sin;
};

Another solution is to add type checking. Any string ending with deg would be treated as a degree. You could pass in '180deg'. So Zdog would support both degrees and radians interchangably.

function rotateProperty( vec, angle, propA, propB ) {
  var isDegree = typeof angle == 'string' && angle.match( /deg$/ );
  if ( isDegree ) {
    angle = parseFloat( angle ) / 360 * TAU;
  }
  if ( !angle || angle % TAU === 0 ) {
    return;
  }
  var cos = Math.cos( angle );
  var sin = Math.sin( angle );
  var a = vec[ propA ];
  var b = vec[ propB ];
  vec[ propA ] = a*cos - b*sin;
  vec[ propB ] = b*cos + a*sin;
}

@webocs
Copy link
Author

webocs commented Jun 5, 2019

The second option sounds great and more natural if you ask me. I imagine some people copying demos from the internet that won't work because they missed the config line at the beginning. This approach will eliminate that hazard.

@chrisgannon
Copy link

chrisgannon commented Jun 7, 2019

I think passing a string like '180deg' is cumbersome and unnecessary - passing a number for degrees would be ideal.

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

Successfully merging this pull request may close these issues.

None yet

3 participants