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.

Sign up for GitHub

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

Jump to bottom

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
Draft

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

wants to merge 2 commits into from

Conversation

AMS21
Copy link
Contributor

@AMS21 AMS21 commented Nov 9, 2023 •
edited
Loading

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

@AMS21 AMS21 force-pushed the fix/xr_types_min_zero branch from 13a36a3 to 1b49451 Compare November 9, 2023 10:36
@github-actions github-actions bot added AI Artificial Intelligence Renderer labels Nov 9, 2023
@AMS21
Copy link
Contributor Author

AMS21 commented Nov 9, 2023 •
edited
Loading

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.

AMS21
AMS21 commented Nov 9, 2023
View reviewed changes
src/Layers/xrRenderPC_R1/LightProjector.cpp
@@ -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?

AMS21
AMS21 commented Nov 9, 2023
View reviewed changes
src/utils/xrMiscMath/matrix.cpp
@@ -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

@AMS21 AMS21 force-pushed the fix/xr_types_min_zero branch from 1b49451 to eea5ffe Compare November 17, 2023 14:15
@AMS21 AMS21 force-pushed the fix/xr_types_min_zero branch from eea5ffe to 80bfd73 Compare November 25, 2023 13:10
AMS21 added 2 commits December 4, 2023 12:00
@AMS21
xrCore: Fix definitions of type_min and type_zero and add `type_l…
14d22d6 
…owest`
@AMS21
Refactor: Keep old behavior changed by previous commit
fda0f0a 
The following usages across the code base were changed
`flt_min`  -> `flt_lowest`
`flt_zero` -> `flt_min`
`dbl_zero` -> `dbl_min`
@AMS21 AMS21 force-pushed the fix/xr_types_min_zero branch from 80bfd73 to fda0f0a Compare December 4, 2023 11:00
@Xottab-DUTY Xottab-DUTY force-pushed the dev branch 2 times, most recently from 143d1ca to 118d39d Compare December 16, 2023 10:39
@Xottab-DUTY Xottab-DUTY force-pushed the dev branch 2 times, most recently from 5b2ec76 to 6fffce9 Compare May 4, 2024 03:27
@AMS21 AMS21 marked this pull request as draft May 14, 2024 16:20
@Xottab-DUTY Xottab-DUTY force-pushed the dev branch 2 times, most recently from e89fcc8 to f6fd5cc Compare October 24, 2024 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
AI Artificial Intelligence Code Quality Renderer
Projects
Roadmap
Status: To do
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug in _types.h
2 participants
@AMS21 @Xottab-DUTY