pygame-community / pygame-ce

🐍🎮 pygame - Community Edition is a FOSS Python library for multimedia applications (like games). Built on top of the excellent SDL library.
https://pyga.me
947 stars 158 forks source link

Method to normalize zero vectors #2269

Open Yu266426 opened 1 year ago

Yu266426 commented 1 year ago

Description When normalizing a vector, it is necessary to first check it is not of length zero, otherwise the program will crash.

The game engine bevy has a method for its Vecs that is normalize_or_zero() that allows you to normalize a vector that is potentially of length zero, where it returns a zero vector if that is the case, otherwise normalizing it.

I think this would be useful to have in pygame, as it saves having to type an extra line pretty much every time you want to normalize a vector.

bilhox commented 1 year ago

Mathematically speaking, it's impossible to normalize a zero vector, so returning a zero vector would be weird. That's why, I don't agree with you.

Yu266426 commented 1 year ago

Well, I don't agree in the sense that just because something doesn't "make sense mathematically" means that a more convenient method shouldn't exist.

But also, that's why the method is called normalize_OR_ZERO, which I think helps illustrate that it isn't just normalizing it.

bigwhoopgames commented 1 year ago

I would be in support of having this method added. In nearly ever instance of me using normalize() I use a zero vector if I can't normalize.

Matiiss commented 1 year ago

I like the idea of having a way to conveniently get the zero vector back when normalizing, the proposed method's name is quite clear in what it would do and thus I don't see how it wouldn't make sense mathematically either, you either can normalize or you return a zero vector.

An alternative idea would be to add an optional argument to normalize, like zero that would achieve the same thing: vec.normalize(zero=True) (perhaps, it could be a positional-only argument for performance: vec.normalize(True) albeit that makes it less readable, but maybe using keyword args doesn't make it all that slow, so that should be tested)

DaNubCoding commented 1 year ago

Perhaps xnormalize is also an option, meaning "exclusive normalize", where it will normalize everything excluding zero vector in which case it is handled differently. This is easier to type and doesn't clutter code with long method names. Stuff like FRect uses a similar method of naming.

Mega-JC commented 1 year ago

I'm not sure if pygame needs this to be honest, especially if there is already a super simple solution to this problem of getting a potential ValueError: Simply checking the vector's thuthiness using an if statement, or short-circuiting with and by writing the expression vector and vector.normalize(). In the latter case, if vector is a zero vector, the expression will simply return vector. If vector is not a zero vector, vector.normalize() will be returned instead. This seems to be the exact behavior desired here.

Another example:

>>> from pygame import Vector2
>>> 
>>> vec_a = Vector2(5, 0)
>>> vec_a.normalize()
Vector2(1, 0)
>>> 
>>> vec_b = Vector2(0, 0)
>>> vec_b.normalize()
Traceback (most recent call last):
  File "<pyshell#21>", line 1, in <module>
    vec_b.normalize()
ValueError: Can't normalize Vector of length Zero
>>>
>>> vec_a_norm_or_zero = vec_a and vec_a.normalize()
>>> vec_a_norm_or_zero
Vector2(1, 0)
>>>
>>> vec_b_norm_or_zero = vec_b and vec_b.normalize()
>>> vec_b_norm_or_zero
Vector2(0, 0)
bilhox commented 1 year ago

I think however we should have prebuilt Vectors (Like in Unity I think), like Vector2.zero (0 , 0) for example. So it would more easy to compare and check if a vector in null. (or a method that returns true if it's zero ?)

itzpr3d4t0r commented 1 year ago

I think however we should have prebuilt Vectors (Like in Unity I think), like Vector2.zero (0 , 0) for example. So it would more easy to compare and check if a vector in null. (or a method that returns true if it's zero ?)

There's no need for such method, you can already do if not vector:, vectors support bool checks.

Matiiss commented 1 year ago

I think however we should have prebuilt Vectors (Like in Unity I think), like Vector2.zero (0 , 0) for example. So it would more easy to compare and check if a vector in null. (or a method that returns true if it's zero ?)

Aside from vectors supporting boolean checks, you can get a zero vector by simply instantiating a vector: pygame.Vector2()

DaNubCoding commented 1 year ago

Simply checking the vector's thuthiness using an if statement, or short-circuiting with and by writing the expression vector and vector.normalize().

These methods work, however they are either cumbersome to write when normalize is used frequently (if statement solution) or simply unclear to the reader as to what they are doing (and solution).

Mega-JC commented 1 year ago

or simply unclear to the reader as to what they are doing (and solution).

To clarify, vector and vector.normalize() (short-circuting with and) is the same as vector.normalize() if vector else vector (ternary expression), for those who prefer the longer, more explicit syntax. The former just seemed to be the shortest and the easiest to substitute a bare .normalize() method call with in a long sequence of vector operations (Yu266426 implied wanting a concise solution).

DaNubCoding commented 1 year ago

To clarify, vector and vector.normalize() (short-circuting with and) is the same as vector.normalize() if vector else vector (ternary expression), for those who prefer the longer, more explicit syntax. The former just seemed to be the shortest and the easiest to substitute a bare .normalize() method call with in a long sequence of vector operations (Yu266426 implied wanting a concise solution).

Yes the short-circuiting solution is a good solution as of now, but I think it can be made clearer with the implementation of a normalize_or_zero(). It is indeed a more concise version of the ternary solution, but it loses some clarity in comparison.

JiffyRob commented 1 year ago

Just want to point out, if we did implement this, would we also want to do something similar with scale_to_length()? If we don't it's a simple normalize(zero=True) * length but the whole point of this issue is conciseness. I would be in favor of the normalize method just because I use it so often and end up erring out and having to write boilerplate for it constantly. You're right to say that it's pretty simple just to short circuit, but when it is needed so dang often the trade-off makes less sense. Also many pygame users are just learning python and don't know how short circuiting works. At the very least I would put a note in the docs.

bilhox commented 1 month ago

At the beginning, I was against this idea because it's scientifically wrong. So I checked how it was done on other game engine like Unity and godot, and I found that they both return a zero vector, which joins the idea of the original message.

So if anyone is looking at this issue and wants to implement this feature, this is the way to go imo.

Doublestuf commented 1 month ago

I'd love to take this issue :). It'd be my first contribution to pygame-ce (been wanting to contribute to it for a while) and second contribution overall. Might take a while, though.

bilhox commented 1 month ago

Hi @Doublestuf ,

Sure you can do it ! The code you need to modify should be in src_c/math.c in vector_normalize_ip. You'll also have to write a new test in test/math_test.py, and update the documentation in docs/reST/ref/math.rst in the method paragraph.

oddbookworm commented 1 month ago

I'm still not convinced that this is necessary. It may be what bona-fide game engines do, but pygame-ce is not a game engine. There are ways I can think of that being able to "normalize" the zero vector could go pretty wrong because you're expecting the result to be nonzero in your calculations. IMO implementing this just moves the problem from normalizing to wherever else you use the resulting normalized vector. You still need to be cognizant of the case you have the zero vector, so I don't see any benefit to this in general

aatle commented 1 month ago

Note that the behavior of .normalize_ip() is currently equivalent to .scale_to_length(1), so the latter might also need changes.

Yu266426 commented 1 month ago

I still think a separate method is the way to go here. That way the original functionality is unchanged, while providing a more convenient option for situations like normalising input, or maybe getting an offset direction for AI might end up in the player.