Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to normalize a Vector of length zero #3157

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Doublestuf
Copy link

In issue #2269, it's been decided that pygame-ce should follow game engines such as Unity and Godot in allowing users to normalize zero-vectors for convenience.

Update the function in math_test.c, tests in math_test.py, and documentation in math.rst accordingly.

@Doublestuf Doublestuf requested a review from a team as a code owner October 8, 2024 23:52
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that it was actually determined for sure that this needs to be implemented in pygame-ce. I'm still partially against it because there are several nonintuitive things that could break if you expect normalizing to return a vector of magnitude 1

@@ -237,6 +237,11 @@ Multiple coordinates can be set using slices or swizzling
Returns a new vector that has ``length`` equal to ``1`` and the same
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should update this if we're gonna go this route, as well as the sluglines above

@@ -247,6 +252,11 @@ Multiple coordinates can be set using slices or swizzling
Normalizes the vector so that it has ``length`` equal to ``1``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this and the sluglines above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you still need to update the Vector3 docs with these changes as well

Comment on lines -745 to -757
self.assertRaises(ValueError, lambda: self.zeroVec.normalize())
# 0 vector
self.assertEqual(self.zeroVec.normalize(), Vector2())

def test_normalize_ip(self):
v = +self.v1
# v has length != 1 before normalizing
self.assertNotEqual(v.x * v.x + v.y * v.y, 1.0)
# inplace operations should return None
self.assertEqual(v.normalize_ip(), None)
self.assertEqual(self.zeroVec.normalize_ip(), None)
# length is 1
self.assertAlmostEqual(v.x * v.x + v.y * v.y, 1.0)
# v2 is parallel to v1
self.assertAlmostEqual(self.v1.x * v.y - self.v1.y * v.x, 0.0)
self.assertRaises(ValueError, lambda: self.zeroVec.normalize_ip())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes also need to be applied to the Vector3 tests

@oddbookworm oddbookworm added the math pygame.math label Oct 9, 2024
@oddbookworm oddbookworm linked an issue Oct 9, 2024 that may be closed by this pull request
@Doublestuf
Copy link
Author

Good point -- this adds the possibility of example_vector.normalize().is_normalized() returning False, which could complicate things and essentially defeat the point of this change. I think this idea of making a new function called xnormalize while leaving the current normalize function would be more intuitive.

@bilhox
Copy link
Contributor

bilhox commented Oct 10, 2024

I'm not sure that it was actually determined for sure that this needs to be implemented in pygame-ce. I'm still partially against it because there are several nonintuitive things that could break if you expect normalizing to return a vector of magnitude 1

@oddbookworm Could you provide an example ? If we merge this, it will not break any code as the following structure :

if vector:
  vector.normalize_ip()

Would still work. (which is what people are currently doing / supposed to do)

@bilhox
Copy link
Contributor

bilhox commented Oct 10, 2024

@Doublestuf So the test can pass, you'll have to update the test for normalize, because it's currently expecting an error.

@Doublestuf Doublestuf marked this pull request as draft October 15, 2024 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
math pygame.math
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method to normalize zero vectors
3 participants