64-bit patch for the Dark Mod

I've been spending the past few weeks adding 64 bit support to the Dark Mod, and finally have something that mostly work. This was only developed and tested on Linux, so a Windows version will probably require some more work still.

Things tested:

Able to play through the FM "Thief's Den" to completion.

Able to play various bits of the training mission. (Was not played exhaustively.)

Downloading FMs and switching FMs

Compiling for 32 bit with the original build system.

Loading 64 bit saves on a 32 bit version compiled from the same source.

Some AIs stand in the spot while their arms are moving and their toes are lifting, suggesting they're wanting to walk somewhere but failing to. Some suspicious noise does make them to move forward a bit more. The City Watchman in the Thief's Den is a good example. (64 bit only)

Melee weapons don't connect at all, both for the player and AI. (both 32 bit and 64 bit)

Sky boxes seem to take on random colours based on your location and which direction you're looking. (64 bit only)

What's needed to build:

CMake

Make sure the following libraries and development headers are installed:

Boost

CURL

DevIL

JPEG

OpenAL

PNG

ZLIB

X11

Xext

Xxf86vm

The souce code downloaded from the Darkmod's Subversion repository.

Build instructions:

Apply diff to source from the SVN repository.

Remove "include/boost". This prevents the older headings from conflicting with newer ones.

If your the source code is in "~/darkmodsrc/trunk/" make a directory "~/darkmodsrc/build"

Run the command:

cd ~/darkmodsrc/build
cmake ../trunk -DCMAKE_BUILD_TYPE=Debug

If successful, type the following to build the actual game:

make

If make is successful you should now have "darkmod.x86_64", "gamex86_64.so", and "tdm_game02.pk4" in your build directory. Copy these files to your installation of TDM (or make symlinks).

Other notes:

Currently the CMakeLists.txt has *x86_64 file names baked into it, but if the file names are fixed, this should also work on 32 bit systems.

I've only tested on Linux Mint 17.2 (64 bit).

Technically it's still a work in progress, but I'm going to having less time to work on it in the near future, and the remaining bugs that I'm aware of have me stumped at how to find the cause of their problems. Anyway, I figured I'd put it out here, rather than let it languish forgotten on my hard drive.

Besides obvious issues you already mentioned, you will have collision issues on both Linux and Windows builds. Plus you'd have to port all tools to 64bit platform (Linux has not tools, so it's a bit easier to deal with).

I wasn't aware of there being an 64 bit branch already. Now that you've mentioned it, I can't seem to find it. Is there a special spot that it's kept?

May I ask what's the benefit of having 64bit build ?

For me, the advantages is being able to use more advance debug tools. I'm not able to run Valgrind on a 32-bit build, as vital libraries are missing their debug information.

The other advantage in my eyes is being able to compile natively to the distro and against the libraries it has installed.

Besides obvious issues you already mentioned, you will have collision issues on both Linux and Windows builds. Plus you'd have to port all tools to 64bit platform (Linux has not tools, so it's a bit easier to deal with).

I'm not sure what you mean by collision issues. I can build both 64 bit with the new CMake file and 32 bit with the old SCons files on the same machine. There are some manual steps still (like moving include/boost ) but those can be overcome with more dev time.

External tools don't need to be ported to 64 bit immediately. When they're ported, any binary data they produce just needs to keep the same sizes on data types and structures.

The other advantage in my eyes is being able to compile natively to the distro and against the libraries it has installed.

This is a huge advantage. All modern Linux distributions are 64-bit and having to install separate 32-bit libraries is a pain in the ass. I agree that it's probably less important on Windows, where most games are still 32-bit as I understand it.

Also the fact that you have updated the build system to use CMake is a great leap forward. Last time I tried to compile TDM on my own system I couldn't because the horribly broken Scons build only worked on some ancient distribution like Ubuntu 10.04.

This is a huge advantage. All modern Linux distributions are 64-bit and having to install separate 32-bit libraries is a pain in the ass. I agree that it's probably less important on Windows, where most games are still 32-bit as I understand it.

