Created fix for camera settings getting reset each time resizeCanvas() is called

[RandomGamingDev]

Created fix for camera settings getting reset each time resizeCanvas() is called

Resolves #6203.

Changes:

Screenshots of the change:

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[welcome]

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

[RandomGamingDev]

This is the solution for issue #6203.

[davepagurek]

Thanks for taking this on!

Do you think it's equivalent to take out the else branch here?

p5.js/src/webgl/p5.Camera.js

Lines 1615 to 1626 in d2b5d5e

	_resize() {
	// If we're using the default camera, update the aspect ratio
	if (this.cameraType === 'default') {
	this._computeCameraDefaultSettings();
	this._setDefaultCamera();
	} else {
	this.perspective(
	this.cameraFOV,
	this._renderer.width / this._renderer.height
	);
	}
	}

If it is equivalent, it might be better to make the code change there to avoid doing some work then undoing it

[davepagurek]

Also, just a heads up, looks like we get some linter errors currently:

[RandomGamingDev]

About the resize function, tbh I'm not very sure, but they don't look like they do the same things is all of I have to say ig lol.
About linting not running through it properly sorry about that. I made sure that the code was of the same style, but wasn't able to check directly since lint was complaining about the different line endings.

[RandomGamingDev]

I combined my previous work with your advice inaridarkfox4231 and now orbitControl() seems to work fine.

[RandomGamingDev]

custom is also applied for perspective(). I wanted to add the part where it only updates the projection matrix if the camera is "custom" (aka has had its projection matrix manually modified by some p5.js function) since davepagruek recommended that I add it so that the camera, if not of type custom, will continue to dynamically update to match the screen dimensions.

[RandomGamingDev]

lol dw it's fine, I didn't realize that there was a problem with orbitControl() since I typically just deal with the matrices and didn't see the other values getting interacted with much. I should've probably thought about cases like that.

[davepagurek]

Just thought of a few more things to consider, one is that Framebuffers also have a default camera that gets resized here, so we probably want to implement this in both spots:

p5.js/src/webgl/p5.Framebuffer.js

Lines 677 to 685 in 63bea68

	this._deleteTexture(oldColor);
	if (oldDepth) this._deleteTexture(oldDepth);
	const gl = this.gl;
	if (oldColorRenderbuffer) gl.deleteRenderbuffer(oldColorRenderbuffer);
	if (oldDepthRenderbuffer) gl.deleteRenderbuffer(oldDepthRenderbuffer);
	this._recreateTextures();
	this.defaultCamera._resize();
	}

Also, if we copy the camera and set this._renderer._curCamera = cam with the copy, users who have stored their camera in a local variable might be confused when editing their camera no longer affects how their sketch rendered.

About the resize function, tbh I'm not very sure, but they don't look like they do the same things is all of I have to say ig lol.

The p5.Camera resize method is the one responsible for overwriting the properties we want to persist, so I was thinking maybe we can edit that to not overwrite them? i.e. just update the aspect ratio in the projection matrix? Just a suggestion though.

One other thing, it would be great to add a unit test -- maybe something like, make a canvas, set frustum(), then resize it, and get the pixels; and then make a second canvas already at that final size, set frustum() with the same parameters, and assert that the pixels are the same between the two.

[RandomGamingDev]

tbh I didn't know that p5.Framebuffer was part of the p5.js library lol. I'll implement smth for that when I can. Also yeah, I sorta forgot about the outside reference to camera part. Originally I only copied the matrices because of that, but ig I just forgot. I'll make it so that it just copies the attributes later. I think I'll probably just create a class and iterate over each of the key value pairs in order to write them. Their attributes like the matrices however would have to have their references replaced which would be a bit annoying if someone were to have cached those tho lol, altho at this point basically unavoidable. I'll try to write smth for the unit test too when I can.

[davepagurek]

I think it's ok to replace the matrix properties, we also haven't publicly documented their existence, so I think it's fair game to modify that without it causing a breaking change 🙂

[RandomGamingDev]

lol it's fine. also yeah ig it'd probably be fine since they rn't documented.

[RandomGamingDev]

I'll be a bit busy, but I'll add those to the pull request when I can.

[RandomGamingDev]

