godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Add mutating methods to math types #3019

Open cptchuckles opened 3 years ago

cptchuckles commented 3 years ago

Describe the project you are working on

Working in 3D, I often times want to calculate a new Transform, new Vector3 or (quite often) a new Basis and lerp my global_transform[.basis, .origin] towards the new value.

Describe the problem or limitation you are having in your project

This gets pretty verbose when I always have to write thing = thing.lerp(other_thing):

$Head.global_transform.basis = $Head.global_transform.basis.lerp(new_basis, factor)

I would much rather just type the identifier once, and use a mutating method to change it

$Head.global_transform.basis.mut_lerp(new_basis, factor)

and so on.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I would like to see some of the most common and useful mathematical methods on math types have a mutating version to elide this code verbosity. Yes Spatial has mutating methods like look_at and rotate, but those aren't quite specific enough for the kinds of transformations I often do, like interpolating only the basis, for example, and they don't lerp. I would love to see mut_* wrappers around things like Transform::rotated(), Basis::lerp(), Vector3::reflect() etc. which assign to itself the result of those method calls.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Every new Thing::mut_* method would just assign to itself the result of the normal, value-returning implementation, calling the latter internally. E.g.

void Vector3::mut_slide(const Vector3 &p_normal) const {
    this = slide(p_normal);
}

and so on

If this enhancement will not be used often, can it be worked around with a few lines of script?

Nobody has to use these methods and all the old code will work fine

Is there a reason why this should be core and not an add-on in the asset library?

Methods like these are just part of the engine, so they can't really be an addon

Calinou commented 3 years ago

Nobody has to use these methods and all the old code will work fine

The issue with mutable methods is that they tend to encourage bad practice and "fragile" code. Unless there is a performance requirement to have mutable methods, I think we should stick to having immutable methods only whenever possible.

In date manipulation libraries like Moment.js, having mutable methods is generally considered to be a billion-dollar mistake there :slightly_smiling_face: In fact, many of its replacements like date-fns or day.js are designed to be immutable.

However, methods that return new values like this are a good idea to have: https://github.com/godotengine/godot-proposals/issues/2150

cptchuckles commented 3 years ago

I must disagree; I don't see how methods as described in that other proposal would help the verbosity problem here. This problem is like neglecting a += b because we have a = a + b. I find the reference to Moment.js to be dis-analogous, as dates and times are a property of the world which don't belong to things, unlike a body's position and orientation in space, which it owns and must mutate one way or another for a game to have things happening in it.

Really this is an issue about ergonomics, so to your first point

The issue with mutable methods is that they tend to encourage bad practice and "fragile" code.

I'm not clear on what those bad practices would be in situations like dealing with 3D transformations, especially when the only way I've been able to get the job done in most situations is to shoehorn an immutable operation into a mutating expression by assigning it to itself anyway. Can you elaborate?

CedNaru commented 3 years ago

I personally really like the immutable side of those types because I feel like they are "Godot primitives". I don't see them as mutable objects in the first place.

When it comes to fragile code, it's simple enough. Sometimes you might make a mistake and give a reference as a method parameter or set it as a member of another instance. If you were to mutate that reference in your code, it would also affect the original location this reference came from. Only using copy assignments make sure all your variables are independent of each other, Reducing the risk of coupling and side effects which are the cause of many bugs.

dalexeev commented 3 years ago

Adding mutable methods requires the effort of listing these methods, adding methods, coming up with names (it's better to give names like rotate for a mutable method and rotated for an immutable method, rather than using the mut_ prefix), resolving potential name conflicts, adding descriptions to the documentation.

Mutable methods will clutter up the engine code and documentation, require support in the future (when renaming, when changing the corresponding immutable methods, when adding new methods). Mutable methods themselves are not the cause of bugs, because when you do x = x.immutable_method(), you do the same. But mutable methods can really encourage bad code.

I have an idea. What if we add the ability to refer on the right side of the assignment to the left side?

# $Head.global_transform.basis = $Head.global_transform.basis.lerp(new_basis, factor)
$Head.global_transform.basis = %.lerp(new_basis, factor)

# vector_with_very_long_name = vector_with_very_long_name.normalized()
vector_with_very_long_name = %.normalized()

# There is no shorthand for a prefix like `string += "suffix"`:
# string_with_very_long_name = "prefix" + string_with_very_long_name
string_with_very_long_name = "prefix" + %

1. It's versatile. 2. There are already non-universal analogs +=, -=, etc., which allow using the left side.

But for the most part, it's just syntactic sugar. The problem of long lines is not that significant.

cptchuckles commented 3 years ago

An interesting idea. I wonder if % would be a bad candidate for that because it is already math-modulo and string-interpolate, though. Maybe a special case of $ e.g. $!, $@ or $< could be used instead. I wanted to suggest $_ but that appears to be a valid NodePath :(