p5.Camera unit tests #3083

AidanNelson · 2018-07-13T16:39:05Z

This adds unit tests for the p5.Camera object as added in #3080.

I wasn't quite sure how to create tests for projection matrices without simply recreating the ortho() and perspective() functions in the test, so any feedback on that would be appreciated!

kjhollen · 2018-07-13T22:40:29Z

hey @AidanNelson, instead of replicating the ortho/perspective/etc functions in the unit tests, what you'll do instead is determine the expected values for each entry of the matrix and hard code them. in theory, the 'expected' values are generally known ahead of time and not computed—things that are easily unit-testable have predictable results.

this feels hard for me to explain, somehow—if this doesn't make sense, we can set up some time on google hangouts.

kjhollen

Hi Aidan, this is a good start. I think the ideas/goals of unit testing take a bit of practice to set in, and seeing this gives me a good idea of where you're at and where to go next. I've left some comments in line and thought it would be good to share some general comments, too:

in some cases, because the camera is a complex object, you may want to check that additional values are set

for most functions, you'll want to test them with a variety of inputs, especially if there are any edge cases in the code or you can think of some possible gotchas in the matrix math. the tests in math/calculation.js have some good examples for this—the distance function is an especially good one to look at.

kjhollen · 2018-07-23T21:54:46Z

test/unit/webgl/p5.Camera.js

+    });
+
+    test('SetPosition() should set eyeXYZ', function() {
+      var pos = Math.random();


generally speaking, unit tests should be deterministic. a known value should be applied as an argument to a function, then hardcoded values should be tested. instead of testing a true random set of numbers, it's good to be systematic about what types of numbers you're testing.

some things we might consider here:

are any or all of the values positive? covers a case where the code might have special cases for >0 or <0

are any or all of the values negative?

are any or all of the values 0? all 0s is a good case to test with matrix math because it means the magnitude of the supplied vector is 0, which can be trouble in some calculations

is there a mix positive/negative/0?

are all the values the same, or is each one different? as written, this test would not find a bug where the first argument is accidentally applied for all three (this is something I have done before!)

what if a non-number value is passed as one of the arguments?

are any other values updated as a result of setPosition that should be checked in their own assertions as part of this test? the assertions will let you provide some more detail in an error message, so you can figure out what if any conditions are not met.

kjhollen · 2018-07-23T21:55:17Z

test/unit/webgl/p5.Camera.js

+      myCam.lookAt(23, 23, 23);
+      assert.strictEqual(myCam.centerX, 23);
+      assert.strictEqual(myCam.centerY, 23);
+      assert.strictEqual(myCam.centerZ, 23);


are other values (up, eye) expected to change here? if so, we should test their values.

kjhollen · 2018-07-23T22:05:39Z

test/unit/webgl/p5.Camera.js

+          myCam.projMatrix.mat4[i],
+          myp5._renderer.uPMatrix.mat4[i]
+        );
+      }


in the same test, we should check if the expected values are in the matrix with additional assertions. you can add an argument to the assert call to print a more helpful error so it's obvious if the error was that the matrices didn't match or that they didn't have the expected values (applies to next test, too)

kjhollen · 2018-07-23T22:07:15Z

test/unit/webgl/p5.Camera.js

+                     0, -1,   0,  0,
+                     0,  0,  -3, -1,
+                     0,  0, -40,  0];
+


cool. another good value to test is the default! you may want to write a helper function that iterates over two arrays or matrices to check their values—or maybe there's already a chai function that already handles arrays?

Yes, switched to using chai's deepEqual assertion.

kjhollen · 2018-07-23T22:08:15Z

test/unit/webgl/p5.Camera.js

