Immutable vectors

[jcornaz]

• [x] ImmutableVector2
• [ ] ImmutableVector3

---

Hi,

Thank you very much for providing theses utilities for Kotlin :-)

I would like to propose to introduce some immutable classes for at least vector and colors. (could be called KVector2 or ImmutableVector2 for instance)

In order to allow easy usage with LibGDX they could either:

• inherit from their equivalent in LibGDX, deprecate all mutation method and throw if a mutation method is called
• provide adapter like ImmutableVector2.asVector2() and Vector2.toImmutableVector2()

Why? Even in Java I find error-prone that classes like vector or color are mutable. It is for instance possible to change by mistake Vector2.ZERO, and it is a mistake really easy to do. But I think that's even more true in Kotlin where the language provide good support for immutability (val is the default, data class provides copy to easily get a new object, and immutable fields can be smart-casted).

Mutable vectors cannot be safely shared, and one has to do defensive copy each time it gets a vectors, which is cumbersome, unsafe, and inefficient.

In short:

• Even in Java it should have been immutable
• In Kotlin, immutable data classes are even more idiomatic

[czyzby]

I think LibGDX sacrifices safety for conciseness and performance - the difference might not be noticeable on desktop, but it does somewhat matter on mobile.

That being said, immutable vectors and colors would be a great addition and might prevent some non-obvious bugs - but since there is no Vector or Color interface that you could just implement, the conversion code interacting with existing LibGDX APIs could quickly become tedious. We might also have to reimplement all (or most) methods, or at least delegate them to the original, mutable objects.

@jcornaz Would you like to try implementing such immutable classes or is it a feature request?

[jcornaz]

I think LibGDX sacrifices safety for conciseness and performance - the difference might not be noticeable on desktop, but it does somewhat matter on mobile.

Yes, it does. However as you said it is less true on desktop. And I think it may even actually lead to worse performances, as it is dangerous to share the mutable vectors mutable different threads. Making performances improvement through multi-threading almost impossible.

Would you like to try implementing such immutable classes or is it a feature request?

This is a feature request. But I don't mind trying to implement it myself and propose a PR.

[czyzby]

I'd be happy to review the PR, thanks.

[jcornaz]

What would you think about the following design:

data class ImVector2(val x: Float = 0f, val y: Float = 0f) {
  val isZero get() = x == 0f && y == 0f

  // immutability allow some performance improvements as well, by making able to cache results:
  val len2 by lazy { (x * x) + (y * y) }
  val len by lazy { sqrt(len2) }

  // [...] Other relevant members

  companion object {
    val ZERO = ImVector2()
    val X = ImVector2(x = 1f)
    val Y = ImVector2(y = 1f)
  }
}

// Conversion from/to LibGDX vectors:
fun ImVector2.mutable(): Vector2 = Vector2(x, y)
fun Vector2.immutable(): ImVector2 = ImVector2(x, y)

// Operators as extension functions
operator fun ImVector2.plus(vector: ImVector2): ImVector2 =
  ImVector2(x = x + vector.x, y = y + vector.y)

fun ImVector2.lerp(target: ImVector2, alpha: Float): ImVector2 {
  val invAlpha = 1f - alpha
  return ImVector2(
    x = (x * invAlpha) + (target.x * alpha),
    y = (y * invAlpha) + (target.y * alpha)
  )
}

// [...] Other operators written as extension functions

I'am only talking about design. Of course I'll make more operators, and I'll do it for Vector3 as well. Colors may come in a separate PR.

[czyzby]

Not sure about the name of the class. We could either go with a verbose ImmutableVector2, or a less obvious KVector2. I'm leaning towards KVector2, what do you think?

Lazy fields might be too much of an overhead - instead of just 2 float fields, each vector object would hold a ton of float wrappers if we go that way. Most available vector operations just aren't heavy enough to justify the cache. Let's trust the user to save the computation results.

