Added example and Unit tests for ClampToZero method on p5.Vector. #7199
Conversation
Forchapeatl
changed the title
src/math/p5.Vector.js
Outdated
| @@ -3901,6 +3901,22 @@ p5.Vector = class { | |||
| * @method clampToZero | |||
| * @return {p5.Vector} with components very close to zero replaced with zero. | |||
| * @chainable | |||
| * @example | |||
| * <div"> | |||
There was a problem hiding this comment.
do we want div> instead of div"> .... in the second one we have extra quotation marks
src/math/p5.Vector.js
Outdated
| * <div"> | ||
| * <code> | ||
| * function setup() { | ||
| * // Create a new vector |
There was a problem hiding this comment.
this comment can be made more useful, by saying that we are creating a vector with components having small numerical value
src/math/p5.Vector.js
Outdated
| * <code> | ||
| * function setup() { | ||
| * // Create a new vector | ||
| * let v = createVector(0.0000000000000002220446049250313, 5 ); |
There was a problem hiding this comment.
why are we using such a detailed number can't we use 0.000....02
do we need all those digits ?
There was a problem hiding this comment.
same value is used in tests as well,
it is ok if this value is kept, you can make that decision
src/math/p5.Vector.js
Outdated
| * // Clamp components close to zero to zero | ||
| * v.clampToZero(); | ||
| * console.log('After:', v.x , v.y); | ||
| * describe('Round down very small numbers to zero'); |
There was a problem hiding this comment.
I think this can be improved by saying that we round down the value of components of a vector,
you can use your own words, basically this function doesn't round down small number, this is for a vector, we can make that clearer
| expect(v.y).to.equal(0); | ||
| }); | ||
|
|
||
| test('should clamp very small numbers in all components of a 3D vector to zero', function() { |
There was a problem hiding this comment.
I like how you mentioned that we are clamping "components of a vector", I think it would be awesome if other tests followed the same wording
ie instead of saying that we are rounding down the number we can say rounding down the component of a vector
|
Than you for the review @ashish1729 . I have made the updated changes, please have a look at my PR. |
src/math/p5.Vector.js
Outdated
| * | ||
| * console.log('Before:', v.x , v.y); | ||
| * | ||
| * // Clamp negigabe value of x-component to zero |
There was a problem hiding this comment.
I am sorry I don't know this word 'negigabe' , can you please check if this is something intentional
|
thank you for fixing all these, although I am not a maintainer, I don't think I can/should approve changes. |
|
No worries @ashish1729 , I really needed someone to put me on track. Thank you. |
| * console.log('Before:', v.x , v.y); | ||
| * | ||
| * // Clamp negligible value of x-component to zero | ||
| * v.clampToZero(); | ||
| * console.log('After:', v.x , v.y); |
There was a problem hiding this comment.
If you run grunt yui:dev, the user won't be able to see the log values on the reference page. I think they have to open their browser developer tools to see that.
| test('should clamp very small negative number of vector components to zero', function() { | ||
| v = new p5.Vector(-0.0000000000000002, 5); | ||
| v.clampToZero(); | ||
| expect(v.x).to.equal(0); | ||
| expect(v.y).to.equal(5); | ||
| }); |
There was a problem hiding this comment.
I believe we use Math.abs((val || 0) - 0) <= Number.EPSILON ? 0 : val in our clampToZero function, so there's no need to check for negative values. It seems like we're testing whether Math.abs() works correctly rather than testing our p5.js function.
| test('should not clamp regular numbers of vector components', function() { | ||
| v = new p5.Vector(0.01, 5); | ||
| v.clampToZero(); | ||
| expect(v.x).to.equal(0.01); | ||
| expect(v.y).to.equal(5); | ||
| }); |
There was a problem hiding this comment.
Here as well, it looks like we are checking for the same test and we are specifically testing for Number.EPSILON . What I think we can remove all the tests and can just keep a single big test as you have a test at last.
something with :
v = new p5.Vector(
0.00000000000000005,
-0.0000000000000002220446049250313,
0.0000000000000002220446049250313);
With these three values, we can create different combinations to verify if clampToZero() works correctly. Since unit tests can be time-consuming to run, this would be a kind of optimization.
Let me know what you think ;)
|
Keep up the great work. Some thoughts, let me know what you feel. :) Here how it shows when you run
This could be resolved by two solution.
and by writing some description for eg;
Let me know what you think by these thoughts :) |
|
thanks @perminder-17 , I will work on this over the weekend |
Resolves #6750
Changes:
Screenshots of the change:
PR Checklist
npm run lintpasses