Immutable Vector 2D

[jcornaz]

Contributes to #152

I open this pull request, because I consider ImmutableVector2 to be feature-complete.

Before merge I want to:

• Rename for ImmutableVectorN
• Inline alias-functions
• Add tiny vectors (like (0.001, -0.001)) in the list of vector to test
• Add the missing comments
• Replicate syntax sugar KTX was providing for Vector2
• Update the changelog
• Update math module's readme
• Explicitly test equals and hashCode methods. (It is very important, since most of the tests rely on it)
• Re-read everything / Fix typos
• Add examples to the README section
• Add some test against expected values in order to make sure to detect bugs present in both KTX and LibGDX
• Fix documentation and address requested changes

Open questions and points to notice :

• What name should we choose? I know I proposed it, but I don't like KVector, because the K means nothing, and certainly not "immutable". I think I'd prefer ImVector or ImmutableVector
  • Solution: We'll name them ImmutableVectorN and write in the doc that one can create typealiases for shorter names
• Should the 3D vectors be part of this PR?
  • Solution: Vector 3D will be part of a separate PR
• I intentionnaly chose to make angle() return values between -180 and 180 instead of between 0 and 360 like LibDGX does. I chose that to be consistent with angle(Vector2), angleRad() and angleRad(Vector2).
  • Update: We provide angleDeg functions and deprecate angle to make the difference in behavior clearer and less error-prone
• Two tests are disabled while waiting the resolution of Vector2.angleRad(Vector2) returns angle toward -y (instead of toward +y) libgdx/libgdx#5385
• Most of the function got exact same name as their equivalent in Vector2. Exceptions to this rule are setter and functions which became Kotlin operators:
  • setLength and setLength2 became withLength and withLegnth2
  • setToRandomRotation became withRandomRotation
  • scl became the operator times: v * 3 or v1 * v2
    • unaryMinus is the equivalent of scl(-1f): -v == v * -1
  • add became the operator plus: v1 + v2
  • sub became the operator minus: v1 - v2

[czyzby]

Looking good so far, thank you for working on the tests right away.

KVector does sound non-obvious, but it is Kotlin's convention to prefer immutability and prefix mutable variants with Mutable rather than the other way around. I think it sounds reasonable to make an exception here and name them ImmutableVectorN, with a note in the documentation on how to quickly make an alias - possibly with a few examples:

typealias Vec2 = ImmutableVector2
typealias KVector2 = ImmutableVector2

Should the 3D vectors be part of this PR?

If you consider 2D vector implementation feature complete, I don't see why we wouldn't release a snapshot release with what we've got so far.

[jcornaz]

variants with Mutable rather than the other way around

Yes. However when not possible, even the Kotlin team prefix with Immutable: https://github.com/Kotlin/kotlinx.collections.immutable/tree/master/kotlinx-collections-immutable/src/main/kotlin/kotlinx/collections/immutable

Using proper explicit name and propose the user to create typealiases is a great solution, I love it! Thanks.

[jcornaz]

I have a concern about the angle() function which returns between [-180, +180] instead of [0, 360] like LibGDX does.

I made this choice in order to make it consistent with angle(Vector2), angleRad() and angleRad(Vector2).

But I wonder if I shouldn't rename them angleDeg in order to make it clear that they aren't equivalent to the LibGDX ones.

[czyzby]

I prefer more explicit names either way, so I don't see a problem with angleDeg. Do you want to add a deprecated angle method that invokes angleDeg, so that existing code converted to ImmutableVector works out of the box? It could point out that you should use the explicitly named variant to prevent bugs.

[jcornaz]

Good idea to add a deprecated angle function. It provides explicit explaination and quick-fix from IDE. That's Great! Thanks again.

[jcornaz]

I think I'm done @czyzby.

Please let me know if you find any potential issue, if you want me to add more test, or if you have any question about the code I wrote.

[jcornaz]

Please tell me what you think about the examples I added in the readme.

[czyzby]

Great so far, thanks again. When you fix the issues with the documentation, we can definitely merge it.

[jcornaz]

Quick update to say I didn't forget this PR. I just lack of free time at the moment. I'll try to address the remaining issues next weekend.

[czyzby]

@jcornaz It's OK, you've already done a lot, and I'm also pretty busy recently. I could take over the extra tests, but I'm not sure if I'll be able to make it this week. Just let me know if you won't have time to finish it after all.

[jcornaz]

I finally addressed the requested changes.

In a nutshell:

• I fixed typos and improve documentation as requested
• I tested the examples in readme in order to verify their correctness
• I wrote a bunch of test in order to make sure that it works as expected even if there is a bug in LibGDX.

While writing the new tests, I actually noticed a problem in LibGDX vector when it comes to orientation of the vector zero and with many orientation comparison (hasSameDirection, isOnLine, isCollinear, etc.)

LibGDX is not consistent accross theses functions when the vector zero is involved. I opted for correctness instead of replicating errors of LibGDX. The orientation of a zero vector is undefined therefore:

• angle functions involving a zero vector now return NaN. (instead of an arbitrary angle, like LibGDX does)
• isOnline now return false if one of the vectors is zero. (instead of returning true, like LibGDX does)

Other direction-related functions (isCollinear, isPerpendicuar, etc.) have not been touched, since LibGDX was already behaving correctly for them.

[czyzby]

I'll review the changes and update KTX to LibGDX 1.9.9 in the week after this one after I'm done with a conference and a hackathon. Thanks again for the contribution!

[jcornaz]

Note this PR contains too many commits. I'd recommend to squash before merge. (Either by choosing squash-merge in github, or I may rebase if you prefer)

[czyzby]

But 36 commits would move you to the top of the contribution statistics. ;)

I honestly don't mind either way - the extra commits make it easier to see how the code progressed and normally I'm a fan of "commit early, commit often". Squashed commits make it easier to create changelogs based on the history, but then again - we usually do it manually as the changes are added either way.

[jcornaz]

It is as you prefer.

I like to "commit early, commit often" when working on a feature (which explains my 36 commits^^). But once the feature is merged I no longer care that It took x commits to be written. I only care about the changes made by the feature as whole. And the original commits are never lost with squash-commit anyway. They stay attached to the PR, and may still be anylized independently if needed (but I never came back a specific PR commit personally).

But at the end it's up to you, of course. ;-)

[czyzby]

This is a great contribution, thanks. If you still have the time, please apply some minor changes in the docs and the tests - otherwise we can merge it as is and I'll do a slight refactoring after merging.

[jcornaz]

Thanks for your valuable review @czyzby. I addressed the requested changes.

[czyzby]

I think this is among the biggest contributions to KTX. Thanks for your work and putting up with my nitpicking. :)