processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.48k stars 3.29k forks source link

circle(radius) breaks my code and teaching resources, future plans for it to change again? #3494

Closed makeyourownalgorithmicart closed 5 years ago

makeyourownalgorithmicart commented 5 years ago

Hi

I previously proposed changes to p5js to make the language easier for younger and non-technical newcomers, in line with the processing foundations own aims and objectives for reach and accessibility. They were reacted at the time.

I went on and created my own simple.js library which implemented changes which addressed issues found from actual teaching experience with very young children, first-time coders, and newcomcers from the arts. It included several changes such as a repeat() instruction, consistency aliases, spelling options, friendlier random number function .. and a circle() and square() function.

The library was included in the popular openprocessig.org, as an example of uptake.

Since then another suggestion, referring to my original proposal, was made to add circle(). This was approved.

Sadly it broke compatibility with my own code.

My own teaching experience suggests that newcomers / young people find the idea of diameter more intuitive for a circle than radius. My own code used the third parameter as a diameter. The recent p5js 0.7.3 release uses it as a radius.

So this breaks my code, and teaching resources.

So my question is - as go through all that code and teaching resources, will p5js suddently change this behaviour again?

Personally I wish there had been some consultation - especially amongst those of us who had expended a lot of time and care trying to raise the issue, and the do something about it. I also think some news / awareness / social media discussion around this attempt to make p5js more friendly and accessible would also be good. It would address any perception that the upstream developers are not really that interested in such human issues.

hellonearthis commented 5 years ago

Why not just have diameter = radius * 2 or use diameter / 2 when calling circle/ellipse . The center point of circles and rectangles are also different unless rectmode is used. Random looks mostly the same, apart from yours has a floor included.

With the few number of functions in simple,js it should be easy to update that to work with P5.js

lmccart commented 5 years ago

Hey @makeyourownalgorithmicart thanks for your post! We did in fact implement circle() directly based on your feedback and are super excited about your library. Sorry it took a while. I think we just got the implementation details not quite right. An enthusiastic contributor responded to the request for circle() addition and it got merged, all before there was a lot of time for comment. We're totally open to considering using diameter, I think this makes some sense and we can discuss.

p5.js is not at 1.0 yet, so there are sometimes significant changes. We hope that they are not too disruptive but sometimes we don't get everything right. I'd love for there to be more social media discussion / emails / etc, but there is also a bandwidth issue of what we can maintain (in terms of github, social media, website, reference, npm, cdn, etc) while doing this project in volunteered time. Any suggestions you have on this front would be great. One thing I'd love to see is discussion on the forum, maybe there could be a specific channel for discussion of new features. It just needs someone to moderate and relay the points that come up back to github.

So, perhaps we can discuss the circle() implementation some more here and make some positive changes if there is agreement?

makeyourownalgorithmicart commented 5 years ago

hi @lmccart thanks for taking the time to get back to us, and also for making a thoughtful and considered comment.

I totally sympathise with the bandwidth issue! I also do this entirely in volunteer time, including teaching kids and developing teaching resources.


On the raising awareness / social media / blogs point - I do think it is important for several reasons:

I don't mean for the above to sound negative. I hope they are seen as constructive suggestions.

I think p5js is a hugely important technology, enabling people to easily and pleasantly, discover a passion or talent that they didn't know they had. It does this by removing technical barriers (software install), and making the language itself easy to use. Please don't underestimate its power to transform lives - I've seen t first hand, young people with poor opportunities discover they have a passion and talent for coding or digital design.


On the specific issue of circle() . what's the processes for concluding a decision?

Do we need a poll, or a set of pros/cons to a group of project leads? Is there formal governance? Here is my rationale:

I know it is unreasonable for me to ask for a timescale on completing this issue, but my selfish reason is that I will be publishing teaching resources and a book "creative coding for kids" and have started reworking the content and code .. but will pause now.


Afterthought - looking back at this huge comment, it all points to a Foundation funded usability working group ...

hellonearthis commented 5 years ago

artists do come here and use github...

meiamsome commented 5 years ago

In terms of the original bug, +1 for using diameter instead here - we already have ellipseMode(RADIUS) that would make the current implementation behave strangely (The argument becomes half the radius.) I think this should be considered a bug and we should drop the 2 * from the current implementation

makeyourownalgorithmicart commented 5 years ago

I suspect this issue has lost momentum.

What's the best thing I can do? Prepare a PR?

(I'll have to learn how to do this, I've never done one before...)

montoyamoraga commented 5 years ago

hi everyone! i also agree that circle() should use diameter instead of radius i just did pull request #3499 to try to fix it, I'm not totally sure if what I did is right, can someone help me check if it's right?

