Test Vector2D.rotate with more numbers and fix oversight
ClosedPublic
Actions

Authored by elexis on Oct 7 2017, 12:13 AM.

Details

Reviewers

FeXoR

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Summary

As reported by mimo in rP20263, the rotate function changes the x coordinate while needing the original x coordinate in the second statement.
This wasn't found by the test in rP20262 because that coincidentally still works for 180° rotations.
So the test should tap more angles.
Other angles weren't tested before because the approximations of our sin and cos functions are lossy.
So we have to allow an error margin if we want to reveal the bug in the tests.

Test Plan

Compare the numbers with https://upload.wikimedia.org/wikipedia/commons/4/4c/Unit_circle_angles_color.svg

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 3338
Build 5780: Vulcan Build (Windows)	Jenkins
Build 5779: Vulcan Build	Jenkins
Build 5778: arc lint + arc unit

Event Timeline

elexis created this revision.Oct 7 2017, 12:13 AM

elexis edited the summary of this revision. (Show Details)Oct 7 2017, 12:14 AM

elexis added a reviewer: Restricted Owners Package.

FeXoR accepted this revision.Oct 7 2017, 12:48 AM

This revision is now accepted and ready to land.Oct 7 2017, 12:48 AM

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2104/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...

http://jenkins-master:8080/job/phabricator_lint/577/ for more details.

Harbormaster completed remote builds in B3338: Diff 3850.Oct 7 2017, 1:14 AM

fatherbushido mentioned this in rP20263: Readability improvements of globalscripts vector.js..Oct 7 2017, 8:41 AM

(if you want you can rename vector.js in Vector.js (we decided that somewhere, but I was always afraid to do that)).

binaries/data/mods/public/simulation/components/tests/test_Vector.js
70	I guess you considered using TS_ASSERT(Math.abs(expectedVector[s] - computedVector[s]) > 0.00000001) but you wanted that formated error?

mimo added inline comments.Oct 7 2017, 10:24 AM

binaries/data/mods/public/simulation/components/tests/test_Vector.js
70	Not really needed as everything seems to be already covered, but we could also check that vector.dot(vector.rotate(pi/2)) < epsilon

Indeed TS_FAIL was used to specify the format:

Expected ({angle:1.0471975511965976, x:0.5, y:0.8660254037844386}) got ({x:0.49999999721223665, y:0.8660254030820543})

TS_ASSERT only shows this:

../../../source/test_setup.cpp:135: Error: Test failed: Stack trace:
@simulation/components/tests/test_Vector.js:68:1
Assert failed

I will add the dot test case and revert the rotate function completely (besides let/var) to not compute the sin/cos twice.
Perhaps someone like me still finds a way to break the rotate function to be correct for 0 to PI but wrong for > PI, but not really seeing the need to extend the unitCircle numbers further.

Wheather we use Math.pow, Math.square or x * x should be estimated independently, as it seems possible to further improve the performance of that Math function.

Thanks for the reviews.

Fixed by rP20272. Thanks for the reviews and comments!

I know from rP20496 that angles are weird and start at north = 0 and move clockwise, so maybe that's why this function rotates things clockwise as well.

binaries/data/mods/public/globalscripts/vector.js
69	Unless I've gone insane (it is late at night), this is rotating clockwise.
binaries/data/mods/public/simulation/components/tests/test_Vector.js
64	Guessing -angle because angle failed the test?