Also the fact that you have updated the build system to use CMake is a great leap forward. Last time I tried to compile TDM on my own system I couldn't because the horribly broken Scons build only worked on some ancient distribution like Ubuntu 10.04.

Part of my motive for switching to CMake from SCons, is that I found SCons's build files were rather opaque to my eyes and I was puzzled at how to do anything that wasn't a copy and paste job. However, the big motivation for me is the fact I got this "Cannot explain why `somefile.os' is being rebuilt: No previous build information found" for every single file while building. This meant it recompiled the whole thing everytime I changed the simplest of things, and waiting 4 minutes for it to finish compiling got old fast. I couldn't figure out how to fix it, so I created a CMake file to replace it.

Guys, should this be the main release build for Linux? If so we should draw grayman's attention to the thread. I'm not sure what he does to do to package up the release, but obviously it'll be affected.

There are some things we do have to test with any replatforming, that a couple of people have hinted at: collision problems, visportal problems, light-interaction problems, possibly (but unconfirmed) ai pathing.

The light interaction problems are specific to MS Visual Studio. The VP and collision problems are down to different floating-point architecture in MSVS 2012 and above. Prior to msvs2012, the intermediate results of 32-bit floating point calculations were 80-bit. From 2012, they are truncated to 32-bit. You can see the difference with this bit of test code:

That's a real example I tracked down in a map where a visportal was glitching, when we experimented with using the vs2013 compilers. We need to check the result of that test code in any new compiler setup.

There are parts of the game and engine that rely on the greater intermediate precision. That problem stalled our work on the 64-bit build during 2.03 development. We could fix the problem for any given line of code by using doubles, but we have no way to track down all the places that need the fix. The resulting bugs are rare and hard to rep.

That's a real example I tracked down in a map where a visportal was glitching, when we experimented with using the vs2013 compilers. We need to check the result of that test code in any new compiler setup.

There are parts of the game and engine that rely on the greater intermediate precision. That problem stalled our work on the 64-bit build during 2.03 development. We could fix the problem for any given line of code by using doubles, but we have no way to track down all the places that need the fix. The resulting bugs are rare and hard to rep.

Sounds like an excellent example of where an automated unit test would be indispensible. Not necessarily one that would run for every build, but something that would be available for developers to validate the setup of a new platform.

What happens is that Left() creates a temporary idStr, and WeapName stores a pointer to its C string. Because this string is short, it will use idStr's internal base buffer and not allocate anything from the heap. Then once line has finished excuting it deletes the temporary idStr, leaving WeapName with a stale pointer.

On the next line Mid() then creates a new temporary idStr and AttName/ParName stores a pointer to its C string of that. This new temporary idStr reuses the storage for the previous temporary idStr. Because the storage is recycled, the two strings pointers (WeapName and AttName/ParName) point to the same string, which is what's meant to be in AttName/ParName.

The end result is that WeapName doesn't name a weapon name, so the weapon look up fails and it can't do the actual weapon attack/parry, however, it still plays the animation.

Whether or not this bug manifest depends upon when it deletes the temporary idStr for WeapName. If immediately it will bug with melee weapons having no effect, or if its deleted when the scope ends the bug won't happen. So ultimately its dependant on the compiler, and possibly it's optimisation options.

Nice diagnosis. I've seen those string pointers misused before and winced, but only in debug code. Lots of ::Print() functions return a pointer to freed memory too. Debug print functions tend to pass the pointers uncopied to the console, which'll try to access them later. You occasionally see garbage on the console as a result if you have debug logging on.

Thanks for the patch, that's an elegant way to fix it. I see idStr's constructor explicitly copies the internal data of the string.

There are some things we do have to test with any replatforming, that a couple of people have hinted at: collision problems, visportal problems, light-interaction problems, possibly (but unconfirmed) ai pathing.

The light interaction problems are specific to MS Visual Studio. The VP and collision problems are down to different floating-point architecture in MSVS 2012 and above. Prior to msvs2012, the intermediate results of 32-bit floating point calculations were 80-bit. From 2012, they are truncated to 32-bit. You can see the difference with this bit of test code:

