-
Notifications
You must be signed in to change notification settings - Fork 136
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
Camera FieldOfView can be NaN #8
Comments
The only place FieldOfView is ever assigned is here in Player.cs float targetFOV = Calc.ClampedMap(velocity.XY().Length(), MaxSpeed * 1.2f, 120, 1, 1.2f);
World.Camera.FOVMultiplier = Calc.Approach(World.Camera.FOVMultiplier, targetFOV, Time.Delta / 4); The only way I could see NaN being assigned is if velocity is also somehow maybe NaN? Weird. |
Gonna share some progress debugging this crash. I managed to narrow down the Celeste64/Source/Actors/Player.cs Lines 990 to 1120 in 48eebda
The person says they're crashing whenever they try to move around, but jumping works fine - so does moving the camera. Dashing causes the game to freeze, both grounded and airborne. They use keyboard, and not analog (I even completely commented out the Feels like |
Good news; diff --git a/Source/Game.cs b/Source/Game.cs
index 89c0fe8..a5fb4f9 100644
--- a/Source/Game.cs
+++ b/Source/Game.cs
@@ -113,6 +113,9 @@ public class Game : Module
public override void Update()
{
+ if (VectorHelpers.HasNaN(Controls.Move.Value))
+ Log.Error($"{nameof(Controls.Move.Value)} contains NaN! Things are about to go very wrong! ({Controls.Move.Value})");
+
// update top scene
if (scenes.TryPeek(out var scene))
{
diff --git a/Source/Helpers/VectorHelpers.cs b/Source/Helpers/VectorHelpers.cs
new file mode 100644
index 0000000..173d09a
--- /dev/null
+++ b/Source/Helpers/VectorHelpers.cs
@@ -0,0 +1,9 @@
+namespace Celeste64;
+public static class VectorHelpers
+{
+ public static bool HasNaN(this in Vec2 vec2)
+ => float.IsNaN(vec2.X) || float.IsNaN(vec2.Y);
+
+ public static bool HasNaN(this in Vec3 vec3)
+ => float.IsNaN(vec3.X) || float.IsNaN(vec3.Y) || float.IsNaN(vec3.Z);
+} |
Thanks for the investigation! This should narrow it down a lot ... |
Found the source of the I added assertions to every parameter when calculating |
Somehow the diff --git a/Source/Actors/Player.cs b/Source/Actors/Player.cs
index d0482d6..03950ae 100644
--- a/Source/Actors/Player.cs
+++ b/Source/Actors/Player.cs
@@ -611,7 +611,15 @@ public class Player : Actor, IHaveModels, IHaveSprites, IRidePlatforms, ICastPoi
if (Vec2.Dot(input, Vec2.UnitY) >= .985f)
input = Vec2.UnitY;
- return forward * input.Y + side * input.X;
+ Vec2 ret = forward * input.Y + side * input.X;
+ if (ret.HasNaN())
+ {
+ ret.AssertNotNaN($"{nameof(RelativeMoveInput)} contains NaN!");
+ Log.Error($"{nameof(World.Camera.Forward)} = {World.Camera.Forward}");
+ Log.Error($"{nameof(forward)} = {forward} (normalized)");
+ Log.Error($"{nameof(side)} = {side} (normalized)");
+ }
+ return ret;
}
} Running this yields these logs:
This is not a one-off, here's what happens when the camera is not moved:
|
Hmm we do use a custom Normalize method, but all it does is check for x/y being 0 and return 0 in that case. Maybe we need to use an epsilon? public static Vector2 Normalized(this Vector2 vector)
{
if (MathF.Abs(vector.X) <= float.Epsilon && MathF.Abs(vector.Y) <= float.Epsilon)
return Vector2.Zero;
return Vector2.Normalize(vector);
} I'm not sure what else would be the cause... but that doesn't entirely make sense because the input values aren't both near zero (only X in the example above). |
Getting infinite/NaN values is definitely expected for very small values ( Note that even when this is behaving normally, |
Could it be processor-dependent? The person is running on a relatively old CPU (AMD Phenom II X4 955), and It might make sense as I've only seen two people experience this crash in the Celeste server. |
if we do our own Normal calculation does that work for them? what the hell return vector / MathF.Sqrt(vector.X * vector.X + vector.Y * vector.Y) |
Will try tomorrow. 👍 |
For those who are following this issue: It turns out my theory wasn't in fact so crazy. A .NET Runtime bug causes the JIT to emit wrong ASM instructions when calling The solution here is to do a manual normalization. I've forked Foster and made it fall back to manual normalization, if any component of the normalized vector is not finite. I sent the built version to my tester (thank you Tanya!) and they confirmed it worked for them. I'll soon make a release on my fork including the fix. EDIT: The release is out. I hope this'll help those affected until the .NET bug is fixed. |
Wow, thanks for looking into this and figuring it out! I guess at this point it's worth submitting findings to .NET runtime? We could add a hack to use manual normalization if it fails but I don't really want to do that on our end if it's something that can be resolved upstream... Unless .NET team decide it's not worth fixing for some reason. |
@Popax21 said he'd make an issue soon, but not right now as he's currently pretty busy. |
Add support for custom player states
It would probably be a good idea to figure out the minimum .NET version this bug was introduced in while I am still out of action. I'll try to prepare a writeup for a potential .NET bug report once I have the time for it. |
This might be related to issues seen on ARM64 Release Mode: #70 (comment) |
The fork fell a bit out of date, so I pulled the changes and published the 1.1.1 release + a few extra commits; remembering to publish new platforms as well. (unfortunately I don't have any machines to test the releases on, so fingers crossed) |
So this is CPU dependent? huh interesting(i have a phenom 2😭) |
Whoops, I meant to say "AMD Phenom II X4 955", but just noticed I forgot to put the "II" in. |
After spending a few hours trying to chase down this bug in the JIT source code, I found out that it's already known & fixed: dotnet/runtime#96939. Not sure if the fix is in the latest .NET 8 runtime, but it would definitely be worth a try to see if updating fixes this. |
Yeah I think at this point it's worth it for me to just update everything and rebuild and see where we're at. I'll do that soon. |
The text was updated successfully, but these errors were encountered: