Refactor p5.Vector to prioritize smaller dimension in binary operations on mismatched-size vectors#8676
Refactor p5.Vector to prioritize smaller dimension in binary operations on mismatched-size vectors#8676ksen0 wants to merge 19 commits intoprocessing:dev-2.0from
Conversation
| } | ||
| if (this instanceof p5) { | ||
| return new p5.Vector( | ||
| this._fromRadians.bind(this), |
There was a problem hiding this comment.
This is done only in p5.Vector.
Continuous ReleaseCDN linkPublished PackagesCommit hash: 32e351a This is an automated message. |
| assert.isAbove(mockUserError.mock.calls.length, 0, 'FES.userError should have been called'); | ||
|
|
||
|
|
||
| assert.isAbove(mockUserError.mock.calls.length, 0, 'FES.userError should have been called, btw: '+globalThis.FESCalled); |
There was a problem hiding this comment.
Hi @davepagurek ! I don't know why this is happening but some tests that as far as I can tell should not be affected are not passing:
I checked the behavior and errors are emitted correctly.
I did do an FES mocking in p5.Vector.js unit tests like this:
globalThis.FESCalled = false;
globalThis.p5 = {
_friendlyError: function(msg, func) {
globalThis.FESCalled = true;
console.warn(msg);
}
};Though I don't think it's related.
There was a problem hiding this comment.
Are those getting reset correctly when the tests finish?
There was a problem hiding this comment.
as a quick test, if you skip all those vector tests, do you see the strands tests start to pass?
There was a problem hiding this comment.
@davepagurek hmmm good question, no must be the code. Same errors with:
include: [
//'./test/unit/math/p5.Vector.js',
'./test/unit/webgl/*.js',
],
| suite('new p5.Vector(1,2)', function () { | ||
| beforeEach(function () { | ||
| v = new mockP5.Vector(1, 2, undefined); | ||
| v = new Vector(1, 2); |
There was a problem hiding this comment.
@limzykenneth another substantive test case change; fails because I've added number validation to Vector based on my understanding of our earlier discussion
|
|
||
| test('should set values to [0,0,0] if values array is empty', function () { | ||
| test('should NOT set values to [0,0,0] if values array is empty', function () { | ||
| v.values = []; |
There was a problem hiding this comment.
This is a substantive change reflecting intent of the update
| let vect = new mockP5.Vector(1, 2, 3, 4); | ||
| assert.equal(vect.getValue(5), 1); | ||
| let vect = new Vector(1, 2, 3, 4); | ||
| globalThis.FESCalled = false; |
There was a problem hiding this comment.
@limzykenneth this and another fails test stopped passing when I mocked FES, I think maybe they were not failing for the right reason originally? Anyway, here's a proposed update to reflect my understanding of the intent of the setValue/getValue tests
ksen0
changed the title
ksen0
changed the title
3 participants
Note: this is a draft!
Because it's a major change, this needs more work and feedback. It is shared as a draft open for comment
In 2.x, vectors can be of different dimensions. Binary operations should not be done on vectors with mismatched dimensions; however, for backwards compatibility and to avoid breaking older code that might have edge cases (resulting from how 1.x handled 2D vectors), we decided to apply the logic of "always use the smaller dimension" when operating on two vectors with mismatched size. Note that solo numeric arguments (such as
createVector(1,2,3).mult(2)) are still supported.To discuss on discord, please see the "vector-discussion" channel in "Code" category. Prior community discussion which led to this is documented in these meeting notes. One important consideration here was that switching from 2D to 3D can benefit from a bit of intentional scaffolded friction: code should not completely break, but it's okay if it looks a bit wrong, because doing binary operations on mismatched vectors is not actually supported, and should not be done.
PR Checklist
npm run lintpasses