Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

Fix definitions of type_min and type_zero in xr_types.h #1510

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

AMS21
Copy link
Contributor

@AMS21 AMS21 commented Nov 9, 2023

While looking at the code inside xr_types.h I've notices some very strange definitions which are used later in the code to define constants.

// Type limits
template <typename T>
constexpr auto type_max = std::numeric_limits<T>::max();

template <typename T>
constexpr auto type_min = -std::numeric_limits<T>::max();

template <typename T>
constexpr auto type_zero = std::numeric_limits<T>::min();

type_min is not the minimum value of integer types it is actually the minimum + 1. This only works for floating point types.
type_zero is the actual minimum of a type. But is only 0 for unsigned types. For signed types its a negative value.

Thus we have some very strange constants.

constexpr int int_min = type_min<int>;         // actually -2147483647 not -2147483648
constexpr int int_zero = type_zero<int>;       // actually -2147483648 not 0
...
constexpr float flt_zero = type_zero<float>;   // actually 1.175494e-38 not 0.0
...
constexpr double dbl_zero = type_zero<double>; // actually 2.225074e-308 not 0.0

Fixes #195

@Xottab-DUTY
Copy link
Member

This is related to #195.

One of the reasons why this inconsistency still wasn't fixed is that I really don't know if everything will work correct after the change.

Knowing the codebase, some code logic could depend on this inconsistency. The situation when some code was broken because another unrelated code has been changed happened many times

@github-actions github-actions bot added AI Artificial Intelligence Renderer labels Nov 9, 2023
@AMS21
Copy link
Contributor Author

AMS21 commented Nov 9, 2023

Okay I've now added type_lowest and accompanying constants and changed all usages of these constants across the code base to keep the old behavior.

Making these changes I'm a bit suspicious about some of the uses of flt_zero now flt_min. Some of them look to me like they actually meant to check for 0.

@@ -243,7 +243,7 @@ void CLightProjector::calculate()
v.sub(v_Cs, v_C);
;
#ifdef DEBUG
if ((v.x * v.x + v.y * v.y + v.z * v.z) <= flt_zero)
if ((v.x * v.x + v.y * v.y + v.z * v.z) <= flt_min)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this ever be true?

@@ -155,7 +155,7 @@ _matrix<T>& _matrix<T>::invert(const _matrix<T>& a) // important: this is 4x3
a._12 * (a._21 * a._33 - a._23 * a._31) +
a._13 * (a._21 * a._32 - a._22 * a._31));

VERIFY(_abs(fDetInv) > flt_zero);
VERIFY(_abs(fDetInv) > flt_min);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar here this would always be true since flt_min is negative

The following usages across the code base were changed
`flt_min`  -> `flt_lowest`
`flt_zero` -> `flt_min`
`dbl_zero` -> `dbl_min`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AI Artificial Intelligence Code Quality Renderer
Projects
Status: To do
Development

Successfully merging this pull request may close these issues.

Bug in _types.h
2 participants