Skip to content

set() doesn't do anything when passed non-integer numbers #1159

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

Closed
toolness opened this issue Dec 2, 2015 · 5 comments
Closed

set() doesn't do anything when passed non-integer numbers #1159

toolness opened this issue Dec 2, 2015 · 5 comments

Comments

@toolness
Copy link
Member

toolness commented Dec 2, 2015

The following simple sketch didn't work for me:

function setup() {
  createCanvas(100, 100);
  background("black");
}

function draw() {
  set(mouseX, mouseY, color(255, 255, 255));
  updatePixels();
}

This puzzled me for quite some time until I realized that mouseX and mouseY aren't integers on my browser--they're floats!

Should set() automatically call Math.floor() on its arguments to coerce them to integers? Or will the friendly error system (#971) log an error to the console once it's fully operational again?

Also, I didn't realize that there's another existing p5 method called point(). I wonder if it might be useful to mention it in the documentation for set() (and vice versa) and maybe give some guidelines about when you might want to use one over the other?

@gtoast
Copy link

gtoast commented Dec 8, 2015

This is kind of tangential but which browser are you using? I just ran your sketch. Everything seems fine. mouseX mouseY are integers in my browser. Take a look at mouseX and mouseY in the following sketch:

function setup() {
  createCanvas(100, 100);
  background("black");
}

function draw() {
  set(mouseX, mouseY, color(255, 255, 255));
  updatePixels();


  fill(color(255,0,0));
  text([mouseX, mouseY].join(", "), 10, 20);
}

@toolness
Copy link
Member Author

toolness commented Dec 9, 2015

Oh, so I think this is actually a bit related to #974 (comment)... Basically, on either Firefox or Chrome, when you zoom the page just the slightest bit (Ctrl-+/⌘-+), mouseX and mouseY become floats.

I suppose another alternative would be to force mouseX and mouseY to always be integers, just like we did with devicePixelRatio in #1052... Though it still seems like set() shouldn't simply fail silently when passed floats, too.

@gtoast
Copy link

gtoast commented Dec 9, 2015

I just had a pull request merged where I called Math.floor() on decimal values passed into get(). set() and get() should probably be consistent in their handling, whatever we do.

I saw that you called ceil() in that devicePixelRatio patch. What was you reasoning? I chose floor() because I figured if you took the conservative guess you have less of a chance going past your array length. But I'd be happy to change that for a more compelling reason.

@toolness
Copy link
Member Author

toolness commented Dec 9, 2015

Oh cool! Does that mean this issue is fixed and we should close it? Which PR was it?

I saw that you called ceil() in that devicePixelRatio patch. What was you reasoning?

Good question--my reasoning behind using ceil() in #1052 was that devicePixelRatio was used to determine how many HTML5 Canvas pixels to use for every "p5 pixel" on a p5 canvas. For instance, if you're on a retina display where devicePixelRatio is 2, then createCanvas(100, 100) actually makes a 200x200 HTML5 Canvas and uses CSS to resize it to 100x100 logical pixels. (And IIRC, set() actually draws four pixels on that HTML5 Canvas, not one.)

So, that said, my thinking was that if we want the p5 canvas to look as crisp as possible, it's better (albeit more expensive) to act as though the user has a slightly higher-density display than they actually do and then allow the browser to downscale it accordingly via CSS, instead of the other way around (in which case the browser would be upscaling via CSS, potentially making things look a bit blurry).

So that was my thinking, but I could easily be mistaken, so let me know if you disagree!

@lmccart
Copy link
Member

lmccart commented Dec 16, 2015

hey @toolness, regarding your questions about set...
I think it does make most sense to round down, and @gtoast's pr #1169 does this for get() so I'll go ahead and do this for set() to.
point() vs set() is a bit redundant, although set() can do more than just set one pixel. actually, if you are just trying to color one pixel, performance wise it would be faster to use point() because it just adds to the canvas, rather than requiring the loading and updating of the whole pixels array. I'm not sure the best way to communicate this in the documentation, but please feel free to take a stab at it if you like!

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

3 participants