Some extra dynamic properties (like the length) are OK, but I think the API should have all of the original methods from Vector2 to avoid confusion and work as a drop-in replacement.

Kotlin built-in conversion methods usually start with to, so I think toImmutable and toMutable names would be preferable. Maybe we should also add a concise way to convert an immutable vector into Vector2 for compatibility - like invoke(): Vector2, which would allow to do this: val vector: Vector2 = myImmutableVector(). It isn't obvious, though, so I'm not quite sure about this.

[jcornaz]

Not sure about the name of the class. We could either go with a verbose ImmutableVector2, or a less obvious KVector2. I'm leaning towards KVector2, what do you think?

I was not sure either. I like KVectorN except it doesn't carry the immutability concept. However this may not be a problem as the mutable structure should have an explicit name, not the immutable one. I'm fine with any naming here. I'll start with KVectorN and we'll chose the final naming when reviewing the PR.

Lazy fields might be too much of an overhead - instead of just 2 float fields, each vector object would hold a ton of float wrappers if we go that way. Most available vector operations just aren't heavy enough to justify the cache. Let's trust the user to save the computation results.

I don't want to add everything in lazy properties. Only the expensive ones, like len which has to compute a square-root. Other properties could either be computed at construction time or evaluated at each call. And for the properties to cach, I may measure actual performance benefit (with JMH) to make sure it is worth caching them.

Some extra dynamic properties (like the length) are OK, but I think the API should have all of the original methods from Vector2 to avoid confusion and work as a drop-in replacement.

Yes everything available from LibGDX vectors should be available. But is it ok if is it available as extension functions? Or would you put it in the class?

Kotlin built-in conversion methods usually start with to, so I think toImmutable and toMutable names would be preferable

Yes. But in this case it should be named toImmutableVectorN and toMutableVectorN shouldn't it? Then it would fit better in Kotlin conventions, but it would be more verbose. I'm ok with both solutions.

Maybe we should also add a concise way to convert an immutable vector into Vector2 for compatibility - like invoke(): Vector2, which would allow to do this: val vector: Vector2 = myImmutableVector(). It isn't obvious, though, so I'm not quite sure about this.

I think toMutableVector is enough. And actually that's why I thought about simply naming them mutable() and immutable() so it would be less verbose to do : val vector: Vector2 = myImmutableVector.mutable()

[czyzby]

Only the expensive ones, like len which has to compute a square-root.

Each lazy property is an extra object created for each vector, regardless of whether you use that particular expensive method or not. Caching in this case might be a premature optimization of a problem that won't affect most use cases. Feel free to write benchmarks, but I think the memory cost is just not worth it - IMHO, vectors should be as close to value classes as JVM (currently) allows.

But is it ok if is it available as extension functions? Or would you put it in the class?

Either way works. I think keeping them in the class might be more readable.

Yes. But in this case it should be named toImmutableVectorN and toMutableVectorN shouldn't it? Then it would fit better in Kotlin conventions, but it would be more verbose. I'm ok with both solutions.

I feel like toMutable is a good trade-off between conciseness and Kotlin conventions. Personally, when I look for conversion methods, I instinctively write to and wait for the auto-completion to do the job.

[jcornaz]

Actually, does it make sense to have immutable vectors without having immutable matrix and quaternions?

I'm starting to think that adding everything would be too much. And maybe this feature request is too much against the concept: "make LibGDX as Kotlin-friendly as possible without turning the API upside down"

Although I'd love to have immutable matrix, vectors and quaternions.

[czyzby]

Immutability is encouraged in Kotlin, as you've said. As long as using the immutable variants is optional and doesn't get in the way, I don't see why we wouldn't add them. I'd only suggest to use delegation for complex methods rather than try to reimplement every single Vector/Matrix operation. It's OK to create a mutable instance, perform the operation on it, and convert it into the immutable variant.

[czyzby]

