fabiangreffrath / taradino

SDL2 port of Rise of the Triad
GNU General Public License v2.0
31 stars 8 forks source link

Apogee logo rotation on startup is incorrect. #18

Closed malespiaut closed 1 year ago

malespiaut commented 1 year ago

https://github.com/fabiangreffrath/rott/assets/76531574/4000186d-1c5b-4a2a-9be1-b4279cef3713

https://github.com/fabiangreffrath/rott/assets/76531574/570202e3-3d46-48bc-9380-d1666820349c

fabiangreffrath commented 1 year ago

Funny, I never even realized. 😁

malespiaut commented 1 year ago

I'm sure that this is going to be a “fun” one to fix. I remember looking at the function responsible for this animation a couple of years ago, because it was triggering the overflow sanitizers, and I remember looking at a bunch of magic numbers and bitshifts.

fabiangreffrath commented 1 year ago

The "savegame screenshot to rendered scene" animation looks a bit borked, too.

fabiangreffrath commented 1 year ago

Possible that I introduced that bug myself: https://github.com/fabiangreffrath/rott/commit/1ec08e598c493f8379a6d07b1ee31f7fb0855d24#diff-6ac0a9025b59165b47dd0d6d002491682fb65949e3466cdc0de2ec6494644d29

malespiaut commented 1 year ago

That was an easy one. APOGEESCALESTART is casted as unsigned. Casting it as signed is fixing the issue. However I haven't checked if it raises a false positive under cppcheck.

fabiangreffrath commented 1 year ago

Yep, that was my own fault. If casting to signed causes cppcheck issues, we might try to cast it to int64_t instead.

malespiaut commented 1 year ago

Casting to int64_t works fine, so that's a good solution.

On a side note, about types, I wonder if that's a good idea to check if all long should be replaced as int32_t, as they haven't been changed from the original source code. Maybe going overboard… but that's who I am.

fabiangreffrath commented 1 year ago

In DOS, long could also mean intptr_t, so it's risky to assume that it always meant int32_t. However, the current code runs on 32- bit platforms as well as 64-bit Linux and Windows, so I guess it's mostly fine as it is.

malespiaut commented 1 year ago

Here's to the joys of digital archeology. ;-)

fabiangreffrath commented 1 year ago

In DOS, long could also mean intptr_t, so it's risky to assume that it always meant int32_t.

But then that same code would break in Win64 today, so maybe it's safe anyway. 😉

Casting to int64_t works fine, so that's a good solution.

Would you like to file a PR?

fabiangreffrath commented 1 year ago

Casting to int64_t works fine, so that's a good solution.

Would you like to file a PR?

Could you check if simply casting it to int does both silence the cppcheck warning and fix the animation?

malespiaut commented 1 year ago

Alright, I'll do that.

fabiangreffrath commented 1 year ago

Fixed by #31 ?

malespiaut commented 1 year ago

Yes, it's fixed by #31!