Thoughts on refactoring and futher improving the engine

[JJS]

Common/core/types.h:
Shouldn't we use the stdint.h names with _t suffix (int32_t instead of int32)? Actually I would say that including stdint.h where supported and only typedefing ourselves on Windows should be preferred.


Configuration file reading:
I think the reading should be split in two parts: one general and one for each platform. For example the sound device selection is platform-specific while the "use speech pack" option is platform independent.


You used a leading underscore for member variables (at least in CEngine). I would be in favour of marking member variables like that instead of not marking them.


void CAGSEngine::winclosehook()
I don't know if this a callback, but it looks like it. If so, it has to be declared static. Otherwise its signature doesn't match the exprected C function prototype. Just saying, I know the code is not at all final.
Fake edit: This is the same situation as with the sound thread entry points, they have to be static I think.


CLIB:
Every time a resource inside the game exe or another archive is opened, this is done through

Code: C

csetlib(archive_file_name, "");
FILE *f = clibfopen("filename.ext", "rb");
fread(); fscanf(); //... whatever
fclose(f);

I wonder if this can be converted to use the stream classes, but I am not sure it can in any case. This is because of the way clib interacts with the Allegro functions which expect a FILE pointer and being able to use stdio functions for access.

[Crimson Wizard]

You used a leading underscore for member variables (at least in CEngine). I would be in favour of marking member variables like that instead of not marking them.

I combined monkey's suggestion and mine own suggestion here: public member variables named CamelCase and have no marking, internal (private and protected) named using "lower" camelCase; leading underscore helps to distinguish one-word member names from local variables, but I added them to all internal vars in sake of consistency.

void CAGSEngine::winclosehook()
I don't know if this a callback, but it looks like it. If so, it has to be declared static. Otherwise its signature doesn't match the exprected C function prototype. Just saying, I know the code is not at all final.

Ah, I actually missed that one.
For other callback, atexit_handler, I did this:

Code: cpp

...
atexit(atexit_handler); // register global function
...

void atexit_handler() {
    CAGSEngine::GetInstance()->atexit_handler();
}

Well, practically the same, just not static member but global function. Don't know which is better; I made Engine a singleton anyway.

[JJS]

I guess the downside of a static member as the callback is that it can itself only access static member variables. But since it is a singleton it doesn't matter really. Either the class instance must be resolved in the static function or in the C helper function - same difference.
The static method has the advantage of not needing that helper function though.

[BigMc]

I just tried to build the refactory branch on Linux and the Makefile is quite outdated again. I saw that you tried to keep it up to date, but that's of course difficult without testing it. Is it possible to arrange the files into folders in such a way that most folders contain only files that are built on all platforms (say all but Common/platform and Engine/platform)? Then all the makefiles would just need a list of folders and a list of files from the platform folder.

[Crimson Wizard]

Is it possible to arrange the files into folders in such a way that most folders contain only files that are built on all platforms (say all but Common/platform and Engine/platform)? Then all the makefiles would just need a list of folders and a list of files from the platform folder.

Not only possible but certainly should be done. I'll check into that, but first I have to finish something. I decided to make a big change at once, but it takes longer than I thought.
I'll post here when it's done.

[BigMc]

The alternative would be to switch to a cross-platform build system like CMake. This will certainly become the best option at some point, but folder based building may also do for now.

EDIT: BTW, http://gitorious.org/ags uses CMake, so their CMakeLists.txt's could be used as a starting point.

[Crimson Wizard]

Alright I am done!
I mean it. Done.
Phew.


The clean-up stage is now "officially" over.
What's left is make sure engine compiles on other supported platforms. Also copy the latest changes from 'main'. Perhaps from 64bit too, but only after it is merged with main somehow.

@BigMC, I've put all platform-dependent files into platform/<platform-name> folders. I believe the rest could be used for any platform. Except the code in obsolete folders- they should not be used at all.

I hope those porting issues will be eventually worked out. Sincerely I got really bored with this code-tidying task and want to move further and do some other interesting things.