That's a real example I tracked down in a map where a visportal was glitching, when we experimented with using the vs2013 compilers. We need to check the result of that test code in any new compiler setup.

There are parts of the game and engine that rely on the greater intermediate precision. That problem stalled our work on the 64-bit build during 2.03 development. We could fix the problem for any given line of code by using doubles, but we have no way to track down all the places that need the fix. The resulting bugs are rare and hard to rep.

I've been doing a little reading about floating point promotions in C/C++, and dependence on extra precision on intermediates calculations is technically a bug. From what I've seen, there is no requirement in C/C++ standards to promote to double/long double on calculations with only floats, so a compiler can keep things as floats if it wants. This means MSVS2012+ does conform to the standard, even if it's not the desired behaviour. This is a problem that will need to be solved when TDM is ported to a CPU that doesn't give extra precision for free.

Based on some tests with GCC (see below), what I suspect what might be happening with MSVS2012+ is that it's using SSE to perform floating point calculations (by compiling for 64 bit, you implicitly have a CPU with SSE support). Because that can operate with single precision directly, it will give you single precision intermediate results. MSVS2012+ might work if you disable SSE for floating point (or force 387 FPU).

I did some quick tests with your code snippet in GCC and found the following results:

The first test compiles to 64 bit (x86-64) with just the default settings for the architecture. This exhibits the rounding bug.The second test compiles to 32 bit (i386) , again with defaults for the architecture. This does not exhibit the rounding bug.The third test compiles to 64 bit (x86-64) with the SSE used for floating point results. This is the same as the first test and exhibits the rounding bug.The fourth test compiles to 64 bit (x86-64) with the 387 used for floating point results. This gives the correct result.The fifth test compiles to 32 bit but with the x86-64 instruction set, and the SSE used for floating point results. This exhibits the rounding bug.

The above to me indicates that the use of SSE is causing it to round to float with every calculation, so is the probably the cause of the glitches you witnessed in your 64 bit port.

After the above discovery, I decided to add -mfpmath=387 the CMake file and recompile from scratch. After doing so there were no noticeable changes on the known bugs for my 64 bit port. AIs are still blind to the player, some AIs just "stand still", and the skybox still changes colour. So, it indicates these bugs aren't due to intermediate precision and rounding.

Since then I have made some futher observations about those bugs:

AI blindness seems to apply only to the player, as they notice things like bodies and open doors.

The random colour of the skybox seems to be taking its colour from the lower left pixel of the screen. As for when it happens, I suspect it happens when certain things are in your field of view (or at least potentially visible). Holding a candle seems to provoke it more.

Knocking out a "standing still" AI while its completely unaware knocks it out but they remain upright with no visible sign that they're knocked out. If they're doing the walking on the spot thing, they'll continue to do it while asleep. The only way to tell if they're knocked out is if you bump into them and see if they react. If they're slightly suspicious they'll drop immediately. Much the same happens if you kill them while they're unaware, although death is more obvious when they don't react to damage or the arrow sticking out of them. I have dug into it much still, but I suspect the animation system is getting stuck in a state for certain AIs, and it only breaks out for certain conditions (such as suspicions and alerts).

Nice diagnosis. I've seen those string pointers misused before and winced, but only in debug code. Lots of ::Print() functions return a pointer to freed memory too. Debug print functions tend to pass the pointers uncopied to the console, which'll try to access them later. You occasionally see garbage on the console as a result if you have debug logging on.

