Rotation is broken

[Ralle]

These are all expected to return the same vector back but rotate them 45 degrees instead.

(new Victor(1,1)).rotateBy(0)
(new Victor(0,1)).rotateBy(0)
(new Victor(0,-1)).rotateBy(0)
(new Victor(-1,-1)).rotateBy(0)

[epmoyer]

Ralle,

Good catch! I dug into your issue and the problem is both that:

1. The rotate functions do not match their documentation (per http://victorjs.org/)
2. .rotateBy() is incorrectly implemented by any estimation.

Victor implements three (radian) rotation functions, each of which has a “Deg” (degrees) counterpart:

.rotate()
.rotateTo()
.rotateBy()
.rotateDeg()
.rotateToDeg()
.rotateByDeg()

Let’s ignore the last 3 “Deg" variants and focus on the first three, comparing the web documentation to the actual implementation. In all 3 cases angles are CCW from +X axis.

.rotate(angle) :
Documentation says: Rotates the vector to a certain angle
Actual Implementation (as of v1.1.0) is: Rotates the vector by angle

.rotateBy(rotation):
Documentation says: Rotates the vector by a rotation angle
Actual Implementation (as of v1.1.0) is: Rotates the vector by rotation plus the vector’s current angle. Note that this is not a useful behavior, and appears to be a flawed implementation.

.rotateTo(angle):
Documentation says: (not documented)
Actual Implementation (as of v1.1.0) is: Rotates the vector to point in the direction angle

I’d also make the following observations:

1. You stated that "These are all expected to return the same vector back but rotate them 45 degrees instead.”. In fact, you’ll find (if you are using 1.1.0), that the examples you gave are rotating by 0 plus the vector's original angle. so:

   (new Victor(1,1)).rotateBy(0)
   Victor { x:1.1102230246251565e-16, y: 1.414213562373095} // rotated by 45 deg
   (new Victor(0,1)).rotateBy(0)
   Victor { x:-1, y: 6.123233995736766e-17} // rotated by 90 deg
   (new Victor(0,-1)).rotateBy(0)
   Victor { x:-1, y: -6.123233995736766e-17} // rotated by -90 deg
   (new Victor(-1,-1)).rotateBy(0)
   Victor { x:-1.1102230246251565e-16, y: 1.414213562373095 } // rotated by -135 deg

2. The test case for .rotateBy() doesn’t detect the error because it takes the vector (100,100), requests a 45 degree rotation, and expects the result (-100,100), but that is the wrong expected answer (it represents a 90 degree rotation). The correct expected answer would be approximately (0, 141.42)

CONCLUSIONS
At present:

• .rotateTo() does what the documentation says .rotate() should do
• .rotate() does what the documentation says .rotateBy() should do
• .rotateBy() is broken. It’s behavior is deterministic, but it does not preform a useful function.

RESOLUTION
There is a decision to make here. The behavior of the methods .rotateTo() and .rotate() does not match their documentation. Max should decide what he wants to do, but I’d suggest changing the documentation to match the code, rather than making a code change which could break existing projects that use Victor.

Therefore I would suggest the following actions:

1. Change the documentation for .rotate() to read:
   .rotate(rotation), Rotates the vector by a rotation angle, given in radians CCW from +X axis.
2. Change the documentation for .rotateBy() to list it as an alias for .rotate()
3. Code .rotateBy() to be an alias for .rotate()
4. Add documentation for .rotateTo, reading:
   .rotateTo(angle), Rotates the vector to a certain angle, in radians CCW from +X axis.
5. Given the previous confusion, the test cases and documentation examples for the rotate functions would be better served by each having 2 test cases and 2 examples to clearly disambiguate (and validate) their behavior.

All of the above follows for the 3 “Deg” variants as well. They have the same issues.

IMPLEMENTATION
If Max approves of the resolution steps above I’ll put together a pull request for the code.
I don’t know how the web documentation gets maintained. I don’t see it in the repo.

Cheers

[Ralle]

Wow. What a nice answer. You rock.

Just to help Max make his decision. My need was to be able to rotate a vector incrementally, so rotating it 0 would not rotate it at all.

[epmoyer]

Ralle,

Thanks! In the meantime, you'll find that you can use .rotate() instead of .rotateBy(), and it will do what you were originally looking for.

[jackgeek]

+1 Just lost about 2 hours on this before finding this issue!

[wooki]

Can't believe this is still an issue three years on - just wasted half a day narrowing down the issue before eventually deciding it must be a problem in these functions and finding this issue. Great answer epmoyer thanks!

Who maintains the docs?

[a-robu]

@epmoyer that's great investigative work!

It seems the documentation is currently unmaintained, but I found the source code for the webpage in the gh-pages branch of this repo and forked it.

I fixed the documentation issue in my fork and it's now online at https://a-robu.github.io/victor/.