hellonearthis commented 5 years ago

I like the idea of a mode setting so users can select radius or diameter.

montoyamoraga commented 5 years ago

Hi @hellonearthis ! Yes, I agree that users should be able to select either radius or diameter, and I think ellipseMode() should affect the settings of the circle, since a circle is a special case of an ellipse, with equal width and height. The same goes for rectMode() affecting rect() and square().

I think that since circle() and square() are aliases for ellipse() and rect(), this behaviour is already implemented, right?

And I agree with people above, that the default behavior should be diameter, not radius, in order to match ellipse().

What do you think?

meiamsome commented 5 years ago

@montoyamoraga

You are correct, ellipseMode does already affect circle, but it is very clearly wrong - in diameter mode the third parameter is the radius of the circle. In radius mode it is half the radius, which leads to the conclusion that this was just a mistake.

intuitivetextmining commented 5 years ago

default should be diameter

I've successfully changed the source code and will post my changes on the next few minutes

intuitivetextmining commented 5 years ago

here's the. new code which only has tiny changes, and also includes the changes for the auto-generated documentation which I've also tested work correctly

/**
 * Draws a circle to the screen. A circle is a simple closed shape.
 * It is the set of all points in a plane that are at a given distance from a given point, the centre.
 * This function is a special case of the ellipse() function, where the width and height of the ellipse are the same.
 * Height and width of the ellipse is equal to the diameter of the circle..
 * By default, the first two parameters set the location of the centre of the circle, the third sets the diameter of the circle.
 *
 * @method circle
 * @param  {Number} x  x-coordinate of the centre of the circle.
 * @param  {Number} y  y-coordinate of the centre of the circle.
 * @param  {Number} d  diamater of the circle.
 * @chainable
 * @example
 * <div>
 * <code>
 * // Draw a circle at location (30, 30) with a diameter of 20.
 * circle(30, 30, 20);
 * </code>
 * </div>
 *
 * @alt
 * white circle with black outline in mid of canvas that is 55x55.
 */
p5.prototype.circle = function() {
  var args = Array.prototype.slice.call(arguments, 0, 2);
  args.push(arguments[2]);
  args.push(arguments[2]);
  this.ellipse.apply(this, args);
};

my apologies that this isn't a PR, I am totally new to grunt, npm, etc ...

intuitivetextmining commented 5 years ago

ps @meiamsome .. hello from Cornwall! (I also used to live in Bristol ..)

Spongman commented 5 years ago

is all the arguments array manipulation still necessary?

wouldn't it be simpler and less prone to deoptimization, just to call this.ellipse(x, y, r, r) ?

meiamsome commented 5 years ago

@Spongman There's no reason we couldn't just do p5.prototype.circle = p5.prototype.ellipse as ellipse already handles the 3 argument case... depending upon if we care about the behaviour when someone calls circle with 4 arguments.

(@intuitivetextmining oh, hello :P )

Spongman commented 5 years ago

having a separate definition would allow parameter validation to happen...

makeyourownalgorithmicart commented 5 years ago

great! looks like we have a solution.

what are the next steps? do we have to ask someone to "approve it" .. or is it a "board" decision?

do we need to consult more widely?

(thanks @meiamsome and @montoyamoraga @Spongman for helping on this)

makeyourownalgorithmicart commented 5 years ago

I learned how to create a PR

here it is: https://github.com/processing/p5.js/pull/3500

lmccart commented 5 years ago

Thanks @makeyourownalgorithmicart I'm going to try to merge #3499 which has the same change as soon as we get one small edit taken care of.

Regarding your notes about raising awareness / social media / blogs: thanks for these thoughts. To be clear, I was not arguing against this at all, I am completely in favor of all of it. I was mentioning that it is a lot to take on and so any help is very much appreciated! Additionally, I'm happy to share that the Processing Foundation has just hired a Director of Education Outreach @saberkhaniscool who will be helping us in efforts to better connect with a broader range of teachers and students. An official medium post introducing and welcoming him will be coming soon. 😊

makeyourownalgorithmicart commented 5 years ago

thanks @lmccart that makes a lot of sense.

and @saberkhaniscool I'd be very happy to discuss ideas and wha I've learned my experience with you.

great work!

saberkhan372 commented 5 years ago

Hi @makeyourownalgorithmicart would love to hear your thoughts. Wanna drop me an email so we can get started. It is in my bio.

montoyamoraga commented 5 years ago

thanks everyone! I just updated #3499 to fix the var instead of let. now it passes the travis checking, and its ready to be merged. can anyone double check that what I wrote is right? go p5.js =)

lmccart commented 5 years ago

fixed iwth #3499 🙌