-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
checking for pixel array reassignment inside updatePixels #5127
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
base: main
Are you sure you want to change the base?
Conversation
@divyamahuja I might be doing something wrong here but with the following code, I still wasn't able to assign to function setup() {
createCanvas(1280, 720);
loadPixels();
}
function draw() {
// for(let i=0; i<pixels.length; i++){
// pixels[i] = 255;
// }
pixels = pixels.map((pixel) => {
return 255;
});
updatePixels();
} The |
@limzykenneth I guess, I totally ignored the fact that global mode |
What do you mean by that? |
@limzykenneth I meant in this way. As the hacky patch I applied checks if we are in global mode and acting upon instance of p5.Renderer2D, it checks for value of pixels array attached to globalThis( that is window ) |
@limzykenneth now that I think, should I reassign global pixel array reference to newly create ImageData object (for performance improvement) (After experimenting, I guess there ain't any need for this) |
I'm currently tied up with something so couldn't review this yet, I'll trr to get to it next week. Do have a look at how the codebase handles variables attached to the global scope (or instance) and see if we can emulate it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested out the code and it works from what I can test. Performance-wise I didn't see any significant difference between reassignment and no reassignment of pixels
, perhaps a bit more garbage collection activity but not significant enough to suggest any noticeable slowdown in practice.
Just one comment inline, not super important, just a bit of micro-optimizing 😝
Would you be able to write some unit tests that goes with the changes here?
src/core/p5.Renderer2D.js
Outdated
if (pixels.length === pixelsState.imageData.data.length) { | ||
imgData = new ImageData(pixels, pixelsState.width, pixelsState.height); | ||
} else { | ||
let imgArr = Array.from(pixels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be more efficient to use the TypedArray's set()
method to get the array to the right size, like the one shown here. So instead of creating an array and a Uint8ClampedArray, we just create one Uint8ClampedArray of the right size and copy the existing data over.
); | ||
} | ||
} else { | ||
if (Array.isArray(pixels)) { | ||
pixels.length = pixelsState.imageData.data.length; | ||
let imgArr = new Uint8ClampedArray(pixelsState.imageData.data.length); | ||
imgArr.set(pixels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the original implementation for this is simpler but if you just want to keep things consistent then I guess this should be fine as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I thought it'll be fine this way too, if we are doing the same above. :D
As for unit tests, I checked and I think no unit tests exist for updatePixels()
, so I guess I will write some.
Thanks @divyamahuja for working on it and @limzykenneth for reviewing. Just to follow up on this PR, is it ready to be merged after we resolve the conflicts? |
Resolves #4993
Changes:
checking for pixel array reassignment inside updatePixels and creating new ImageData object accordingly, keeping optimizations related to copying in mind (like if map() is used the resulting UInt8ClampedArray can directly be used by ImageData Constructor and no copying of data will be needed ).
Screenshots of the change:
PR Checklist
npm run lint
passes