cleanup passing of mat4 arguments

[lindapaiste]

Background:

This is just a code cleanup and should not change any behaviors. There is a big block of code for passing the arguments of a 4x4 matrix. It is excessively verbose and it's repeated in 10 different places!

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

Lines 1427 to 1444 in fb70e98

	this._renderer.uMVMatrix.set(
	this.cameraMatrix.mat4[0],
	this.cameraMatrix.mat4[1],
	this.cameraMatrix.mat4[2],
	this.cameraMatrix.mat4[3],
	this.cameraMatrix.mat4[4],
	this.cameraMatrix.mat4[5],
	this.cameraMatrix.mat4[6],
	this.cameraMatrix.mat4[7],
	this.cameraMatrix.mat4[8],
	this.cameraMatrix.mat4[9],
	this.cameraMatrix.mat4[10],
	this.cameraMatrix.mat4[11],
	this.cameraMatrix.mat4[12],
	this.cameraMatrix.mat4[13],
	this.cameraMatrix.mat4[14],
	this.cameraMatrix.mat4[15]
	);

These blocks are one of many issues brought up by @RandomGamingDev in this comment on #6527.

Changes:

Simplifies these blocks of code to:

this._renderer.uMVMatrix.set( 
   ...this.cameraMatrix.mat4.slice(0,16)
)

Using:

• slice(0,16) to access the first 16 element of the array (indices 0 through 15)
• ... (spread syntax) to pass the elements of the array as individual arguments.

Questions:

• Are there every fewer than 16 elements in these arrays? If so then there would be a behavior change because this approach will only pass as many arguments as there are in the array. The current code would pad with undefined in that situation. It's the difference between calling func(a, b, undefined, undefined) and func(a, b).
• Are there always exactly 16 elements in these arrays? If so, the .slice(0,16) is not necessary.
• It seems like the Matrix.set() method accepts an array, in addition to accepting individual numbers. Should I drop the ... and call with the array directly? (But keeping the .slice() to clone it). Or use the .copy() method, as suggested by @RandomGamingDev?

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]

The previous issue

Make sure to reference the issue so that the 2 can be more easily linked in the future for those trying to participate: #6527

Responses

Are there every fewer than 16 elements in these arrays? If so then there would be a behavior change because this approach will only pass as many arguments as there are in the array. The current code would pad with undefined in that situation. It's the difference between calling func(a, b, undefined, undefined) and func(a, b).
Are there always exactly 16 elements in these arrays? If so, the .slice(0,16) is not necessary.

For some reason, although the library is documented to be a matrix class for mat4, there is partial support in multiple areas for mat3 as stated in the previous issue. To be honest, I'm not sure whether this means that we should compensate for that incomplete functionality, divide it up into multiple classes, delete it, or leave it as is. Perhaps @davepagurek could provide more insight into that.

It seems like the Matrix.set() method accepts an array, in addition to accepting individual numbers. Should I drop the ... and call with the array directly? (But keeping the .slice() to clone it). Or use the .copy() method, as suggested by @RandomGamingDev?

I'd recommend calling with the array if you want to keep all of the references to the matrix pointing at the matrix, especially since this can make debugging easier since an abnormally small array is easier to track down than some random undefined values. However, you'd still need to copy the array instead of using the array directly if the array is used anywhere else and since it'd be best not to have a bunch of the "separate arrays" pointing to the same object when debugging.

If you don't care about keeping the references pointing towards the Matrix pointing towards the Matrix then I'd recommend using the .copy() method.

Something that I'd like to say regarding the issue

I'd just like to note here that, while I do support improving the existing p5.Math math library, I still believe, as stated in #6527, that it'd be best if we transitioned to another library to avoid wasting developer time, and to get the best math library to fuel p5.js and its developers through the documentation, performance, code clarity, cleanliness, and features of those other libraries that currently and realistically, won't be in p5.Math.

[lindapaiste]

For some reason, although the library is documented to be a matrix class for mat4, there is partial support in multiple areas for mat3 as stated in the previous issue. To be honest, I'm not sure whether this means that we should compensate for that incomplete functionality, divide it up into multiple classes, delete it, or leave it as is. Perhaps @davepagurek could provide more insight into that.

@RandomGamingDev After creating this PR I dove deeper into the p5 Matrix class and it really is quite a confusing mess. Now I understand all of the points that you are making in #6527 and I can see the logic in using a third party library rather than reinventing the wheel. There is a lot than can be cleaned up either way. I'll leave a comment on the issue but I think a starting point for detangling might be to separate the existing methods into a core Matrix class and two specific Matrix3x3 and Matrix4x4 classes that extend it. For now I think it's probably safe to assume that the matrixes used in the Camera, FrameBuffer, etc. would have the correct number of values.

