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

angleMode() and degrees() / radians() #163

Closed
shiffman opened this issue Mar 30, 2014 · 17 comments
Closed

angleMode() and degrees() / radians() #163

shiffman opened this issue Mar 30, 2014 · 17 comments

Comments

@shiffman
Copy link
Member

Thinking out loud about degrees() and radians(). Should these methods depend on the current "angle mode"? In other words, at the moment the following two snippets produce different results.

angleMode(DEGREES);
var a = degrees(PI);
angleMode(RADIANS);
var a = degrees(PI);

I wonder if you are calling degrees() you really always mean to do that conversion while other methods that act on an angle (rotate(), PVector.fromAngle(), etc.) of course act according to the current "angle mode".

I am not sure about this one.

@brysonian
Copy link
Contributor

I cannot imagine why degrees(PI) should ever return anything other than 180. Also angleMode isn't part of the processing API as of now, is it necessary/helpful?

@evhan55
Copy link
Contributor

evhan55 commented Mar 30, 2014

@brysonian angleMode is a nice way to think about angles for some students, also, Khan academy uses it in their curriculum and they are considering switching to p5.js, so it was a request from them.

@evhan55
Copy link
Contributor

evhan55 commented Mar 30, 2014

@shiffman so degrees() is in the current Processing API? yes it probably always means to do the conversion. we could add a parallel radians() function to do the opposite conversion on the fly.

@lmccart
Copy link
Member

lmccart commented Mar 30, 2014

yep, radians is in there already, too!

@shiffman
Copy link
Member Author

Great, I'm happy to make this change to degrees() and radians() to have them always do the conversion (regardless of the current angle mode) if everyone agrees that is best.

Does anyone know how the proper JS way to get access to the p5's current angle mode in the PVector object? I'll need it for rotate(), heading(), and angleBetween().

@evhan55
Copy link
Contributor

evhan55 commented Mar 31, 2014

Maybe we should make constants global.

@taseenb
Copy link
Contributor

taseenb commented Mar 31, 2014

not sure, but you can try to get the p5 object, before the PVector constructor: var p5 = require('core');
then i guess you should be able to get the current angle mode from the settings: p5.settings.angleMode

(i think it is better to avoid using globals, unless in global mode, or that would be worse, later..!)

@taseenb
Copy link
Contributor

taseenb commented Mar 31, 2014

sorry no. it won't work. the core module just returns the p5 class, not the instance. and i noticed that PVector is global... i am probably missing something, but dont think there is a way, at the moment.

@lmccart
Copy link
Member

lmccart commented Mar 31, 2014

yeah, I found the same thing. having a similar problem trying to get access to constants in the unit tests. will think about it some more...

@taseenb
Copy link
Contributor

taseenb commented Mar 31, 2014

i think we need a reference to the current instance of p5 (not only the constructor) always available after it has been created.

PVector (and any other constructor used within p5) could be a constructor method of p5.prototype, with a global reference for convenience.

@evhan55
Copy link
Contributor

evhan55 commented Mar 31, 2014

@taseenb I like both of your suggestions. I have been thinking PVector should be within p5 as well.

@shiffman @lmccart thoughts? Would this break compatibility with Processing?

@shiffman
Copy link
Member Author

I think @taseenb's proposal (for PVector to be a constructor method of p5.prototype) makes sense for gaining access to the p5 instance. However, my one concern is that I think it is useful for PVector to exist on its own as a stand-alone object/prototype. For example, would there ever be a case where you want to create PVector instances without there being an actual p5 instance? I think so, there is no conceptual need for a vector to be tied to p5. A vector is just numbers and some math operations.

So, is the following possible?

  • PVectors have access to the p5 instance, unless that instance is undefined in which case PVector defaults to a simple behavior.

So far I believe all we need from the instance is angleMode and access to random() so that random PVectors can also be seeded.

  • For angleMode, if p5 is undefined, then we will assume angles are in radians.
  • For random2D() and random3D() if p5 is undefined we will default to using Math.random().

@taseenb
Copy link
Contributor

taseenb commented Apr 1, 2014

@shiffman sure, i see what you mean. but PVector can be added as method of p5 and exposed to the global context at the same time. just an alias. sure, it will behave differently, because the global version won't have access to any sketch data. but, as you say, you can easily manage both cases.

concerning the instance of p5, it probably deserves a more general discussion, since it involves how the instances of p5 can/should expose the prototype. this in important because, this may be how to create plugins and extend p5.
jquery uses an excellent way of handling these problems. i don't think that in p5 we should provide a constructor to the user. we could better return an "improved" instance. this is possible:

[EDIT : solution improved, without new operator]

p5()
// it should return a new instance of the p5 object representing the sketch

p5.extend()
// extends the prototype of p5 with new functions

we would be able to extend the library by creating plugins (and also mix global and local objects, if needed in some particular cases like PVector):

var PVector = window.PVector = function() { 
  var p5sketch = this.p5sketch;

  if (p5sketch) {
    // we have access to the instance of p5
  }
};

// by using an 'extend' method, everyone can add a function to the prototype of p5
// and make it available in every instance (ie. that's a plugin)
p5.prototype.extend({ 'PVector': PVector });

See: https://gist.github.com/taseenb/9941545

@lmccart
Copy link
Member

lmccart commented Apr 2, 2014

@evhan55 do you have thoughts about this solution? should I try to implement something like this or do you want to do it since you've been working on core recently?

@taseenb
Copy link
Contributor

taseenb commented Apr 2, 2014

@lmccart @evhan55 as you may have noticed, there is at least one mistake in my example: ie, i simplified too much the extend step (and you cannot bind 'this' to another function when using 'new', like in PVector). a working solution must be a little different, but should work. i'll try to make a working gist to test it.

@taseenb
Copy link
Contributor

taseenb commented Apr 2, 2014

i made a gist: https://gist.github.com/taseenb/9941545

the cleanest solution i see in javascript is to remove the new operator. that makes it easy to extend p5 and get access to the sketch instance. the new operator does not allow binding a js function to another context.
@shiffman in this case, you would simply need to modify your class to return an instance, not a constructor:

function PVector(x, y, z) {
  // get the sketch instance, if available
  var sketch = this.p5sketch; 

  // return an instance of the vector
  function _vector(x, y, z) { ... }
  return new _vector(x, y, z);
}

@evhan55
Copy link
Contributor

evhan55 commented Apr 2, 2014

@taseenb Thank you, this is looking great. I would like to try to incorporate this suggestion into core to practice with PVector and see how it feels to have it as an extension or plugin. It would be a great first step to having a system for plugins, and PVector can be the first official plugin of p5.

I have a couple of other tasks to do in core first, and then I will attempt this.

@shiffman @lmccart how does the plugin solution sound to you?
I think it sounds well worth trying, and I am happy to try it.

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

No branches or pull requests

5 participants