unsigned overflow for linearskyangle

[fabiangreffrath]

linearskyangle is type angle_t i.e. unsigned int so it overflows once (viewwidth/2-x) < 0 here:

https://github.com/MrAlaux/Nugget-Doom/blob/db0a180845bbe31cf67284327d29e5655b743dac/src/r_main.c#L324

This is not a problem per se, because angles are meant to "wrap around", but the division of the absurdly large value by the fovdiff float variable will be causing problems.

[fabiangreffrath]

Maybe try to cast to regular int before the division.

[MrAlaux]

Maybe try to cast to regular int before the division.

Something like 70ee02b7582a581ab3ba0584ade5ea4e2e4ae1bf?

[fabiangreffrath]

Worth a try. Btw, I guess this is rather compiler specific than cpu/os specific, so you you try to reproduce the bug when compiling with clang instead of gcc?

[MrAlaux]

Worth a try. Btw, I guess this is rather compiler specific than cpu/os specific, so you you try to reproduce the bug when compiling with clang instead of gcc?

It looks like I finally managed to compile Nugget with clang. However, the bug seems to be absent.

[MrAlaux]

I managed to make contact with the user who has the issue, and while we weren't able to fix it, I think I've reached some conclusions:

• Removing the division by fovdiff does indeed eliminate the issue
• Taking https://github.com/MrAlaux/Nugget-Doom/commit/70ee02b7582a581ab3ba0584ade5ea4e2e4ae1bf as reference, replacing the cast to int:
  • Casting to uint64_t replicates the issue on my end
  • Casting to angle_t (uint32_t?) causes a similar issue, but instead of stretching the sky from the middle towards the right even at rfov == 90, it's more like a sky of its own were being rendered there
  • Casting to int64_t and int32_t works normally, but casting to int16_t causes the whole sky to be stretched

[fabiangreffrath]

🤔 Does this indicate that angle_t is indeed unsigned long? What does sizeof(angle_t) give on your system?

By intermediate casting to a signed type that is capable of holding the result (int16_t appears to be out of the question here), the negative value of the first part of the calculation isn't wrapped around to a large positive value.

Could you try something along these lines?

int64_t temp = ((viewwidth/2-x)*((SCREENWIDTH<<6)/viewwidth))*(ANG90/(NONWIDEWIDTH<<6)); 
linearskyangle[x] = (angle_t)(temp / fovdiff); 

[MrAlaux]

What does sizeof(angle_t) give on your system?

Sorry if this very naive: printf("%llu", sizeof(angle_t)) outputs 4. In case it matters, I tried %lu at first but the compiler warned about sizeof(angle_t) being of type long long unsigned int.

Could you try something along these lines?

I assume you were asking to try that in their system, but just in case, I tried it on my own, and it seems to work fine (then again, I'm not the one with the bug). I'll ask them later, maybe tomorrow.

I should clarify: all the conclusions in my previous comment, except for the first one, are the results of tests on my system, not the other user's.

What we did try on their system was:

• Removing the division by fovdiff, of course; that solved the issue.

• Keeping the division, but casting to angle_t before it; that caused similar issues on both of our machines, see:

Details

`rfov == 80`  `rfov == 95`  `rfov == 90` looks normal.

• Same as the above, but casting to int64_t; according to them, that would "give the stretch effect for every FOV", which sounds like the same issue they already had.

[fabiangreffrath]

Sorry if this very naive: printf("%llu", sizeof(angle_t)) outputs 4. In case it matters, I tried %lu at first but the compiler warned about sizeof(angle_t) being of type long long unsigned int.

Indeed, sizeof() returns type size_t which is an alias for the widest unsigned int type on a particular system. But sizeof(angle_t) == 4 means that angle_t is indeed unsigned int, fine.

I assume you were asking to try that in their system, but just in case, I tried it on my own, and it seems to work fine (then again, I'm not the one with the bug). I'll ask them later, maybe tomorrow.

That'd be nice, thanks!

[MrAlaux]

Alright, they just tried your patch, and unfortunately it results in the same issue we started with:

Screenshot


[MrAlaux]

Deprecated by Woof 14.0.0 merge.