//--------------------------------------------------------------------------------------------------
EDIT: some useless statistics (does not include library sources and patches, and unused code):

AGS 3.2.1 SVN:

Common:
- headers: 15
- sources: 12
Engine:
- headers: 4
- sources: 24
Total:
- headers: 19
- sources: 36

Refactory branch:

Common:
- headers: 51
- sources: 39
Compiler:
- headers: 11
- sources: 9
Engine:
- headers: 206
- sources: 220
Total:
- headers: 268
- sources: 268 <--- good coincidence

[Joseph DiPerla]

Awesome work CW! When I get a chance today I will try and compile the source to see if it works for me. Thanks for all your hard work on this!

[BigMc]

@BigMC, I've put all platform-dependent files into platform/<platform-name> folders. I believe the rest could be used for any platform. Except the code in obsolete folders- they should not be used at all.

Great! I commited changes that make it build on Linux again. There were some more Windows and DOS only files and I moved them. It should now be much easier to make it work on the other platforms. Is it something for the coding conventions that all files that are not intended for all platforms should go into "platform"? Also, aren't some of the files currently in the platform folders intended for all platforms and should therefore go somewhere else?

[Crimson Wizard]

Is it something for the coding conventions that all files that are not intended for all platforms should go into "platform"?

Well, maybe. I don't see anything bad in that. There should not be too much of them anyway.

Also, aren't some of the files currently in the platform folders intended for all platforms and should therefore go somewhere else?

I think I was going to put platform-dependent functions and declarations there, like File routines, but now I am not sure. Perhaps function prototypes could be put somewhere else and endian-specific implementation left in platform.
As for agsplatformdriver code, well, it is called 'platform driver' so I guess it belongs there .
I gotta think more about that.

[Joseph DiPerla]

OK, so your new source compiles successfully, but now when I run ACWIN.exe, it crashes. I am using 64-bit windows 7 if that makes a difference, but I do not see how since all other versions of AGS worked before, even the refactored code. Not that this seems very helpful, but here is the error report I get:

Code: AGS

Problem signature:
  Problem Event Name:	APPCRASH
  Application Name:	acwin.exe
  Application Version:	3.2.1.1115
  Application Timestamp:	5002b3d2
  Fault Module Name:	acwin.exe
  Fault Module Version:	3.2.1.1115
  Fault Module Timestamp:	5002b3d2
  Exception Code:	c0000005
  Exception Offset:	00094d16
  OS Version:	6.1.7601.2.1.0.768.3
  Locale ID:	1033
  Additional Information 1:	0a9e
  Additional Information 2:	0a9e372d3b4ad19135b953a78882e789
  Additional Information 3:	0a9e
  Additional Information 4:	0a9e372d3b4ad19135b953a78882e789

Read our privacy statement online:
  http://go.microsoft.com/fwlink/?linkid=104288&clcid=0x0409

If the online privacy statement is not available, please read our privacy statement offline:
  C:\Windows\system32\en-US\erofflps.txt

[Crimson Wizard]

Hmm, runs for me. I too have Win 7 64 bit.
How do you run that exactly?
Do you build and run plain acwin.exe?
Or do you build editor and compile and run some game from editor?
Or else?

I really need more detail.
Also, since you are compiling that, you could make Debug version and run from the MSVS debugger.

[Joseph DiPerla]

I am running ACWIN as is with no game data and not running it through the editor. I was just testing it. Normally what happens when I do that is that I get a long winded message telling me that it needs game data files. Instead, this time, it just crashes on me. Are you still using VC++ 2008 Express or did you upgrade to 2010?

[Crimson Wizard]

I am running ACWIN as is with no game data and not running it through the editor. I was just testing it. Normally what happens when I do that is that I get a long winded message telling me that it needs game data files. Instead, this time, it just crashes on me. Are you still using VC++ 2008 Express or did you upgrade to 2010?

Hmmm... I did the same, and I got that message.
I am using full VC 2008 version, not Express one. Unfortunately I don't remember the differences, I thought they mainly related to mixed code.

