Add Vector2.from_angle() method

[KoBeWi]

Resolves 2D part of godotengine/godot-proposals#1719 (someone wanted Vector3 equivalent, but it probably requires some more math so meh)

With this you can do

Vector2.from_angle(PI/2)

and it will return Vector2(0, 1)

(or at least it would if static functions weren't bugged, see #48238)

[aaronfranke]

Doesn't work in my testing:

Missing test cases.

Missing C# version.

[Xrayez]

Missing test cases.

Writing tests is not currently required (unlike documentation), but highly recommended. See also godotengine/godot-proposals#1586.

Missing C# version.

Are there any contribution guides in the documentation to make this happen? I understand that C# is officially supported language, but I'm not sure if it's required for contributors to implement those methods currently. This question also applies to VisualScript, GDNative etc...

As for myself, I'm not using C# at all. Does that mean my own contribution to core would be blocked because of lack of C# knowledge?

[aaronfranke]

@Xrayez It's not necessarily a blocker, but if it's not done here, someone will have to do it later for consistency. It's easier to just do it everywhere at once. If you want to update the documentation, that would be great.

As for VisualScript and GDNative, they get access to methods via variant_call.cpp (C# doesn't do this). I don't know if this includes static methods, but looking at the code, I don't see any special logic for from_hsv...

[KoBeWi]

Doesn't work in my testing:

This is due to #48238.
You need to call it on a vector for now.

Missing test cases.

This is task for #47202

Missing C# version.

Added, but I never compiled mono version, so I can't test it.

[Xrayez]

It's not necessarily a blocker, but if it's not done here, someone will have to do it later for consistency. It's easier to just do it everywhere at once.

Most of the time (if not always), I'd like to test a feature myself before doing changes. If I don't know how to test something, I'd rather avoid doing those changes, unless someone else is willing to test those changes before merging, otherwise this could lead to broken code. So, IMO, it's better that someone else does those changes in a separate PR who know better.

I'd just make it clear that you're willing to test the implementation if you do want to see them in any PR, not just "Missing C# version". I'm just providing my feedback on the review process, sorry. 🙂

[aaronfranke]

The C# code looks good, and the tests can wait. This PR isn't fully functional until #48238 is fixed, so we can either fix that first, or merge this first and use this method as a test for #48238.

[groud]

The idea makes sense, but what you are providing here is simply half of of what would be needed to allow building any vector from polar coordinates.

Consequently, I think it would be more feature-complete to rename the function to from_polar(...) and to allow a second argument (that could be 1.0 by default), that would allowing providing a length too.

[KoBeWi]

The length argument wouldn't be very useful. It's a unit vector, so you can just multiply it.

Also the function was never intended to have anything to do with polar coordinates (which very few people use). from_angle() has similarity to angle() method, which makes it more obvious that the two methods are related, so it's easier to find.

[groud]

The length argument wouldn't be very useful. It's a unit vector, so you can just multiply it.
Also the function was never intended to have anything to do with polar coordinates (which very few people use). from_angle() has similarity to angle() method, which makes it more obvious that the two methods are related, so it's easier to find.

I mean, at least it looks like something that has a mathematical sense. The angle() function itself is basically returning a float out of a vector, without the 3 points usually needed to define an angle. This is simply because it returns the angle part of it's polar coordinates.

I think that, if we implement the possibility to create a vector from an angle, then that's basically equivalent to building a vector from it's polar coordinates (returned by the angle() and lenght() function the other way around). It makes the API more consistent and easier to search.

[aaronfranke]

without the 3 points usually needed to define an angle.

We don't need 3 points. The origin and X axis are implied.

When it comes to polar coordinates, this is just something that is super easy to do with existing methods (angle & length, and from_angle & multiply), while simultaneously being a very niche use case for games, so it doesn't make sense to have APIs for it.

[groud]

The origin and X axis are implied.

Yep, that's basically the issue here. As explicit naming is better than implicit naming, I think from_polar is better than from_angle() as it is more explicit.

[Xrayez]

Compare:

Vector2.from_angle(radians) * length
Vector2.from_polar(radians, length)

I really do think it's not worth being explicit here. I'm for complementing angle() method rather than this.

@groud your opinion on this is basically the complete opposite of what you said in #43103 (comment):

Simply allow generating a random unit vector then let the user multiply it. That's a simple and efficient API.

The from_angle is "simple and efficient" API as well according to your comment. I understand that I'm taking this from other context, but it's very similar to what we have here.

[Xrayez]

Alternative solution could be:

1. Merge this PR as-is;
2. Implement new methods such as:
   • Geometry2D.polar_to_vector(theta, length)
   • Geometry3D.spherical_to_vector(theta, phi, length)

if really needed. Due to this, I've even considered to do the same thing in goostengine/goost#65, where using polar terminology would probably make more sense, since Geometry already implies that we deal with something related to space, contrary to Vector2/3 classes which are not always used in a geometry/spatial context.

Consequently, I think it would be more feature-complete to rename the function to from_polar(...) and to allow a second argument (that could be 1.0 by default), that would allowing providing a length too.

Being feature-complete can mean anything to a user. For instance, are we going to implement other conversion methods such cylindrical coordinate system as well? That would certainly make those features complete, but I'm afraid those methods could bloat Vector2/3 classes with features that nobody is going to use in their game projects.

That's why I'm suggesting that we implement coordinate system conversion methods in classes such as Geometry2D/3D (again, if needed), and merge this PR for the reasons already outlined above by @KoBeWi and @aaronfranke.

[Xrayez]

Surprise! Looks like I've found existing methods which do polar to cartesian conversion and vice versa, I had no idea we have those functions already, and I'm not sure whether you were aware of them either... 054a2ac.

• cartesian2polar
• polar2cartesian

The only problem is that polar2cartesian accepts length as first parameter, so we can't omit it to default 1.0.

That said, since we already have those functions, I humbly think that adding from_angle() still makes perfect sense to have as a Vector2 method to complement angle() and there's no need to rename it to from_polar(), because we already have polar2cartesian() for that as a GDScript method.

Again, quoting KoBeWi in this PR:

Also the function was never intended to have anything to do with polar coordinates (which very few people use). from_angle() has similarity to angle() method, which makes it more obvious that the two methods are related, so it's easier to find.

Cross-referencing those methods in the documentation should be worth it, still.

[reduz]

ready to merge when passes CI. Do not forget to remove polar/cartesian functions from variant_utility in a subsequent PR, as they are redundant and a confusing API for Godot users compared to this one.

[aaronfranke]

Works in my testing and looks good except for one style issue.

[aaronfranke]

Looks good and works in my testing. I haven't tested the C# version but it looks good.

[vnen]

Thanks!