Make acos and asin safe

[lawnjelly]

A common bug with using acos and asin is that input outside -1 to 1 range will result in Nan output. This can occur due to floating point error in the input.

The standard solution is to provide safe_acos function with clamped input. For Godot it may make more sense to make the standard functions safe.

Simpler alternative to #76876
Fixes #76857

Notes

• This is working on the assumption that there isn't a massive difference between runtime performance of the clamped / non clamped version (see Add safe acos and safe asin #76876 comments for benchmarks)
• Would defer decision on how to handle bottleneck code until we find example that requires non-clamped version. Perhaps ::acos could be called directly on such a rare occasion.

[lawnjelly]

Very rough poll for those interested?
Vote 👍 on this post for this simple PR and 🚀 for #76876 . 😄

Either is good as long as we decide!

[kleonc]

LGTM.

Given ::asin/::acos can always be called directly when desired I prefer this over #76876, seems simpler / potentially less confusing.

[ghost]

This also creates problems in CharacterBody2D because angles are determined by going through acos () and when it returns NAN, often, the internal set_collision_direction () function returns the result of hitting a "wall" because that is the default fall-through case. In this sort of situation, the code should be amended because it doesn't really make sense that the user has to create their own get_floor_angle (), etc. functions to bypass the unsafe version of acos ().

[lawnjelly]

This also creates problems in CharacterBody2D because angles are determined by going through acos () and when it returns NAN, often, the internal set_collision_direction () function returns the result of hitting a "wall" because that is the default fall-through case. In this sort of situation, the code should be amended because it doesn't really make sense that the user has to create their own get_floor_angle (), etc. functions to bypass the unsafe version of acos ().

Yes, this is a snag, anything that relies on Nan will have to use the raw version (::acos) or be modified. I wrote a bit about this in the other PR where there is an unsafe version. Will check this bit of code and try and look for any other cases. 👍

EDIT:
Just to be clear, I'm not familiar with this code, but under which circumstances are you seeing this return a Nan?

get_angle is:

		real_t get_angle(Vector2 p_up_direction) const {
			return Math::acos(collision_normal.dot(p_up_direction));
		}

For this to return Nan, collision_normal would have to be higher than 1 length, or p_up_direction less then 1 length I think? Which is perfectly reasonable from float error. So this seems to fix a bug rather than create one?

If collision_normal is zero (i.e. no movement) the result would the same as before. Maybe I'm missing something on the mechanism of this problem.

[ghost]

I can't remember the exact details, as it was a while ago, but both normals involved were normalized and I was noticing that the number was slightly outside of the -1.0..1.0 range and therefore acos () was returning NAN. I added the clamp () when I noticed this, and I haven't noticed the problem since.

collision_normal was never 0 in the circumstances I encountered. The normals were valid and I was surprised because the documentation states that if the normals are normalized, then you are guaranteed a value that is between -1.0..1.0 inclusive only and that was not the case.

[lawnjelly]

Ah yes, I was misunderstanding you. You were pointing out that this was previously a bug, and it is now fixed by this PR. 👍

It was indeed quite a nasty bug, because you got easily get Nans there.

collision_normal was never 0 in the circumstances I encountered. The normals were valid and I was surprised because the documentation states that if the normals are normalized, then you are guaranteed a value that is between -1.0..1.0 inclusive only and that was not the case.

After normalizing a vector, the vector will usually be slightly below or slightly above unit length. This tiny float error is enough to get outside the -1 to 1 range, and thus cause the Nan.

[ghost]

It was the first of a few bugs I've found in the CharacterBody2D code but I can confirm it is easily solved with clamping.

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.0.3.

[lawnjelly]

Also reminder there is a 3.x version #76902 which is mostly similar except there are no unit tests for this in 3.x.

If we could review / merge that too while it is fresh in mind it would be great. 👍

[MichaelMacha]

* This is working on the assumption that there isn't a massive difference between runtime performance of the clamped / non clamped version (see [Add safe acos and safe asin #76876](https://github.com/godotengine/godot/pull/76876) comments for benchmarks)

I'm happy to hear that this is getting patched! That said, if we ever go this route, I would prefer that acos and sin be safe by default, and for niche cases where its range check is a problem, we could have unsafe_cos and unsafe_sin for higher-speed and less checked behavior. Most users prefer stability over speed, until they know they need to inch a process out a little faster and are aware of the risks.

Moreover, having a function called "unsafe" highlights that there's a risk, more than having a slower alternative called "safe" which they should probably be using by default.