Start with just the vectors, though. I try to write tests for every feature, and I imagine the vector operations alone would require quite a bit of tests. It's also easier to thoroughly review smaller PRs.

[jcornaz]

Should we provide operators overloads accepting LibGDX VectorN as arguments?

For instance on could do val newImmutableVector = immutableVector + mutableVector.

PROS:

• Smoother integration with LibGDX vectors
• Better performances when dealing with mutable vector as it wouldn't be necessary to create an intermediate object.

CONS:

• Add a lot of functions. And they should be tested.
• If necessary one can convert the vectors: val newImmutableVector = immutableVector + mutableVector.toImmutable()

[czyzby]

In times like these, I wish Kotlin had union/structural types.

I could see how that might be very convenient, I'd say we can stick to immutable vector operations and explicit conversions when interacting with mutable variants - at least for now. Unless you feel up to implementing the extra methods.

[czyzby]

@jcornaz Any updates on this? If you started working on this and have some code that we can use to speed up the implementation, feel free to setup a PR.

[jcornaz]

Yes I started it. I want to add some tests before creating the PR.

I'll see if I have time to propose something this weekend

[jcornaz]

I'm basing my tests on the fact that "all my functions should return same value LibGDX Vector2 would have returned for the same given arguments".

However LibGDX returns quite strange results for angles.

// angleRad without argument returns angle from x axis.
println(Vector2(0.0f, 1f).angleRad()) // 1.5707964

// but if one explicitly specify X axis:
println(Vector2(0.0f, 1f).angleRad(Vector2.X)) // -1.5707964

What am I supposed to do here? Should I replicate this inconsistent behavior for the sake of behaving as LibDGX does?

[czyzby]

@jcornaz It does seem like a bug. Try adding an issue to the official LibGDX repo. Although both approaches are a source of potential bugs, I think we should choose correctness over consistency in this case and wait for LibGDX team to fix their implementation.

[jcornaz]

I open an issue for it: https://github.com/libgdx/libgdx/issues/5385

[jcornaz]

I think we should choose correctness over consistency

It's not only inconsistent. It is incorrect as well. println(Vector2(0.0f, 1f).angleRad(Vector2.X)) should print 1.5707964, not -1.5707964. Here -1.5707964 is incorrect.

[czyzby]

Just to clarify, I meant consistency with LibGDX. Reimplementing the bug would be consistent with LibGDX API (and incorrect), but in this case we should sacrifice full LibGDX compatibility and go with the correct solution.

[jcornaz]

This one is quite an edge case case, and is only about consistency:

println(Vector2(-1f, -0f).angleRad()) // -3.1415927
println(Vector2(-1f, -0f).angle()) // 180.0

Here the "problem" is the fact that angleRad and angle don't return the same sign for the same arguments. I don't see any rationale behind that, even if both result are properly speaking corrects.

Of course one may ask what the rationale of having -0.0 in y axis? I'd answer it can happen in real application, when y is computed. For instance KVector.X * -1 will return KVector(-1f, -0f).

Since here LibDGX's result is inconsistant but correct. Should I replicate this inconsistency?

And I'd like to ask the same kind of question for isZero() and isZero(margin). Due to float imprecision, there are edge cases (when playing with Float.MIN_VALUE either in the vector or the margin) where isZero and isZero(margin) doesn't behave consistently. Should I create two separate functions and replicate LibGDX behavior? Or can I use only one function with a default agument, knowing that we'll have a slightly different results for very small vectors)

[czyzby]

You should definitely use methods with default params whenever possible (and sensible). Since there is no interface to implement that you have to conform to, the difference between overloaded methods and methods with default params should be barely noticeable by the users. As long as it simplifies the implementation or maintenance, go for it.

As for the inconsistencies in LibGDX API: it's the KTX goal to smooth out LibGDX rough edges without completely changing the APIs. If you consider this behavior as borderline buggy, feel free to implement it the correct, consistent way. You might also want to create another bug report in their repo. Thanks.