search

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.73k stars 3.34k forks source link

loadPixels(x,y,w,h) ignores arguments #2539

Closed diwi closed 6 years ago

diwi commented 6 years ago

arguments are not passed to the subsequent renderer.loadPixels().

https://github.com/processing/p5.js/blob/master/src/image/pixels.js#L521

I feel like I have seen similar things a before but didnt keep track of it, so this is just a hint for a general check. Here is the updated version.

p5.prototype.loadPixels = function() {
  this._renderer.loadPixels.apply(this._renderer, arguments);
};
lmccart commented 6 years ago

the base loadPixels() call does not take any arguments: https://p5js.org/reference/#/p5/loadPixels if loadPixels() is called on the GL renderer directly, the arguments are taken into consideration. https://github.com/processing/p5.js/blob/master/src/webgl/p5.RendererGL.js#L614 i think this is correct behavior? @mlarghydracept?

stalgiag commented 6 years ago

I think that was intentional back when I did it but I don't know if it makes sense in hindsight. My personal instinct is to say that it makes more sense to change it such that it always just pulls the whole canvas into pixels. It is already forcing the pixels array to be the size of the full canvas on this line. So the utility of grabbing a fragment becomes mostly non-existent.

lmccart commented 6 years ago

for simplicity sake, that makes sense to me. we already have so many issues around pixels, pixel density, and canvas sizes. let's not complicate further if it's not adding a whole lot.

diwi commented 6 years ago

I had experimented a bit with loadpixels in this object-picking demo:

https://www.openprocessing.org/sketch/496458

You'll notice every couple of frame some stuttering (a ~150ms pause) which has to do with how loadPixels() is implemented. At first I thought it had to do with the amount of pixels to transfer from the gpu to the app, which is a costly operation and therefore it would be nice to have the x,y,w,h-arguments to defined a rectangle.

However I now had a closer look at the implementation, and now i think it's the GC. Loadpixels allocates a new array for the full frame every call, instead of reusing the same buffer.

Edit: Demo is updated with custom readPixels() to get rid of the framerate stuttering.

stalgiag commented 6 years ago

Good point! The data should be transferred directly from the gpu to the p5 instance's pixels array.