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

sign returns 1 for NAN #79036

Closed
lilizoey opened this issue Jul 4, 2023 · 1 comment · Fixed by #81464
Closed

sign returns 1 for NAN #79036

lilizoey opened this issue Jul 4, 2023 · 1 comment · Fixed by #81464

Comments

@lilizoey
Copy link

lilizoey commented Jul 4, 2023

Godot version

4.1.rc3

System information

Linux 5.10.181-2-MANJARO

Issue description

sign(NAN) and signf(NAN) both return 1. But this doesn't seem right, as NAN > 0 is false (as all other NAN comparison are). These should really return NAN not 1.

Steps to reproduce

run

print(sign(NAN), " ", signf(NAN))

output is:
1 1

Minimal reproduction project

N/A

@dalexeev
Copy link
Member

dalexeev commented Jul 5, 2023

godot/core/typedefs.h

Lines 110 to 113 in cdd2313

template <typename T>
constexpr const T SIGN(const T m_v) {
return m_v == 0 ? 0.0f : (m_v < 0 ? -1.0f : +1.0f);
}

static inline Variant sign(const Variant &x, Callable::CallError &r_error) {
r_error.error = Callable::CallError::CALL_OK;
switch (x.get_type()) {
case Variant::INT: {
return SIGN(VariantInternalAccessor<int64_t>::get(&x));
} break;
case Variant::FLOAT: {
return SIGN(VariantInternalAccessor<double>::get(&x));
} break;
case Variant::VECTOR2: {
return VariantInternalAccessor<Vector2>::get(&x).sign();
} break;
case Variant::VECTOR2I: {
return VariantInternalAccessor<Vector2i>::get(&x).sign();
} break;
case Variant::VECTOR3: {
return VariantInternalAccessor<Vector3>::get(&x).sign();
} break;
case Variant::VECTOR3I: {
return VariantInternalAccessor<Vector3i>::get(&x).sign();
} break;
case Variant::VECTOR4: {
return VariantInternalAccessor<Vector4>::get(&x).sign();
} break;
case Variant::VECTOR4I: {
return VariantInternalAccessor<Vector4i>::get(&x).sign();
} break;
default: {
r_error.error = Callable::CallError::CALL_ERROR_INVALID_METHOD;
return Variant();
}
}
}

static inline double signf(double x) {
return SIGN(x);
}
static inline int64_t signi(int64_t x) {
return SIGN(x);
}

I'm not sure what behavior should be, it's probably implemented this way for performance. But perhaps we could add checks to the @GlobalScope utility functions without touching the core SIGN() function?

For comparison, in JavaScript:

Math.sign(NaN) // NaN

@akien-mga akien-mga added this to the 4.2 milestone Sep 12, 2023
@akien-mga akien-mga added the bug label Sep 12, 2023
mandryskowski pushed a commit to mandryskowski/godot that referenced this issue Oct 11, 2023
Fixes godotengine#79036. sign(NAN) now returns 0.
This should not impact performance much in any way.
Adds a test for the NAN case. Updates the documentation to clarify the new behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants