Add static versions of existing mutable vector functions #5044

weslord · 2021-02-14T21:42:03Z

Resolves #4852

Changes:

Updated functions, docs and unit tests for:

PR Checklist

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

welcome · 2021-02-14T21:42:04Z

🎉 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!

weslord · 2021-02-14T21:56:52Z

An ongoing branch for development of the static functions and supporting materials.

The existing test suites for p5.Vector take dramatically different approaches from one test to another (eg: assert vs expect, calling createVector vs overwriting v.x, v.y & v.z), so it is unclear what convention I should be following. Since I will be editing a lot of these tests anyways in the course of this PR, I'll do my best to get them to a more uniform standard. I've used my best judgment (for example, trying to remove dependencies on values initialized in previous suites), but I have very little experience writing tests for JS, so I would appreciate any guidance or feedback in that particular area.

limzykenneth · 2021-02-15T12:20:50Z

For unit tests you can start by looking at here and if there's anything missing or unclear (whether to use assert or expect for example) I think an issue can be filed to either edit the contributor docs or refactor the test code.

codecov-commenter · 2021-07-06T19:01:53Z

Codecov Report

Merging #5044 (c8527ee) into main (bb4ba44) will increase coverage by 0.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5044      +/-   ##
==========================================
+ Coverage   68.09%   68.57%   +0.47%     
==========================================
  Files          79       79              
  Lines       12286    12327      +41     
  Branches     2720     2733      +13     
==========================================
+ Hits         8366     8453      +87     
+ Misses       3920     3874      -46