+    test('copy() returns a new p5.Camera object', function() {
+      var newCam = myCam.copy();
+      assert.instanceOf(newCam, p5.Camera);
+      assert.notDeepEqual(newCam, myCam);


should check that the values for both are the same here, to ensure that the copy worked

Added a test for this.

kjhollen · 2018-07-23T22:09:13Z

test/unit/webgl/p5.Camera.js

+      for (let j = 0; j < 3; j++) {
+        assert.typeOf(local.x[j], 'number');
+        assert.typeOf(local.y[j], 'number');
+        assert.typeOf(local.z[j], 'number');


great. are there any other conditions that should be true here? should the axes be normalized? should they be orthogonal?

Yes, those checks are added.

kjhollen · 2018-07-23T22:22:29Z

test/unit/webgl/p5.Camera.js

+    });
+
+    test('perspective() sets renderer uPMatrix', function() {
+      myCam.perspective(Math.PI / 3.0, 1, 1, 100);


is a negative value for the field of view allowed?

Yes, but the view will be reversed! I wasn't sure whether / how to check for this (and a few other) edge cases which produce valid matrices but unexpected views.

AidanNelson · 2018-07-30T14:50:35Z

Ok, @kjhollen I added several unit tests and updated them throughout to ...

set camera defaults in the test-suite setup, in case these default values ever change in the library,
use a hardcoded camera or projection matrix as the expected value,
use chai's deepEqual assertion to iterate over matrix values,
also sometime also check eyeXYZ, or upXYZ so that error messages would be more helpful.

kjhollen

Great job, Aidan! This is looking much better. One thing I was thinking while I read through this was, is there some way you can test this on values you are likely to encounter using orbitControl? especially for the camera() method!

kjhollen · 2018-08-07T04:19:04Z

test/unit/webgl/p5.Camera.js

+  var delta = 0.001;
+
+  // copy of p5.Vector.prototype.angleBetween...
+  var angleBetween = function(a1, a2, a3, b1, b2, b3) {


hmm, I think it ought to be ok to just use the vector version (and make some vectors) when you need this? unless I'm missing something?

Ok. I was trying to keep it isolated, but not for any particular reason!

AidanNelson · 2018-08-07T16:08:18Z

Ok! Once #3088 is merged, I could add a unit test for orbitControl using the p5.Camera.prototype._orbit() method directly (without needing mouse input).

kjhollen · 2018-08-10T01:04:02Z

cool! a nice bonus is that using p5.Vector made the test more legible too :)

AidanNelson added 2 commits July 12, 2018 15:13

p5.Camera unit tests

e7c1c38

create delta variable and add tests

de4d82e

AidanNelson requested a review from kjhollen July 13, 2018 16:39

AidanNelson added 2 commits July 17, 2018 10:11

add perspective and ortho tests

e75ac37

Adjust values for perspective/ortho tests

631f833

AidanNelson added the GSOC label Jul 18, 2018

lmccart added the Area:WebGL label Jul 20, 2018

kjhollen requested changes Jul 23, 2018

View reviewed changes

kjhollen reviewed Jul 23, 2018

View reviewed changes

AidanNelson added 2 commits July 27, 2018 10:45

updates to p5.Camera tests

acc86fb

updated p5.Camera unit tests

cf965f4

kjhollen reviewed Aug 7, 2018

View reviewed changes

use p5.Vector objects for getLocalAxes() assertions

92cf8b3

AidanNelson mentioned this pull request Aug 8, 2018

add p5.Camera.prototype._orbit() unit tests #3129

Merged

kjhollen approved these changes Aug 10, 2018

View reviewed changes

kjhollen merged commit f7fdd10 into processing:webgl-gsoc-2018 Aug 10, 2018

AidanNelson deleted the camera-unit-tests branch August 11, 2018 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p5.Camera unit tests #3083

p5.Camera unit tests #3083

AidanNelson commented Jul 13, 2018

kjhollen commented Jul 13, 2018

kjhollen left a comment

kjhollen Jul 23, 2018

kjhollen Jul 23, 2018

kjhollen Jul 23, 2018

kjhollen Jul 23, 2018

AidanNelson Jul 30, 2018

kjhollen Jul 23, 2018

AidanNelson Jul 30, 2018

kjhollen Jul 23, 2018

AidanNelson Jul 30, 2018

kjhollen Jul 23, 2018

AidanNelson Jul 30, 2018

AidanNelson commented Jul 30, 2018

kjhollen left a comment

kjhollen Aug 7, 2018

AidanNelson Aug 7, 2018

AidanNelson commented Aug 7, 2018

kjhollen commented Aug 10, 2018

p5.Camera unit tests #3083

p5.Camera unit tests #3083

Conversation

AidanNelson commented Jul 13, 2018

kjhollen commented Jul 13, 2018

kjhollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AidanNelson commented Jul 30, 2018

kjhollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AidanNelson commented Aug 7, 2018

kjhollen commented Aug 10, 2018