I'd like to implement savegame compression

[stgatilov]

I have already posted here that I'm trying to implement savegame compression in TDM. I have implemented the class to capture ID file input/output by using named pipes but now I feel that it was not necessary.

 

I've noticed the SourceHook code. So you have already hacked Doom3=) Seems that you can replace or add code to closed Doom3 engine functions.

 

Is it possible to get access to idFile_Memory constructor or the idFileSystem member which creates it (if it exists)? If it is possible, then we can simply use ID tools to store data to memory (so CRenderPipe would be unnecessary and implementing compression would be easy).

Edited December 28, 2010 by stgatilov

[greebo]

Can you give me a birds-eye overview about how your current implementation works? And did you manage to get D3 to recognise your compressed savegame in the Load/Save dialog?

[stgatilov]

Can you give me a birds-eye overview about how your current implementation works? And did you manage to get D3 to recognise your compressed savegame in the Load/Save dialog?

 

Yes, it is currently working (at least on my windows XP machine )

 

 

Implementation is very big and dirty. It creates named pipe like CRenderPipe in windows. There are two modes:

 

1. For small file without threads. Works exactly like CRenderPipe. It assumes that all the data fits the pipe buffer. So in case of writing: ID writes to the pipe everything it wants (e.g. screenshot), then I read from the pipe everything it has written.

 

2. For big file with threads. I don't think windows will give me 2 x 100Mb pipe buffer for savegame compression. So likely file won't fit the buffer. That's why I have to create another thread. In case of writing this thread constantly tries to read from the pipe. After all the writes are completed (end of SaveGame for example) and file is closed this thread is joined.

 

In both cases the contests of pipe are stored to std::vector. After writing the contests of this vector can be read. Reading works the similar way.

 

Since the whole save/load with compression is working now, I'll soon post a patch.

 

As you see, the way it is working now is very ugly. The problem is in limited size of pipe buffer. On linux /dev/shm can hold data of any size, so there is no need to create any threads.

 

The best solution is to make idFile_Memory working. I think pipes and /dev/shm will become unnecessary in this case. The other option is to find /dev/shm equivalent for windows and use it. Then the thread ugliness will go away.

[stgatilov]

Here is the patch...

 

I'm not permitted to attach ".patch", so I renamed it to ".txt".

savegame_compression_patch_v1.txt

[greebo]

I can't use that patch file - I'm using SVN here, and I think you created it using git? Can you just send me the files you modified, unless there is a method to convert that git patch to something TortoiseMerge can use?

[stgatilov]

Here is the modified/added files. Delete renderpipe* files.

 

http://www.mediafire.com/?3ceq80wxt021335

 

 

I hoped that hg and svn diffs were compatible. The patch looked like the usual diff...

Edited December 28, 2010 by stgatilov

[greebo]

It's crashing on me, what's the purpose of this block here?

 

memoryBufferFile = fileSystem->MakeTemporaryFile();
fileSystem->CloseFile(memoryBufferFile);
if (dynamic_cast<idFile_Memory*>(memoryBufferFile)) {
	memoryBufferFile = 0;
}

 

Opening a file, closing it again, then type-casting? This is leading to undefined behaviour, isn't it? Commenting out these lines seems to resolve the crash.

 

A conceptual question: what is keeping ourselves from implementing our own idFile subclass, which is implementing the compression on the fly?

 

Another question: have you been investigating the Linux side of things so far?

[stgatilov]

It's crashing on me, what's the purpose of this block here?

 

memoryBufferFile = fileSystem->MakeTemporaryFile();
fileSystem->CloseFile(memoryBufferFile);
if (dynamic_cast<idFile_Memory*>(memoryBufferFile)) {
	memoryBufferFile = 0;
}

 

 

 

Sorry, this was not intended to get into patch. This block is from working copy (I was investigating the type of MakeTemporaryFile result). I copied files manually, so this block got into zip. Of course it should be deleted.

 

 

A conceptual question: what is keeping ourselves from implementing our own idFile subclass, which is implementing the compression on the fly?

 

 

 

Well, I tried this way but got linker error. All the methods of idFile are unresolved symbols if you try to create an object of class derived from idFile. So I decided that it was not possible. Also I didn't wanted to implement all the Read* and Write* methods of idFile.

 

But now I think linker error can be resolved (with some hacks maybe). Something like create class with exactly the same virtual functions and C-cast its pointer to idFile*. I'll dig in this direction.

 

 

 

 

Another question: have you been investigating the Linux side of things so far?

 

 