Impacted Files	Coverage Δ
src/math/p5.Vector.js	`84.07% <100.00%> (+1.04%)`	⬆️
src/core/main.js	`78.81% <0.00%> (-8.43%)`	⬇️
src/core/transform.js	`59.67% <0.00%> (-3.04%)`	⬇️
src/webgl/p5.RendererGL.Immediate.js	`51.53% <0.00%> (-2.37%)`	⬇️
src/core/shape/vertex.js	`63.35% <0.00%> (-1.73%)`	⬇️
src/webgl/light.js	`88.46% <0.00%> (-1.39%)`	⬇️
src/image/loading_displaying.js	`72.72% <0.00%> (-1.25%)`	⬇️
src/webgl/p5.RendererGL.Retained.js	`68.42% <0.00%> (-0.55%)`	⬇️
src/image/filters.js	`19.17% <0.00%> (-0.52%)`	⬇️
src/dom/dom.js	`60.21% <0.00%> (ø)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb4ba44...c8527ee. Read the comment docs.

weslord · 2021-07-09T22:52:57Z

This is a bit of a looong one, but I've done my best to keep each individual commit mostly clean and self-contained, so it may be easier to review this commit-by-commit.

This should implement class/static methods for all outstanding Vector functions, except for .toString(). Since Function.prototype.toString() already, overriding it on p5.Vector has some potential side-effects. I've put those changes in #5348 so they can be discussed separately.

Qianqianye · 2022-12-14T01:33:29Z

Thank you @weslord. I am inviting more Math stewards to review this PR @limzykenneth, @jeffawang, @AdilRabbani @davepagurek.

davepagurek

Just going through and reading everything! Overall this looks good, just a few comments!

davepagurek · 2022-12-14T02:28:34Z

src/math/p5.Vector.js

+ * Calculate the angle of rotation for this vector (only 2D vectors).
+ * p5.Vectors created using <a href="#/p5/createVector">createVector()</a>
+ * will take the current <a href="#/p5/angleMode">angleMode</a> into
+ * consideration, and give the angle in radians or degree accordingly.


Tiny nitpick, for here and line 2466: this should probably be "degrees" instead of "degree"

Oh, I see this is actually a typo in the existing docs for the instance methods too 😅 Maybe worth fixing there too, although it's not urgent for this PR

davepagurek · 2022-12-14T02:42:57Z

test/unit/math/p5.Vector.js

-      expect(v3.angleBetween(v4)).to.be.closeTo(180, 0.00001);
-    });
+      test('should not trip on rounding issues in 2D space', function() {
+        v1 = new p5.Vector(-11, -20);


Was there a reason to switch to new p5.Vector() here over myp5.createVector()?

Since this is a bit of an old PR I would suggest for us to just test changing these back to using myp5.createVector() and see if it is working. If that works fine, we can revert this locally then merge this PR so as not to hold things up for much longer.

Half the file already used new p5.Vector(), and half used myp5.createVector() (one of the many inconsistencies that inspired this query: #5044 (comment)).

My preference is new p5.Vector() because it seems more pure and less stateful. If myp5 has some influence, we probably don't want that. If it doesn't, why use it? Right?

But ultimately it's not a strong preference. I've checked "allow edits by maintainers". Make whatever changes you want.

I don't have a strong opinion either way either, I just wanted to make sure there wasn't anything that wasn't working when using createVector. I think since the docs mostly describe using createVector, it's maybe best that we make sure we test that primarily, although it's also good to know that new p5.Vector works too!

Thanks for making the other updates, I'll pull your branch and double check that createVector works.

Just confirmed that createVector works locally! I'll merge your PR as-is though, you're right that the tests are currently very inconsistent with what form it uses, but it does seem to use new p5.Vector slightly more in the tests, so this adds a bit more consistency (and we can always discuss standardizing on something else in another issue.)

Accidentally hit the wrong button!

davepagurek · 2022-12-14T13:30:42Z

test/unit/math/p5.Vector.js

+      v1 = new p5.Vector(1, 2, 3);
+    });
+
+    suite('p5.Vector.prototype.magSq() [INSTANCE]', function() {


Thanks for making tests for these methods too!

davepagurek

Thanks again for making these changes!

davepagurek · 2022-12-17T00:14:30Z

@all-contributors please add @weslord for doc, test

allcontributors · 2022-12-17T00:14:38Z

@davepagurek

I've put up a pull request to add @weslord! 🎉

weslord added 4 commits February 14, 2021 10:56

Create src/math/README.md with p5.Vector.js description

dec7fe5

Minor cosmetic improvements to p5.Vector.js test module

1a5d6db

Create tests for static version of p5.Vector.rotate

b115d4b

Group all vector rotate tests together and cleanup

9f07a3a

weslord added 4 commits February 15, 2021 10:36

Add class method p5.Vector.copy()

afd27dd

Create tests for p5.Vector.copy()

1fbb44a

Create tests for p5.Vector.mag() and p5.Vector.prototype.mag()

dd92548

Add class method and tests for p5.Vector.magSq()

c8527ee

weslord added 10 commits July 6, 2021 12:39

Add class method and tests for p5.Vector.limit()

8cc4393

Add class method and tests for p5.Vector.setMag()

dfd10c9

Add class method and test for p5.Vector.heading()

6a025ea

Clean up existing tests and docs for p5.Vector.prototype.angleBetween()

f5869d5

Add class method and tests for p5.Vector.angleBetween()

a9b4322

Minor cleanup for Vector docs

27bc1fd

Add class method and tests for p5.Vector.reflect()

4ad3d25

Add class method and tests for p5.Vector.array()

7a76676

Add class method and tests for p5.Vector.equals()

20b976c

Merge branch 'main' into static-vector

d60822c

weslord marked this pull request as ready for review July 9, 2021 22:38

weslord mentioned this pull request Jul 9, 2021

Add class method and tests for p5.Vector.toString() #5348

Open

3 tasks

Qianqianye requested review from davepagurek and limzykenneth December 14, 2022 01:34

davepagurek previously approved these changes Dec 14, 2022

View reviewed changes

davepagurek reviewed Dec 14, 2022

View reviewed changes

weslord added 2 commits December 16, 2022 03:36

Pluralize degree accordingly

35f7d9a

Merge branch 'main' into static-vector

fe1427f

davepagurek approved these changes Dec 16, 2022

View reviewed changes

davepagurek merged commit f825b7a into processing:main Dec 16, 2022

allcontributors bot mentioned this pull request Dec 17, 2022

docs: add weslord as a contributor for doc, and test #5904

Merged

Uh oh!

Add static versions of existing mutable vector functions #5044

Add static versions of existing mutable vector functions #5044

Uh oh!

Conversation

weslord commented Feb 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

welcome bot commented Feb 14, 2021

Uh oh!

weslord commented Feb 14, 2021

Uh oh!

limzykenneth commented Feb 15, 2021

Uh oh!

codecov-commenter commented Jul 6, 2021

Codecov Report

Uh oh!

weslord commented Jul 9, 2021

Uh oh!

Qianqianye commented Dec 14, 2022

Uh oh!

davepagurek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

limzykenneth Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

weslord Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 16, 2022

Choose a reason for hiding this comment

Uh oh!

davepagurek Dec 14, 2022

Choose a reason for hiding this comment

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

davepagurek commented Dec 17, 2022

Uh oh!

allcontributors bot commented Dec 17, 2022

Uh oh!

Uh oh!

weslord commented Feb 14, 2021 •

edited

Loading

davepagurek left a comment •

edited

Loading

weslord Dec 16, 2022 •

edited

Loading