Add normalize_and_get_length() to Vector2 and Vector3 #56567

lawnjelly · 2022-01-06T17:58:52Z

Length is calculated as part of the normalize routine. This is often desired, and also can be used to flag the condition where the Vector was too small to normalize (by returning zero length).

Helps address #74852

Notes

Most engines I've worked on have returned the length from normalize, as this can be a useful value in a lot of cases, and should cost nothing from c++ if not used (as it should be optimized out).

In a PR meeting it was decided this would be better as a separate function, rather than modifying the existing normalize().
Returning zero if less then EPSILON allows users to deal with error cases where the input Vector length is small.
Due to the ability to handle error conditions (over the existing normalized() bound function) the function is bound, in order to be available from script etc.
I noticed that Vector3::normalize() sets all values to zero if length is zero (curiously Vector2 does not do this). I'm presuming this is to get a standard value for later comparisons because of the different bit representations near zero. This didn't seem appropriate to do in this PR with an epsilon, but can change if desired.

This can be done longhand which is what I normally end up doing, e.g.:

real_t length = myvec.length();
if (length >= EPSILON)
{
myvec /= length;
}
// do something with length

but the form enabled by the PR is more concise and less error prone (especially epsilons can be standardized in the normalize function):

real_t length = myvec.normalize_and_get_length();
if (length == 0)
{
// Failed ...
}
// do something with length

or simply

if (myvec.normalize_and_get_length() == 0)
{
// Failed
}

(This is one of those rare occasions we can do an exact floating point comparison)

aaronfranke

Note: This would only be for internal use. normalized is what's exposed, and it returns a Vector2.

aaronfranke · 2022-01-06T18:18:47Z

core/math/vector2.h

+	real_t l = x * x + y * y;
+	if (l != 0) {
+		l = Math::sqrt(l);
+		x /= l;
+		y /= l;
+		return l;
+	}
+	return 0.0;


Could be simplified:

Suggested change

real_t l = x * x + y * y;

if (l != 0) {

l = Math::sqrt(l);

x /= l;

y /= l;

return l;

}

return 0.0;

real_t l = x * x + y * y;

if (l != 0) {

l = Math::sqrt(l);

x /= l;

y /= l;

}

return l;

Yup sure. 👍
I was intrigued to find in godbolt that the assembly is slightly different.

Even adjusting for the literal issue I mentioned in #56396 (and is probably addressed in a separate PR if we get some agreement on it), I wonder if it is trying to account for signed zero. We'd have to time them to see which was "better", I don't expect there's a lot in it! 😁

-0.0 would be the only case that is different, so that has to be it. I also checked NaN but that should enter the if and then explode things there so it wouldn't be affected by this.

lawnjelly · 2022-01-20T15:36:54Z

PR Meeting: Add normalize_and_get_length() separate function if this has enough use, rather than change normalize().

AThousandShips · 2023-03-13T10:01:58Z

core/math/vector2.h

@@ -241,6 +242,17 @@ _FORCE_INLINE_ bool Vector2::operator!=(const Vector2 &p_vec2) const {
 	return x != p_vec2.x || y != p_vec2.y;
 }