Maybe you could run debug version and at least see where it crashes exactly?

[Joseph DiPerla]

First error:

main.cpp at line 426 with END_OF_MAIN(), I get this: Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.

Here is the output from debug:

Code: AGS

'acwin.exe': Loaded 'C:\Joeys fun projects\ags sources\ags-final-eng-refact2\adventuregamestudio-ags-08d4d5f\Solutions\.build\Debug\acwin.exe', Symbols loaded.
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ntdll.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\kernel32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\KernelBase.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\quartz.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\msvcrt.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\gdi32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\user32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\advapi32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\sechost.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\rpcrt4.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\sspicli.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\cryptbase.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\lpk.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\usp10.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ole32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\oleaut32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\winmm.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\shell32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\shlwapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\opengl32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\glu32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\ddraw.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dciman32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\setupapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\cfgmgr32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\devobj.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dwmapi.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dinput.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\dsound.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\powrprof.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\imm32.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\msctf.dll'
'acwin.exe': Loaded 'C:\Windows\SysWOW64\nvinit.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\detoured.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\Nvd3d9wrap.dll'
'acwin.exe': Loaded 'C:\Program Files (x86)\NVIDIA Corporation\coprocmanager\nvdxgiwrap.dll'
First-chance exception at 0x012c4d16 in acwin.exe: 0xC0000005: Access violation writing location 0x00000218.
Unhandled exception at 0x77e915de in acwin.exe: 0xC0000005: Access violation writing location 0x00000218.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.
First-chance exception at 0x77e8016e in acwin.exe: 0x00000000: The operation completed successfully.
Unhandled exception at 0x77e915de in acwin.exe: 0x00000000: The operation completed successfully.

Here is the output from callstack:

Code: AGS

>	ntdll.dll!77e915de() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	ntdll.dll!77e915de() 	
 	ntdll.dll!77e8014e() 	
 	kernel32.dll!757e7814() 	
 	ntdll.dll!77ee74df() 	
 	ntdll.dll!77ee73bc() 	
 	ntdll.dll!77ed3c48() 	
 	ntdll.dll!77e8016e() 	
 	kernel32.dll!757e7814() 	
 	ntdll.dll!77ee74df() 	
 	ntdll.dll!77ee73bc() 	
 	ntdll.dll!77ed3c48() 	
 	ntdll.dll!77e8016e() 	
 	acwin.exe!_getptd_noexit()  Line 618	C
 	acwin.exe!014aedcf() 	
 	ntdll.dll!77ecb459() 	
 	ntdll.dll!77ecb42b() 	
 	ntdll.dll!77e9edff() 	
 	acwin.exe!_getptd_noexit()  Line 618	C

[Joseph DiPerla]

Also lines 408 with this code in main.cpp gives me an error it seems:

Code: AGS

#ifndef USE_CUSTOM_EXCEPTION_HANDLER
    the_engine->GetSetup()->DisableExceptionHandling = 1;
#endif

[Joseph DiPerla]

Also, when I try to compile release, I get this:


3>libcmt.lib(wincrt0.obj) : error LNK2001: unresolved external symbol _WinMain@16
3>C:\Joeys fun projects\ags sources\ags-final-eng-refact2\adventuregamestudio-ags-08d4d5f\Solutions\\.build\Release\acwin.exe : fatal error LNK1120: 1 unresolved externals

[Crimson Wizard]

Also lines 408 with this code in main.cpp gives me an error it seems:

Code: AGS

#ifndef USE_CUSTOM_EXCEPTION_HANDLER
    the_engine->GetSetup()->DisableExceptionHandling = 1;
#endif

WAAAAIT a minute.
Did I told NOT to run the "pre-advance" branch? It is NOT supposed to work. It simply WON'T.
Use "refactory" branch instead.

[Joseph DiPerla]

Oh crud... I thought I was. Whoops, I was running the project solution in the wrong folder. STUPID FOLDER DISORGANIZATION. My bad!

[Crimson Wizard]

Heh.
Well, I hope it will settle up for you. I must go sleep now so no more tech support today