Godot: Make vectors and such on Mono use properties to be idiomatic

[PJB3005]

Right now, the C# glue code like vectors replicates the GDScript API closely, which isn't exactly idiomatic. Methods such as Length() should be made into properties, so they can be accessed as such:

var vec = new Vector2(5, 5);
GD.Print(vec.Length)

For examples of this, see OpenTK's Vector2 type or Unity3D's.

There is the concern that this might make them harder to use if their API doesn't match the GDScript one (since there's no tabs in the API ref to change to a different language) but I doubt that's a significant point since they're already a different naming convention, and you have to be a masochist to code C# without Intellisense of any kind in 2017.

[RabbitB]

…and you have to be a masochist to code C# without Intellisense of any kind in 2017.

I'm sure it's pretty low on the list of things that need to be done, for obvious reasons, but one day we're going to have to take a serious tackle at implementing an Intellisense equivalent for GDScript. The current code-completion in the editor is unfortunately a bit lack-luster. Especially for someone coming from the C# development world, where we have the likes of ReSharper and Roslyn's built-in code-analysis and refactoring.

Of course, you're much more limited in what you're capable of reliably inferring in a duck-typed language, but when static-typing for GDScript comes around, maybe something can be put together.

[vnen]

The different naming convention is one thing, it can be sorted out automatically and it's easy to convert in your head from the reference to the code. Now which methods in GDScript should be made into properties in C#? They would have to be marked as such in the ClassDB so the conversion works and the documentation text should be careful to not mention it as a method so it doesn't look odd to C# users. And Length() for instance is quite costly since it does not cache but calculates the value each time, it's clearer that it's an operation if it remains a method.

since there's no tabs in the API ref to change to a different language

That's a problem to be solved, documentation will eventually have tabs, so this is a moot point.

In any case, I would want to hear @neikeq's opinion on this topic.

---

@RabbitB I understand your point, but it is out of the scope for this issue's discussion.

[NathanWarden]

I agree with @vnen here. Properties are good when they do very little work, but getting the magnitude/length of a 3D vector requires 2 additions, 3 multiplications and a square root. Giving the impression that this value is simply stored in vec.Length instead of making it apparent that it must b calculated via vec.Length() is probably not a good idea. It's better to make it clear that it needs to be calculated.

[NathanWarden]

Just to quickly add to what I just said, unless I'm mistaken, a 3D vector length calculation costs about 200 CPU cycles on a typical CPU. It's okay to mask some functions with properties, but I think this is a case where I would personally avoid it :)

[RayKoopa]

I like how the introductionary post doesn't mention SharpDX which uses methods for this.

Let's take a moment and recap what Microsoft's official C# guidelines say about this and apply it to Length / Length() for example:

• [ ] Consider using a property if the member represents a logical attribute of the type. For example, BorderStyle is a property because the style of the border is an attribute of a ListView.
• [ ] Do use a property, rather than a method, if the value of the property is stored in the process memory and the property would just provide access to the value.

Do use a method, rather than a property, in the following situations:

• [x] The operation is orders of magnitude slower than a field set would be. If you are even considering providing an asynchronous version of an operation to avoid blocking the thread, it is very likely that the operation is too expensive to be a property. In particular, operations that access the network or the file system (other than once for initialization) should most likely be methods, not properties.
• [ ] The operation is a conversion, such as the Object.ToString method.
• [ ] The operation returns a different result each time it is called, even if the parameters do not change. For example, the NewGuid method returns a different value each time it is called.
• [ ] The operation has a significant and observable side effect. Note that populating an internal cache is not generally considered an observable side effect.
• [ ] The operation returns a copy of an internal state (this does not include copies of value type objects returned on the stack).
• [ ] The operation returns an array.

1:0 for methods. The fact that there are cycles burned for retrieving Length is still valid for something simpler and faster like LengthSquared.

[PJB3005]

@RayKoopa I actually never used SharpDX, so I didn't know about that. Fair point.

On the point of "it should a method following Microsoft's official guidelines": the fact that they mention stuff such as IO and asynchronous makes it seem to me they're still talking about a few more orders of magnitude more than calculating the length of the vector.

[RayKoopa]

That doesn't invalidate that it's slower than a field set would be.

[NathanWarden]

@PJB3005 I understand wanting to make it look more friendly and feel more like a variable, but appearances can be deceiving :)

Calculating the length of a vector is pretty heavy, after looking a bit at current answers as to how many cycles each calculation takes, it looks like my estimation I posted above was a bit high, but it still takes about 100 cycles on average to calculate the length of a vector simply because it uses a square root (this applies to both 2D and 3D since they both use 1 square root). MS's guidelines for using a property includes storing a value for later use then passing it directly through the property.

Do use a property, rather than a method, if the value of the property is stored in the process memory and the property would just provide access to the value.

A Vector3 doesn't do that, it must calculate the value (50-100 cycles) and then pass it. Storing and retrieving a value takes 1 or 2 cycles. So, MS says to use a method if it "is orders of magnitude slower than a field set would be", therefore according to MS Guidelines it should be a method since it's about 50x slower than retrieving the stored value.