[RandomGamingDev]

Yeah, it is pretty messy, but it doesn't look like anyone can work on replacing the math library rn. For now, I'd recommend not assuming that it's always the same value and to copy the list in a way that works regardless of its length like some that I mentioned above just in case some use the mat3 functionality.

[davepagurek]

To answer your questions:

• Are there every fewer than 16 elements in these arrays? If so then there would be a behavior change because this approach will only pass as many arguments as there are in the array. The current code would pad with undefined in that situation. It's the difference between calling func(a, b, undefined, undefined) and func(a, b).
• Are there always exactly 16 elements in these arrays? If so, the .slice(0,16) is not necessary.

Right now the mat4 property will only have exactly 16 array elements. 3x3 matrices currently use the mat3 property, which should have exactly 9 elements always if it's defined. We can treat those as invariants right now, especially if that simplifies the code.

• It seems like the Matrix.set() method accepts an array, in addition to accepting individual numbers. Should I drop the ... and call with the array directly? (But keeping the .slice() to clone it). Or use the .copy() method, as suggested by @RandomGamingDev?

A few thoughts on this one:

• It looks like internally, we never call .set(array) or .set(matrix), and only ever use the variant where we pass all the numbers individually.
• Currently, .set(array) and .set(matrix) end up sharing array references and don't copy things for you.
• I suspect we never want to make a new matrix that has a reference to an array that is already used in another matrix so that updating the source p5.Matrix doesn't also accidentally edit the one it was copied to with set().
• Maybe we should update the implementation to do a copy internally so we never end up in that situation? Then, we could just pass in the array directly without a slice() or spread.

Yeah, it is pretty messy, but it doesn't look like anyone can work on replacing the math library rn. For now, I'd recommend not assuming that it's always the same value and to copy the list in a way that works regardless of its length like some that I mentioned above just in case some use the mat3 functionality.

I think it's probably fine to do a quick check in set() to see if the length is what we expect, and throw an error otherwise, if that lets us simplify our implementation.

Yeah, it is pretty messy, but it doesn't look like anyone can work on replacing the math library rn.

Yeah, I'm also not opposed to it, but it kinda needs a "champion" to make it happen. In the mean time working on small chunks to improve the code is good!

For some reason, although the library is documented to be a matrix class for mat4, there is partial support in multiple areas for mat3 as stated in the previous issue. To be honest, I'm not sure whether this means that we should compensate for that incomplete functionality, divide it up into multiple classes, delete it, or leave it as is.

I wonder if this was initially inspired by the native DOMMatrix class, which handles both 3x3 and 4x4 matrices. Anyway, for our purposes, I think it makes sense to pull them apart into two separate classes at some point, maybe with a base class with a more generic implementation, leaving the option open for subclasses to override method implementations with more optimized algorithms if it turns out we really need it.

[lindapaiste]

A few thoughts on this one:

• It looks like internally, we never call .set(array) or .set(matrix), and only ever use the variant where we pass all the numbers individually.
• Currently, .set(array) and .set(matrix) end up sharing array references and don't copy things for you.
• I suspect we never want to make a new matrix that has a reference to an array that is already used in another matrix so that updating the source p5.Matrix doesn't also accidentally edit the one it was copied to with set().
• Maybe we should update the implementation to do a copy internally so we never end up in that situation? Then, we could just pass in the array directly without a slice() or spread.

I think this is a good idea so I did it😄. The set function now copies the values from the source. We can now pass in the matrix that we want to copy, like this:

this._renderer.uMVMatrix.set(this.cameraMatrix)

This is not ready for merging just yet -- there are some test failures which I need to investigate.

[RandomGamingDev]

Other than the test failures this looks good to me

[lindapaiste]

This is not ready for merging just yet -- there are some test failures which I need to investigate.

Ok so it turns out the the failing tests were not due to any changes that I made. I get the same failures when testing the main branch.

I see the cause of the problem so should I fix it? I think yes, but in a separate PR. All of the tests related to the camera _orbit method fail with TypeError: _main.default.Vector.cross is not a constructor because the code uses improper syntax new p5.Vector.cross(up, front). cross is a static method so there shouldn't be a new there.

[davepagurek]

Great work @lindapaiste! Thanks for also adding some error logging for when our input assumptions aren't met!