Thanks. Stuff like stale pointers and such are things that should be fix at some point in time (even if they're just in debug mode). One thing that bothered me in the code base was seeing the use of sprintf() style functions instead of snprintf() style.

AIs are still blind to the player, some AIs just "stand still", and the skybox still changes colour.

After some more investigation, I discovered that AIs were going dormant. While dormant, they stand still and just run their animations, and, as I've seen before, they won't collapse if KOed or killed while dormant. AIs that were immune to this were those that were tagged as never dormant.

Investigating deeper into why they were being dormant, I found they were being set to dormant due to game thinking, that according to portal visibility, the AI and the player were in disconnected areas. From there the bug was found to be that the PVS wasn't allocate enough bytes for some optimisation tricks to work correctly.

After fixing that bug (see attached patch tdm--pvs-bit-correctness-fix.txt1.17KB9 downloads ) AIs no longer stood still, they could now see the player properly, and the sky box was being drawn correctly. So apparently the root cause of the remaining bugs were the same.

With that fix, my 64 bit version no longer has any known bugs. Soon I'll try to playing a good sized mission and see if I run into any more bugs.

The other attached patch ( tdm--dormant-ko-death-fix.txt563bytes5 downloads ) makes it so that AIs will collapse if they're KOed or killed while dormant due to being in a disconnected area. I'm undecided as to the value of this patch, as it doesn't affect players in normal play and will only be of interest to scripts that remotely kill/KO AIs in disconnected areas.

Thanks for this work. And kudos for your super-speedy bug hunting. It makes sense that all that could be down to the same cause: the skybox only gets drawn in a frame if the player can potentially see it.

What happens is that Left() creates a temporary idStr, and WeapName stores a pointer to its C string. Because this string is short, it will use idStr's internal base buffer and not allocate anything from the heap. Then once line has finished excuting it deletes the temporary idStr, leaving WeapName with a stale pointer.

On the next line Mid() then creates a new temporary idStr and AttName/ParName stores a pointer to its C string of that. This new temporary idStr reuses the storage for the previous temporary idStr. Because the storage is recycled, the two strings pointers (WeapName and AttName/ParName) point to the same string, which is what's meant to be in AttName/ParName.

The end result is that WeapName doesn't name a weapon name, so the weapon look up fails and it can't do the actual weapon attack/parry, however, it still plays the animation.

Whether or not this bug manifest depends upon when it deletes the temporary idStr for WeapName. If immediately it will bug with melee weapons having no effect, or if its deleted when the scope ends the bug won't happen. So ultimately its dependant on the compiler, and possibly it's optimisation options.

I looked at the patch, and it seems it just works around the issue by using different instructions. Should this really be fixed by removing the usage of c_str() everywhere (there might be a lot of other places the same happens), or should idStr be fixed to not return a pointer that later can get re-used?

"The reasonable man adapts himself to the world; the unreasonable one persists in trying to adapt the world to himself. Therefore, all progress depends on the unreasonable man." -- George Bernard Shaw (1856 - 1950)

After some more investigation, I discovered that AIs were going dormant. While dormant, they stand still and just run their animations, and, as I've seen before, they won't collapse if KOed or killed while dormant. AIs that were immune to this were those that were tagged as never dormant.

Investigating deeper into why they were being dormant, I found they were being set to dormant due to game thinking, that according to portal visibility, the AI and the player were in disconnected areas. From there the bug was found to be that the PVS wasn't allocate enough bytes for some optimisation tricks to work correctly.

After fixing that bug (see attached patch tdm--pvs-bit-correctness-fix.txt ) AIs no longer stood still, they could now see the player properly, and the sky box was being drawn correctly. So apparently the root cause of the remaining bugs were the same.

With that fix, my 64 bit version no longer has any known bugs. Soon I'll try to playing a good sized mission and see if I run into any more bugs.

The other attached patch ( tdm--dormant-ko-death-fix.txt ) makes it so that AIs will collapse if they're KOed or killed while dormant due to being in a disconnected area. I'm undecided as to the value of this patch, as it doesn't affect players in normal play and will only be of interest to scripts that remotely kill/KO AIs in disconnected areas.

The first patch is a very good find - if this opens up a 64bit version, that would be mighty cool. 64bit builds not only can access more memory and don't need 32bit compatibiity libs, but they can also use modern CPUs better - all of which are fantastic to have on Linux

The second patch looks like it can never hurt and only do good, so I'd say it should go in.

"The reasonable man adapts himself to the world; the unreasonable one persists in trying to adapt the world to himself. Therefore, all progress depends on the unreasonable man." -- George Bernard Shaw (1856 - 1950)

Thanks. Although I now realise there's a small bug in the PVS patch. LONGBITS should really be set 64, so that the size of PVS data matches on both 32 bit 64 bit platforms. Although from my reading of the code this will only affect networked games, which isn't big concern for TDM (but I do remember seeing a thread about someone working on multiplayer).

It makes sense that all that could be down to the same cause: the skybox only gets drawn in a frame if the player can potentially see it.

Yeah, it all makes sense now, but for a good while I kept trying to find a spot in the renderer code where I overlooked a mismatched data type.

Btw, a CMAKE build would be most welcome, because SCONS is really finickle and fails more than it works, and the errors are always obscure and never tell you really what you need to fix..

"The reasonable man adapts himself to the world; the unreasonable one persists in trying to adapt the world to himself. Therefore, all progress depends on the unreasonable man." -- George Bernard Shaw (1856 - 1950)

I looked at the patch, and it seems it just works around the issue by using different instructions. Should this really be fixed by removing the usage of c_str() everywhere (there might be a lot of other places the same happens), or should idStr be fixed to not return a pointer that later can get re-used?

The patch works by guaranteeing the lifespan of the the strings. WeapName can't be deleted until after statement it's used in is completely evaluated. Once that statement is finished WeapName is no longer needed and can be deleted whenever the compiler decides to. AttName/ParName can't be deleted as they point to the tail end of an idStr that's stored in part of the idAnim object that this function belongs to. Additionally, none of the statements between AttName/ParName's assignment should have side effects that affect the string they're pointing to.

While there is merit to the idea of moving away from c_str() in general, it won't be a quick undertaking, as there is a lot code that works on C-style string pointers and not dedicated string objects. Also with any string object, you'll have to consider what the performance over heads are for it, and if they'll impact performance noticeably.

The first patch is a very good find - if this opens up a 64bit version, that would be mighty cool. 64bit builds not only can access more memory and don't need 32bit compatibiity libs, but they can also use modern CPUs better - all of which are fantastic to have on Linux

Since I've fixed those last bugs, I've been running it with a 64bit build with no noticeable problems.

@NagaHuntress: I have a test case for collision issues (player gets stuck in the floor) in 64bit build (or 32bit compiled with /fp:strict). Would you care to give it a spin and potentially fix it ? (it was made for Doom 3, but I am sure it will run in TDM)

@NagaHuntress: I have a test case for collision issues (player gets stuck in the floor) in 64bit build (or 32bit compiled with /fp:strict). Would you care to give it a spin and potentially fix it ? (it was made for Doom 3, but I am sure it will run in TDM)

"The reasonable man adapts himself to the world; the unreasonable one persists in trying to adapt the world to himself. Therefore, all progress depends on the unreasonable man." -- George Bernard Shaw (1856 - 1950)

I've been doing a little reading about floating point promotions in C/C++, and dependence on extra precision on intermediates calculations is technically a bug. From what I've seen, there is no requirement in C/C++ standards to promote to double/long double on calculations with only floats, so a compiler can keep things as floats if it wants. This means MSVS2012+ does conform to the standard, even if it's not the desired behaviour. This is a problem that will need to be solved when TDM is ported to a CPU that doesn't give extra precision for free.

The idTech4 code is over 10 years old, but we're still weeding out occasional outright bugs in it still, which is to be expected. This isn't a bug in the traditional sense, it's just bad design or an oversight that unfortunately restricts our compiler options

MSVS2012+ might work if you disable SSE for floating point (or force 387 FPU).

Thanks, I'll test whether that works with MSVC too. When we discovered the problem, I spent some time looking for compiler switches that might restore the old higher-precision intermediates, as did grayman and greebo, but we didn't find any. I didn't try fiddling with SSE.

Stuff like stale pointers and such are things that should be fix at some point in time (even if they're just in debug mode).

Especially when you don't even need to care about performance. Debug functions output at most once per frame, so there's no excuse for not safely copying the string. There are a few rough edges like that, including a heap of compiler warnings (mostly from the original code) that I'd love to get round to clearing up one day. The idTech4 code has its hairy side. Those string library functions that return pointers to released memory, for example. But it also has some lovely code, especially in the front-end renderer where all the geometry gets done.