Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ahujadivyam
Copy link
Contributor

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

@lmccart lmccart requested a review from limzykenneth March 29, 2021 02:47
@limzykenneth
Copy link
Member

@divyamahuja I might be doing something wrong here but with the following code, I still wasn't able to assign to pixels.

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 for loop still works.

@ahujadivyam
Copy link
Contributor Author

@limzykenneth I guess, I totally ignored the fact that global mode pixels was just another reference, so intenal reference was not getting reassigned and code was not working. But I just applied a hacky patch, so let me know what are your views about it. It may not work if someone tries to alter internal pixels array, but for general use case where one is altering just pixel array reference , it will work fine

@limzykenneth
Copy link
Member

It may not work if someone tries to alter internal pixels array

What do you mean by that?

@ahujadivyam
Copy link
Contributor Author

@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 )

https://editor.p5js.org/DivyamAhuja/sketches/Sd50S3ym-

@ahujadivyam
Copy link
Contributor Author

ahujadivyam commented Mar 30, 2021

@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)
image
This function does the job I guess.

@limzykenneth
Copy link
Member

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.

Copy link
Member

@limzykenneth limzykenneth left a 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?

if (pixels.length === pixelsState.imageData.data.length) {
imgData = new ImageData(pixels, pixelsState.width, pixelsState.height);
} else {
let imgArr = Array.from(pixels);
Copy link
Member

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);
Copy link
Member

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.

Copy link
Contributor Author

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.

@Qianqianye
Copy link
Contributor

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?

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

Successfully merging this pull request may close these issues.

"pixels" array cannot be reassigned
3 participants