Only in theory=) I hope that ordinary file in /dev/shm directory will be enough. It resides in memory when it is possible and it can have arbitrary size. So we

[greebo]

Reading your code and thinking a little about this, I'd suggest not to mess around with the Renderpipe to work around missing idFileMemory* references and to go ahead and implement a custom wrapper:

 

class CFileCompressed : public idFile
{
private:
// The wrapped file where compressed data will be written to / read from
idFile* _file;

// The buffer (can be resized along the way during writing)
std::vector<char> _buffer;
public:
enum Mode
{
	READ,
	WRITE,
};

// Construct a compressed file
CFileCompressed(idFile* target, Mode mode);

// implement necessary idFile methods like ReadUnsignedByte etc.

// Load a compressed block and prepare the internal buffer
bool Uncompress();

// Compress and write any collected data in the buffer to the wrapped idFile
bool CompressAndWrite();
};

 

Thoughts? This approach doesn't involve messing with a platform-specific re-implementation of the renderpipe and doesn't involve hacking with sourcehook or other horrible stunts. Implementing the few virtual Read/Write idFile methods won't take nearly as long as getting all those hacks to work in the first place.

[stgatilov]

class CFileCompressed : public idFile

 

Not so easy. You cannot derive CFileCompressed from idFile. You won't be able to construct such an object because compiler will put a call to constructor of base class like this:

 

 

//C++ must call constructor of base class:

CFileCompressed::CFileCompressed() : idFile() {}

 

 

Linker has no way to find where idFile constructor is and will output error with unresolved external idFile::idFile. Maybe this will be OK:

 

class CFileCompressed {

 //Here are declarations of exactly the same virtual functions!

}

//Virtual function pointer tables should have the same format

idFile *newfile = (idFile*)(new CFileCompressed());

newfile->Write(blablabla); //will be redirected to our code because of virtual function redirection

[greebo]

Not so easy. You cannot derive CFileCompressed from idFile. You won't be able to construct such an object because compiler will put a call to constructor of base class like this:

Ah, I missed the part that idFile is not an abstract class. Right, then this approach won't work.

 

Well, then I'd suggest going to idSaveGame directly and add another constructor? We have the full source of idSaveGame and idRestoreGame, so we can adjust them not to require an idFile* pointer. We can either change the system entirely (creating a new CCompressedSaveGame class and adjust the Save/Restore methods with Find&Replace) or making all the idSaveGame methods virtual (with a minor performance penalty) and subclassing it.

[stgatilov]

Just tried doing as I proposed.

 

savegame.ReadBuildNumber() calls readInt function of my class, so it works.

 

 

Should I start implementing idFile this way?

 

Maybe it is better to separate compression from the idFile implementation? Create for example idFile_Stream class which will call Read/Write methods of its CByteStream member. And create CByteStream implementation which actually compresses/decompresses the stream. To make further idFile implementations easier...

[stgatilov]

Ah, I missed the part that idFile is not an abstract class. Right, then this approach won't work.

 

Well, then I'd suggest going to idSaveGame directly and add another constructor? We have the full source of idSaveGame and idRestoreGame, so we can adjust them not to require an idFile* pointer.

 

