-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
implementation of p5.Vector slerp function #6222
implementation of p5.Vector slerp function #6222
Conversation
Implements vector slerp function. Performs spherical linear interpolation with the vector of arguments and returns a new vector. It does not change the original vector.
some unit tests for slerp function.
Change to behavior similar to cross().
use res vector to check
I'm checking to see if it returns a new vector, but I don't understand why I'm getting an error... |
I saw a similar unit test with cross(), but this calculation takes the cross product with (0,0,0), so the result is (0,0,0). So, I thought that the check was done not by object equality, but by component equality. var res;
setup(function() {
v.x = 1;
v.y = 1;
v.z = 1;
});
test('should return a new product', function() {
expect(v.cross(new p5.Vector())).to.not.eql(v);
}); In fact, even if they are the same object, they are different vectors if they have different components. In my test, I check with a vector of the same component, so it seems that even if it is created with copy(), it will be regarded as the same vector. I don't know why... |
Eliminate this because the test for returning a new object is not working.
Since cross() returns the result with this.copy(), it will be different from the original object, so I thought this test was checking it, but I may be misunderstanding. |
I thought about various things, but I still think that the specification that changes the original vector is better, so I would like to rewrite it that way. |
…changes Instead of creating a new vector, let the vector affected by the function change.
Due to the specification change, some unit tests have been changed. Also added detailed tests.
With this specification, for example, if you want to change v, just write v.slerp(w, amt), and if you don't want to change it, just write p5.Vector.slerp(v, w, amt), so it can more accommodate flexibly. |
benchmark test: |
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.
Great work on this! My only comments are about making the documentation a tad clearer.
src/math/p5.Vector.js
Outdated
* translate(50, 50); | ||
* | ||
* const theta = Math.atan2(mouseY-50, mouseX-50); | ||
* const v = createVector(50 * Math.cos(theta), 50 * Math.sin(theta)); |
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.
It might be a little clearer here if we do const v = createVector(mouseX-50, mouseY-50).setMag(50)
without the extra trig?
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.
It's true that shorter is better, so I'll fix it!
src/math/p5.Vector.js
Outdated
* const v4 = p5.Vector.slerp(v1, v2, t); | ||
* const v5 = p5.Vector.slerp(v2, v3, t); | ||
* const v6 = p5.Vector.slerp(v3, v1, t); | ||
* translate(v4.x, v4.y, v4.z); |
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.
For this example, I wonder if we can make what's happening a bit clearer:
- Since the spheres end in a configuration that looks identical to the start, it might accidentally look like the vectors keep moving, maybe coloring them differently and/or making them oscillate back and forth instead of jumping back to the start would make it easier to see what's happening?
- Although it's less lines of code, subtracting the previous point from the next might add a bit of confusion. Maybe we could push/pop even though it's longer? or use something like
line()
where we can use coordiantes withouttranslate()
?
As a quick test, do you think something like this is clearer? https://editor.p5js.org/davepagurek/sketches/NBexafWPU
function draw(){
background(255);
const t = map(sin(frameCount/30), -1, 1, 0, 1);
// v1, v2, v3 is not changed by slerp function.
// because this function is static version.
const v4 = p5.Vector.slerp(v1, v2, t);
const v5 = p5.Vector.slerp(v2, v3, t);
const v6 = p5.Vector.slerp(v3, v1, t);
strokeWeight(10)
strokeCap(SQUARE)
stroke('red')
line(0, 0, 0, v4.x, v4.y, v4.z);
stroke('green')
line(0, 0, 0, v5.x, v5.y, v5.z);
stroke('blue')
line(0, 0, 0, v6.x, v6.y, v6.z);
}
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 thought that example might be a little confusing for me too. It's true that "cylinders" or "lines" are easier to understand, so I'm going to rewrite them. I would like to adopt the "lines" because it is easier for the code to understand.
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.
How about this...?
slerp sample for 3D
function setup(){
createCanvas(100, 100, WEBGL);
}
function draw(){
background(255);
const vx = createVector(30, 0, 0);
const vy = createVector(0, 30, 0);
const vz = createVector(0, 0, 30);
const t = map(sin(frameCount * TAU / 120), -1, 1, 0, 1);
// v1, v2, v3 is not changed by slerp function.
// because this function is static version.
const vSlerpXY = p5.Vector.slerp(vx, vy, t);
const vSlerpYZ = p5.Vector.slerp(vy, vz, t);
const vSlerpZX = p5.Vector.slerp(vz, vx, t);
strokeWeight(6);
strokeCap(SQUARE);
stroke("red");
line(0, 0, 0, vSlerpXY.x, vSlerpXY.y, vSlerpXY.z);
stroke("green");
line(0, 0, 0, vSlerpYZ.x, vSlerpYZ.y, vSlerpYZ.z);
stroke("blue");
line(0, 0, 0, vSlerpZX.x, vSlerpZX.y, vSlerpZX.z);
}
Renamed variables to make it clearer which direction the axis is pointing.
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.
looks great!
src/math/p5.Vector.js
Outdated
/** | ||
* Performs spherical linear interpolation with the other vector | ||
* and returns the resulting vector. | ||
* The result of slerping between 2D vectors is always a 2D vector. |
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.
By only mentioning 2D vectors in the description, I worry people might incorrectly assume it's only for 2D. Maybe we can say "This works in both 3D and 2D" before this sentence?
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.
Spherical linear interpolation is usually used in 3D, so I didn't mention it, but I would like to describe it if necessary (I wanted to emphasize that the result is 2D even if each 2D vectors are pointered directions oppositely).
Thank you for your review! I will try to respond. |
For the examples, I've made it more clear what you're doing. Also added to the documentation that it can be used in 3D.
remove trailing space
use singleQuote
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.
Thanks for making these changes!
Thanks for review and merge! ('ω') I'm looking forward to seeing people using this function... |
Implements vector slerp function.
Performs spherical linear interpolation with the vector of arguments and returns a new vector. It does not change the original vector.
The lerp is designed to be changed, but unlike the lerp, I felt that it was common to use the same vector, so I decided to do so.
Resolves #6220
Example:
3D slerp test
2023-06-19.00-46-39.mp4
PR Checklist
npm run lint
passes