Ignoring MS guidelines for a moment, a property looks and is coded in the same way a field is, therefore a person inexperienced with the API might expect this to be as fast as a field, and then wonder why their code is so slow. If they're calling a method they will expect it to do work and maybe take more time than a field when they call it.

Real world example: Unity has a vertices property on the Mesh class. I was using it to manipulate the verts of a subdivided flat plane. If I remember correctly, the code looked something like this:

``` C#
for (int i = 0; i < mesh.vertices.Length; i++)
{
Vector3[] verts = mesh.vertices;
verts[i] = GetNewVertPosition(i);
mesh.vertices = verts;
}

This code will run extremely slowly, because while it looks like Unity is just going to pass me the vertex array so that I can manipulate it, this isn't what it actually does. Instead it copies the entire array into a new one, then passes the new one. So every time I check the `Length` property it copies the entire array into a new one and then gives me the size of the array, then in the line where I set the new position of the vertex it copies the entire array so that I can manipulate a single element of it. So if this were a 10000 vert array (which is what I think it really was as it was a 100x100 flat plane, haha) it would have copied the 10000 elements 20000 times. *edit: That's 200,000,000 cycles slower assuming the CPU can retrieve and store the new value in a single cycle. This is an obvious example, but what about where it makes a not so obvious difference? It's much more difficult to spot the problem. It's better to avoid the performance issue from the start by making it clear where performance bottlenecks might be, by making them methods.* I would have written my code completely differently had Unity used the methods GetVertices and SetVertices instead of using a property. This is how I would have written it from the very start:

``` C#
Vector3[] verts = mesh.GetVertices();

for (int i = 0; i < verts.Length; i++)
{
     verts[i] = GetNewVertPosition(i);
}

mesh.SetVertices(verts);

I would have copied the 10000 elements 1 time.

[PJB3005]

Yeah you're definitely right it's a lot slower, and the Unity example is extreme (and I agree it's bad design). I didn't really think much of simple stuff like a vector's Length though, and figured it'd be nice to have.

[RabbitB]

Is there a reason why we don't cache the length or other properties, and only recalculate if the data they rely on has been altered since the property was last read?

This is probably a stupid question as there's likely a very good reason why it isn't done, but it seems like the much more performant way to go about it.

[PJB3005]

@RabbitB first of all, changing the size of the vector structs would make it painful to use with FFI, because now you have multiple extra fields. Also results in more memory usage.

Second of all, the bookkeeping of tracking that would make simple operations more expensive and annoying. It's much better to just store the length manually in intensive code.

[NathanWarden]

@RabbitB I don't think that's a bad question at all and I even asked myself the same question. I think the clearest reason would simply be that:

A) You can cache it yourself (edit: by using the GetHashCode function, if it changes then you're vector has changed)
B) It will add more overhead if you never actually needed caching in the first place. Branching can be expensive. This would add two forms of branching: 1) x, y, and z would now have to be properties, which are actually methods and methods are a form of branching. 2) It would now need an if statement in the Length method/property to check whether it's dirty to recalculate it.
C) Chances are if you're checking the Length of it on a regular basis it's likely because you know that it's changing in your game.

So, unless the common case was that the Length would stay the same most of the time, caching would actually be adding a lot of overhead. Especially if you're checking the distances of a lot of 3D nodes every frame.

[neikeq]

Also worth the mention: System.Numerics.Vector3.Length is implemented as a method.

[migueldeicaza]

Despite the .NET design guidelines, my preference for it being a property is rooted in the fact that visualizing the data with a debugger would render the value for Length, but would not do that for Length(). So it is quite convenient when debugging.

Lastly, this is an idiom that is handled gracefully by the LLVM backend of Mono (which I would hope you would use when you are releasing to production), which on modern CPUs produces:

    mulsd   %xmm0, %xmm0
    mulsd   %xmm1, %xmm1
    addsd   %xmm0, %xmm1
    xorps   %xmm0, %xmm0
    sqrtsd  %xmm1, %xmm0

Which is pretty decent, and would additionally be inlined.

[RayKoopa]

For debugging purposes, a DebuggerTypeProxy or DebuggerDisplayAttribute would IMHO still be the better choice before changing code just to get the debugger to show what is useful. I'm not sure if every debugger supports it, though. However, especially with the code posted above, it should not matter too much here.

[aaronfranke]

Aside from design guidelines, one functional argument in favor of Length being a property would be the ability to make a setter for it. Some people may find it useful to write:

myVector.Length = 5;

Rather than:

myVector = myVector.Normalized() * 5;

But of course there are other good arguments against it mentioned above, such as consistency with GDScript & core, making its slow calculation speed apparent through the design, and the fact that it isn't common to do it that way. Neikeq pointed out System.Numerics uses a method, and so too does MonoGame, Xenko, Microsoft's XNA. Unity uses a property but doesn't make use of the setter advantage that I mentioned above.

[RayKoopa]

That just visually looks like a hack to me. There are infinite ways to get a vector to have a length of 5, and it's not clear which way is used there (you could guess it, but don't write guesswork features). I'd really prefer clarity over shortcuts there.
(And then again you can still add extension methods and soon properties if you really want that)

[aaronfranke]

I'm closing this issue since this needs discussion in a proposal. If you would still like to see this API in Godot, consider opening a proposal on the godot-proposals repository to write out the justifications and gather community feedback.