It is impossible. There is some data which must be saved but is unaccessible. That's why idSaveGame calls closed source methods like this:

 

 

	if ( ui->WriteToSaveGame( file ) == false ) {

WriteToSaveGame is method inside Doom3 and it won't accept our classes - only idFile

[greebo]

It is impossible. There is some data which must be saved but is unaccessible. That's why idSaveGame calls closed source methods like this:

 

 

	if ( ui->WriteToSaveGame( file ) == false ) {

WriteToSaveGame is method inside Doom3 and it won't accept our classes - only idFile

Bummer, that's indeed a problem.

 

I'm not sure whether your method (replicating the interface and hard-casting it) is portable? I know that it works, but I'm hesitant to add a solution like this unless we absolutely have to - otherwise things could break later down the road and fixing that might cause some pain. Wouldn't it be safer to hook into the existing idFile* instance passed to idGameLocal::SaveGame() and temporarily redirect the Write/Read calls to some buffer.

 

A small helper class which is adding the sourcehook in its constructor and removing it in its destructor?

[stgatilov]

The whole TDM relies on the fact that equal class declarations give equal virtual function pointer tables. Notice that there are no symbols/addresses declared or passed related to idFileSystem methods, for example. But we call them on both windows and linux completely without any linking or checking.

 

Now consider the example. If you add your own virtual function into idFileSystem class declaration (into the middle of it), Doom will crash badly. Because the VFPT-s become different in Doom3.exe and gamex86.dll. Compiler/linker won't give you any error of warning in this case, so compiler indeed hopes that declarations are the same.

 

I suppose that it is guaranteed by C++ standard or at least GCC and MSVC compilers. I'll look it up.

[greebo]

As long as it's portable and not causing potential headaches in the future I'm fine with the changes - I just want to avoid that things suddenly break when D3 goes open source and a different coder (usually not the one who implemented it) need to spend lots of time to find the issue. You get what I mean, I reckon.

 

Let me know when there is another version to test for me.

[stgatilov]

As long as it's portable and not causing potential headaches in the future I'm fine with the changes - I just want to avoid that things suddenly break when D3 goes open source and a different coder (usually not the one who implemented it) need to spend lots of time to find the issue. You get what I mean, I reckon.

 

Let me know when there is another version to test for me.

Yes, I understand... Unfortunately there is no legal way to do the compression (except platform-specific code like pipes).

 

Kidnapping some real idFile instance is much stranger idea. Though it relies on the SourceHook hacks and won't fail unless the whole SourceHook fails...

 

 

I had a lengthy conversation with my friend about the way gamex86.dll links to Doom3.exe without any symbols, externals (except GetGameAPI) and the like and find proper virtual functions. It is almost impossible to find any documentation about it in MSDN (only hints...). And since C++ standard does not consider dynamic linking at all, it is of no help.

 

 

I send new "patch" for game save/load. Could you check that it works on linux? There is no compression now - the purpose is to check the idea.

 

http://www.mediafire...s7m3r4v87fpfw4s

[greebo]

Yup, works in Linux too. The function table seems to match, at least there were no crashes during saving nor loading.

[stgatilov]

Spend a day to hunt the uber-bug.

 

The Doom3.exe crashes on shutdown under certain circumstances. For example: Load original save -> quicksave -> quickload -> quit mission -> quit. If quicksave is repeated twice instead of once than Doom3 does not crash, which is funny... Apart from shutdown crashes, saving/loading works well.

 

The gamex86.dll is already detached at the moment of crash. The crash occurs in module Doom3.exe here:

CPU Disasm
Address   Hex dump          Command                                  Comments
00675E40  /$ >56            PUSH ESI                                 ; void idStr::FreeData( void );
00675E41  |. |8BF1          MOV ESI,ECX
00675E43  |. |8B46 04       MOV EAX,DWORD PTR DS:[ESI+4]
00675E46  |. |85C0          TEST EAX,EAX
00675E48  |. |74 46         JE SHORT 00675E90
00675E4A  |. |57            PUSH EDI
00675E4B  |. |8D7E 0C       LEA EDI,[ESI+0C]
00675E4E  |. |3BC7          CMP EAX,EDI
00675E50  |. |74 3D         JE SHORT 00675E8F
00675E52  |. |8B15 00F2B102 MOV EDX,DWORD PTR DS:[2B1F200]           ; void idDynamicAlloc::Free( type *ptr );
00675E58  |. |42            INC EDX
00675E59  |. |85C0          TEST EAX,EAX
00675E5B  |. |8915 00F2B102 MOV DWORD PTR DS:[2B1F200],EDX
00675E61  |. |74 29         JE SHORT 00675E8C
00675E63  |. |8D48 F0       LEA ECX,[EAX-10]                         ; void Mem_Free16( void *ptr );
00675E66  |. |FF0D E8F1B102 DEC DWORD PTR DS:[2B1F1E8]               ; void idHeap::Free16( void *p );
00675E6C  |. |8B01          MOV EAX,DWORD PTR DS:[ECX]   !!!!!! CRASHES HERE !!!!!!
00675E6E  |. |99            CDQ                                      ; Calculates abs(EAX)

 

 

The comments are my guesses about what this code is. So I am almost sure that this function is deallocation of idStr buffer (in Doom3.exe module).

Could you help with the following questions:

 

1. Am I right that there are at least two instances of idLib (in particular: idHeap, idStr)? One linked into Doom3 and one linked into gamex86.dll.

 

2. Are the CRT libraries different for modules? I suppose that Doom3.exe uses statically linked CRT.

 

3. What happens when idStr allocated in one module gets deallocated by another module? I have read that memory should be allocated and deallocated in the same DLL to avoid weird things.

 

Probably you have already encountered the situation like this. Maybe you even remember this crash address... Maybe you can guess what the hell is going on?

 

I'll continue kicking the bug tomorrow...

Edited December 30, 2010 by stgatilov

[greebo]

Yes, this usually happens when some memory is deallocated by another module, I usually get that when I leave an idStr or an idList around after idGameLocal::Shutdown(). Does that crash occur without any of your changes or is it due to some of your code? Or is it inherent to TDM 1.03?

 

ad 1. Yes I'm pretty certain that DOOM3 is using its own statically idLib, hence its own heap as well.

ad 2. We're using a statically linked CRT from VC++ 2008. DOOM3.exe was linked using MSVC 2003 (I think?) so the CRT is highly likely to be different. Shouldn't be a problem though if everything is shut down correctly, as said above.

ad 3. The debug heap is complaining about that, not sure if there is any difference in release builds. I once had crashes in DarkRadiant in those situations, but generally I'm not an expert on heaps.

 

The best approach is to narrow down the code section causing this by selectively reverting/commenting code, but I suppose you know that.

[stgatilov]

Yes, this usually happens when some memory is deallocated by another module, I usually get that when I leave an idStr or an idList<something> around after idGameLocal::Shutdown(). Does that crash occur without any of your changes or is it due to some of your code? Or is it inherent to TDM 1.03?

 

 

No, my changes cause the problem. The bad quicksave is properly loaded by 1.03 without any problem, but loading it in my version causes bad shutdown.

 

 

If idLibs are different, we cannot edit idStr object which was created in another module?... Because Doom3 alloced string with its own string allocator, then we try to change the string and get deallocation + allocation in gamex86.dll. And when Doom3 shutdowns, the string buffer is deallocated by Doom3.exe. Seems like two improper deallocations.

 

I'll try commenting WriteString and ReadString to see if they cause the trouble.

 

Thanks for help! Now I don't feel that lost...

[greebo]

If idLibs are different, we cannot edit idStr object which was created in another module?... Because Doom3 alloced string with its own string allocator, then we try to change the string and get deallocation + allocation in gamex86.dll. And when Doom3 shutdowns, the string buffer is deallocated by Doom3.exe. Seems like two improper deallocations.

That's theoretically possible, yes. But I'm curious, when do we encounter such a sitaution? Most of the closed source methods return raw const char* pointers, so we shouldn't have access to any idStr or lists managed by DOOM3.exe, or have we?

[stgatilov]

That's theoretically possible, yes. But I'm curious, when do we encounter such a sitaution? Most of the closed source methods return raw const char* pointers, so we shouldn't have access to any idStr or lists managed by DOOM3.exe, or have we?

There is idFile::ReadString(idStr &string) which has to change idStr.

 

In version 1.03 all the strings from Doom3 are read by idFile routines. And all the strings from TDM are read by idSaveGame::ReadString that uses gamex86.dll heap. Since now I try implement idFile replace, there are calls from Doom3.exe to CIDFileFrontend::ReadString.

 

I commented the commands that change idStr in ReadString, that does not help... So that may be wrong guess.

EDIT: forgot to rebuild...

Edited December 31, 2010 by stgatilov

[stgatilov]

Now I'm sure about the problem.

 

In idGameLocal::RestoreGame the ONLY place where idStrs are loaded is a call to savegame.RestoreObjects();

 

This call in turn calls idRestoreGame::ReadSoundCommands() which redirects the flow to closed part of engine with gameSoundWorld->ReadFromSaveGame( file );

 

That closed part of engine starts reading strings (I suppose names of sound files). The first string does not cause any problem since it has len<32 and is allocated in idStr::baseBuffer inside idStr object. The second string like "sounds/lights/light_flicker.wav" has length=33 and is allocated by my piece of code (CIDFileFrontend::ReadString) in gamex86's idHeap string allocator's memory. Then everything goes smooth.

 

When game shutdowns, Doom3's part of code tries to deallocate the first idStr which was allocated inside gamex86. Address of its buffer is equal to the address of that light_flicker allocated string buffer I mentioned. Since it was already deallocated when gamex86 was detached, we get a crash.

 

Is it possible to find workaround?... I'm afraid that if I load that save and continue playing for a long time Doom3 will try to deallocate that string and I'll get that crash, but not during shutdown. This error is critical. What does gameSoundWorld->ReadFromSaveGame do?

[greebo]

I can see the problem. How does your CIDFileFrontend::ReadString() method look like?

 

Is it correct that when your CIDFileFrontend::ReadString() method tries to change the foreign idStr it still invokes the gamex86 idStr methods, as they are inlined and there are no actual symols to call?

 

Maybe it's possible to remember those strings and to call FreeData() on them right on idGameLocal::Shutdown()? That is risky though as the idSoundWorld could potentially remove strings during ordinary operation and our pointers would be stale by then. Not a good solution, I think.