The problem right now is indeed about the behavior of the camera, but the problem with the camera is the fact that resizeCanvas() not only resizes the canvas, but also the camera and is the issue that's trying to be addressed here although yes, we'd probably want this in other resize functions that reset the camera too.
Also yes, resizeCanvas() is in both 2D and WebGL mode, which is why we're checking for whether the camera's frustum/projection matrix has been previously modified so that we can keep compatibility with the traditional canvas.
Yes, it'd be more flexible, but if someone who hasn't messed around with p5.js before and especially for someone new to programming ran into the camera resetting which requires your own custom fix they'd probably be at least decently annoyed and might just quit the project, since the whole idea of implementing some code separate from your own code that interacts with the p5.js code that interacts with the graphics goes against the idea of abstraction and making programming with the library accessible to more people.
The use case as displayed in issue #6203 is just resizeCanvas() resetting the camera since having the camera reset every time you say, resized the window of a dynamically resizing canvas would be a problem.
Adding the camera resizing feature to p5.Framebuffer.js will happen. I'm just a bit busy rn.
Also yeah, we're gonna have to deal with the other cameras since they do still get reset.
tbh I should've probably been trying to find the root cause of why the CPU values are getting reset by a canvas change that should only be affecting the GPU values. I'll try to find that out later when I can. Hopefully that'll be able to address the root cause so we don't have worry about all of this since otherwise we'd be forced to just copy every single camera every single time which would be painful.

[RandomGamingDev]

I fixed the Resolves #[Add issue number here]. Also, I discussed with davepagurek and he seems to agree that it's a bug. For an example, a lot of programs oftentimes end up using the camera to store their values for movement among other things and even those that don't most likely don't cache them separately outside of the camera. For a 3d application like a game or any other sort of software a simple resize could easily break their code base. Plus, even if we were to say that since the WebGL uniforms reset so everything else should reset too, if you look at the function p5.js is clearly making an effort to cache and restore canvas values, so it's probably more so of a bug that they managed to do it for basically everything except for the camera.

[RandomGamingDev]

As for creating a sketch for this, I feel like it's unnecessary. I could create a sketch demonstrating a moving camera that gets reset upon the window getting resized due to wanting to resize with the window, but that'd only display the same thing as what I said. If u mean getting a full project that has a problem with this, I feel that that's unnecessary.

[RandomGamingDev]

I'm confused how you wouldn't have understood the change and it's reason, but here's a duplicate of the sketch used originally in case you want it: https://editor.p5js.org/PotatoBoy/sketches/7NVWhsA7P.

[RandomGamingDev]

The issue that I attempted to solve was the same one as before, where resizing of the camera resets the camera. Also, sorry for the late responses and the lack of commits recently. I've been busy.

"But nothing happens if you don't call _resize(), so you can't fix the problem about that."
My response afterwards was regarding why the camera behaved this way in the first place as stated previously, since, the resetting of the uniforms relating to the WebGL context on the GPU typically wouldn't effect values relating to it on the CPU, but for some reason both get reset which I will do more research into when I can.

[RandomGamingDev]

As for moving the camera, I believe that only the commands affecting the projection matrix turn the camera to type "custom".

[RandomGamingDev]

The reason for checking for custom in the previous commit originally was because some people wouldn't care about the camera as you've restated.

[inaridarkfox4231]

I have understood well that I cannot help you. thank you very much.
I'll leave the rest to davePagurek.

[davepagurek]

btw @RandomGamingDev I made a little sketch here to test out what it feels like to use a fixed camera distance vs a fixed field of view (what we currently do.)

Screen.Recording.2023-10-25.at.6.33.11.PM.mov

Live: https://editor.p5js.org/davepagurek/sketches/TC3kWHOWk

It looks like using a fixed camera distance has some benefits actually? There are less changes when you resize the camera, so that's arguably a plus. And then for our purposes, it means we would only need to update the perspective matrix for default perspective cameras when the window size changes.

Here's another quick sketch to verify that if you make a camera, resize the canvas, then click the play button to restart with the camera in that new size, it looks visually the same as it did after starting at a different size + resizing:

Screen.Recording.2023-10-25.at.6.39.25.PM.mov

Live: https://editor.p5js.org/davepagurek/sketches/CTKUAAmMZ

[RandomGamingDev]

I'll probably create another pull request to add more complex features like that and just use this pull request to fix the issue with as little as possible and as close to the issue itself. That way other people can more easily chip in and bring up their thoughts on the changes.

[davepagurek]

I think this looks good to go now! Thanks for your patience and the long review process for this one, it ended up being a more complicated issue than we though 😅