+real_t Vector2::normalize_and_get_length() {
+	real_t lengthsq = length_squared();
+	if (lengthsq >= (real_t)CMP_EPSILON) {


Should it be CMP_EPSILON or CMP_EPSILON2

Ah yes, I'm expecting some discussion on the epsilon, and will need a bit of testing. I saw CMP_EPSILON2 but wanted to make sure it is ok for this use with 32 bit real_t. But you are correct we are using squares here.

It has to be a value that is large enough that when sqrted afterwards, it still provides accuracy when used as the divisor. So I think a bit of testing will be good to check this in practice rather than going straight for the smaller CMP_EPSILON2 (without knowing how it was tested).

I'm fully agnostic and the only difference with 64 bit would be the lower limit on magnitude

32 / 64 bit epsilon differences can be quite difficult.

Last time we discussed it (that I remember), reduz was in favour of keeping the 32 bit epsilons when compiling as 64 bit. This loses out on some potential accuracy in 64 bit, but if you use different epsilons you open up a massive surface of difficult to find bugs. Epsilon bugs are some of the worst imo. 😁

Anyway I'm doing some empirical testing on the epsilon as we speak. 👍
Ogre looks like it uses 1e-8f, which is larger than CMP_EPSILON2 (which is 1e-10f).

Epsilon bugs are some of the worst imo.

Agreed

Ok after (I hope) some testing, I'm satisfied that CMP_EPSILON2 should be okay at 32 bit, so I've pushed a version with this epsilon.

I tested in gdscript then in a c++ testbed. Of course there's always the chance I messed up, it's pretty easy to mess up, with math precision. 😁

aaronfranke · 2023-03-14T12:58:47Z

doc/classes/Vector3.xml

+			<return type="float" />
+			<description>
+				Scales the vector to unit length, and returns the original length.
+				If the original vector was too small to normalize, the length returned is [code]zero[/code].


This would be the only method exposed to GDScript that mutates the struct. That's a bit odd. Is this method necessary to expose, or is it intended to be an internal helper for the C++ code?

There are some issues with it because of access issues with GDScript, primitive types are non-mutable when returned, including as properties, GDScript passes them by copy not reference, unless that has changed, as in mutating functions cannot affect them

See #70995

If you can think of an easy way to return two values efficiently from bound functions, I'm happy to modify the PR.

If we don't want it to mutate the struct, we need to return the new Vector and the length (or the new Vector and success or failure).

Originally this was non-bound, but it is now also a potential candidate solution to the problem in #74852, and whichever solution we go with needs to be available from script.

Another non-mutating alternative would be a normalized() function that returns either the normalized vector or failure (maybe a Vector or NULL? can that work?).

Perhaps then for binding returning a variant that is either the normalized value or NULL is the way to go try_normalized() or something like that?

I can modify this particular PR to have this function unbound as before, but will be good to get some discussion going on how to solve the gdscript problem.

It could return an array or dictionary, somewhat cumbersome and inefficient but it gets it done, also I like try_normalized

Length is calculated as part of the normalize routine. This is often desired, and also can be used to flag the condition where the Vector was too small to normalize (by returning zero length).

lawnjelly · 2023-03-14T14:23:34Z

Closing in favour of #74912 , as it appears that modifying in place isn't good for bound functions.

lawnjelly requested a review from a team as a code owner January 6, 2022 17:58

aaronfranke reviewed Jan 6, 2022

View reviewed changes

clayjohn added feature proposal performance topic:core labels Jan 6, 2022

aaronfranke added this to the 4.0 milestone Jan 6, 2022

lawnjelly added the for pr meeting label Jan 20, 2022

lawnjelly removed the for pr meeting label Jan 20, 2022

lawnjelly marked this pull request as draft February 3, 2022 09:07

lawnjelly mentioned this pull request Aug 4, 2022

Check if the axis is zero / vectors are colinear in Vector3 slerp #63912

Merged

YuriSizov modified the milestones: 4.0, 4.x Feb 12, 2023

lawnjelly mentioned this pull request Mar 13, 2023

Vector3 normalized() does not return normalized vector for very small nonzero vectors #74852

Open

lawnjelly force-pushed the normalize_length branch from c1ccb95 to 3a3e1ea Compare March 13, 2023 09:32

lawnjelly changed the title ~~Return length when calling normalize()~~ Add normalize_and_get_length() to Vector2 and Vector3 Mar 13, 2023

lawnjelly force-pushed the normalize_length branch from 3a3e1ea to 288b93c Compare March 13, 2023 09:45

lawnjelly marked this pull request as ready for review March 13, 2023 09:46

lawnjelly requested a review from a team as a code owner March 13, 2023 09:46

lawnjelly added enhancement usability labels Mar 13, 2023

AThousandShips reviewed Mar 13, 2023

View reviewed changes

lawnjelly force-pushed the normalize_length branch 2 times, most recently from 917b24e to dec5096 Compare March 14, 2023 12:47

aaronfranke reviewed Mar 14, 2023

View reviewed changes

Add normalize_and_get_length() to Vector2 and Vector3

e7ba712

Length is calculated as part of the normalize routine. This is often desired, and also can be used to flag the condition where the Vector was too small to normalize (by returning zero length).

lawnjelly force-pushed the normalize_length branch from dec5096 to e7ba712 Compare March 14, 2023 13:04

lawnjelly mentioned this pull request Mar 14, 2023

Robust normalized() and normalize_and_get_length() #74912

Open

lawnjelly closed this Mar 14, 2023

akien-mga added the archived label May 12, 2023

YuriSizov removed this from the 4.x milestone Dec 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add normalize_and_get_length() to Vector2 and Vector3 #56567

Add normalize_and_get_length() to Vector2 and Vector3 #56567

lawnjelly commented Jan 6, 2022 •

edited

Loading

aaronfranke left a comment

aaronfranke Jan 6, 2022

lawnjelly Jan 6, 2022

aaronfranke Jan 6, 2022 •

edited

Loading

lawnjelly commented Jan 20, 2022

AThousandShips Mar 13, 2023

lawnjelly Mar 13, 2023 •

edited

Loading

AThousandShips Mar 13, 2023

lawnjelly Mar 13, 2023 •

edited

Loading

AThousandShips Mar 13, 2023

lawnjelly Mar 13, 2023

aaronfranke Mar 14, 2023 •

edited

Loading

AThousandShips Mar 14, 2023 •

edited

Loading

lawnjelly Mar 14, 2023

lawnjelly Mar 14, 2023

AThousandShips Mar 14, 2023 •

edited

Loading

lawnjelly commented Mar 14, 2023

Add normalize_and_get_length() to Vector2 and Vector3 #56567

Add normalize_and_get_length() to Vector2 and Vector3 #56567

Conversation

lawnjelly commented Jan 6, 2022 • edited Loading

Notes

aaronfranke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfranke Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

lawnjelly commented Jan 20, 2022

Choose a reason for hiding this comment

lawnjelly Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawnjelly Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronfranke Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

AThousandShips Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AThousandShips Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

lawnjelly commented Mar 14, 2023

lawnjelly commented Jan 6, 2022 •

edited

Loading

aaronfranke Jan 6, 2022 •

edited

Loading

lawnjelly Mar 13, 2023 •

edited

Loading

lawnjelly Mar 13, 2023 •

edited

Loading

aaronfranke Mar 14, 2023 •

edited

Loading

AThousandShips Mar 14, 2023 •

edited

Loading

AThousandShips Mar 14, 2023 •

edited

Loading