Outpost Universe Forums

Projects & Development => Projects => Outpost 2 Update => Topic started by: Vagabond on November 05, 2017, 12:16:41 AM

Title: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 05, 2017, 12:16:41 AM
Just wanting to put down my thoughts on this subject in a dedicated thread. I've spent a bit of time reviewing the OP2Ext code in the repository: https://svn.outpostuniverse.org:8443/!/#outpost2/view/head/op2ext/trunk. The code is written for VC++6. Mostly in C++ with one C file.

I was thinking the approximate order of business would be:

 * Push the code responsibly into the GitHub repository (assuming that we want to remove it from the SVN repo)
 * Create a new project using VS2017 and get the code compiling
 * Test a new compilation with Outpost 2 and make sure different features seem to be working
 * Update the ReadMe to be more robust. (Add where to get Outpost 2, a changelog, semantic versioning (http://semver.org/), a small blurb explaining each feature, etc)
 * Actually Change the vol loader code
 * Look at assigning a license for the code (or not?)



It looks like the original authors are BlackBox and Hooman. I would be interested if either one had any thoughts on the matter.

A couple of other details. I wonder if it is important to disable Randomized Base Address (ASLR) and to disable safe exception handling in VS2017 for this project? I don't want it to load with the same base address where scenarios go for sure, but I'm also not sure if it matters beyond that and where it should be loaded.

We should think about separating the contents of the VOL files in a way that supports the mod loader nicely. I know sirbomber's multitek2 modifies sheets.vol, so if we change the name or split the files in there, it would cause him to have to change up multitek2. Not that I see any reason to muck with sheets.vol, just an example. ARE THERE ANY OTHER MODS THAT CURRENTLY CHANGE OUT OTHER VOL FILES?

This would ease modding a little bit. So say you could just add an archive called GreenWorld.vol that contains the tilesets, maps, etc needed without a reserved maps04.vol for such a purpose.

Proposed new arrangement:

 * StockMaps.vol
 * StockTechTrees.vol
 * CustomMaps.vol
 * CustomTechTrees.vol
 * art.vol (Stock tileset, op2_art.prt, color.bmp & virmask.raw)
 * sheets.vol (leave alone)
 * voices.vol (leave alone)
 * story.vol (leave alone)
 * sound.vol (leave alone)

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 05, 2017, 11:20:34 AM
It looks like the original authors are BlackBox and Hooman. I would be interested if either one had any thoughts on the matter.
I also contributed a little to it. I was meaning to rewrite the whole thing but never got around to it. That said, you'll probably need to be familiar with ASM hacking/reverse engineering to work on this thing, which is a bit of a learning curve cliff to say the least.

* Create a new project using VS2017 and get the code compiling
Can we please not have VS2017 projects? It doesn't offer any new C++ features over 15 IIRC, really all it has to offer is an "improved" version of Intellisense which does wonderful things like put red squiggles everywhere, even in 2 random characters in comments and other things that are the opposite of, I don't know, intelligence and sensibility?

Ideally we'd replace all the MSVC project files in the repo with CMake configs, which is a meta-build system that can generate any version of MSVC project files, GNU makefiles, etc. which is awesome, though it'll be more complicated to set up than a MSVC project.

Quote
A couple of other details. I wonder if it is important to disable Randomized Base Address (ASLR) and to disable safe exception handling in VS2017 for this project? I don't want it to load with the same base address where scenarios go for sure, but I'm also not sure if it matters beyond that and where it should be loaded.
Technically speaking, op2ext itself should be ASLR-friendly, but a lot of things aren't, namely anything that uses ForcedExports such as NetFix. If you don't disable ASLR on op2ext then it's entirely possible that op2ext ends up at the address NetFix wants to load at, completely breaking it. I think 0x11000000 was a typical base address for mission DLLs, and 0x17000000 and up are typical bases for mod DLLs, and I think op2ext was 0x13000000? You can check.

Quote
We should think about separating the contents of the VOL files in a way that supports the mod loader nicely. I know sirbomber's multitek2 modifies sheets.vol, so if we change the name or split the files in there, it would cause him to have to change up multitek2. Not that I see any reason to muck with sheets.vol, just an example. ARE THERE ANY OTHER MODS THAT CURRENTLY CHANGE OUT OTHER VOL FILES?

This would ease modding a little bit. So say you could just add an archive called GreenWorld.vol that contains the tilesets, maps, etc needed without a reserved maps04.vol for such a purpose.

Proposed new arrangement:

 * StockMaps.vol
 * StockTechTrees.vol
 * CustomMaps.vol
 * CustomTechTrees.vol
 * art.vol (Stock tileset, op2_art.prt, color.bmp & virmask.raw)
 * sheets.vol (leave alone)
 * voices.vol (leave alone)
 * story.vol (leave alone)
 * sound.vol (leave alone)

-Brett
I'd probably go with something like Maps.vol and Tech.vol instead of StockMaps and StockTechTrees, but otherwise that's fine.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 06, 2017, 12:57:38 AM
Sounds like a reasonable plan.

Quote
Ideally we'd replace all the MSVC project files in the repo with CMake configs, which is a meta-build system that can generate any version of MSVC project files, GNU makefiles, etc. which is awesome, though it'll be more complicated to set up than a MSVC project.

Arklon, would you be able to provide a walkthrough on how to do this? I've never written a CMake file before, nor do I know how to generate Visual Studio project files from one.

The Visual Studio project file version differences are a little annoying. Especially since you can often only download the latest version, and it tends to force upgrade of project files when you use a newer version. It would be great to have a way around this, and it sounds like CMake offers such a solution.

Quote
Technically speaking, op2ext itself should be ASLR-friendly, but a lot of things aren't, namely anything that uses ForcedExports such as NetFix. If you don't disable ASLR on op2ext then it's entirely possible that op2ext ends up at the address NetFix wants to load at, completely breaking it.

Indeed, this is something I've wanted to improve about ForcedExports and related projects. Perhaps by complicating the syntax within ForcedExports, references can be set that could update based on the load address of Outpost2.exe, while still maintaining conventional object access syntax for users of the library.

I've also wanted to review some of the earlier patches to OP2, since the relocation table was not correspondingly updated, and so would have issues if the executable was ever relocated. This happens when you try to load a mission in a debugger. It first loads a dummy exe at the preferred load address before it loads the DLL, which references Outpost2.exe which must then be brought in at a relocated address.

Quote
I'd probably go with something like Maps.vol and Tech.vol instead of StockMaps and StockTechTrees, but otherwise that's fine.

I agree. I'd also suggest not having the "Custom" vol files, since that would require cooperation between different level designers. Who would be responsible for maintaining a consistent file? Probably easier to just let people choose their own names, and perhaps have some naming guidelines. Like name a vol after the level if it's level specific. Each vol file would have contents from only one author.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 07, 2017, 12:46:42 AM
Arklon and Hooman,

Thanks for the feedback.

Quote
Can we please not have VS2017 projects? It doesn't offer any new C++ features over 15 IIRC

I would prefer using the newest stable version of Visual Studio. This way, when the file is reviewed again in 5 years, it is on a 5 year old toolchain instead of a 7 year old toolchain. Although you are right that we do not strictly need anything in VS2017 that VS2015 doesn't have. If you would be interested in reviewing the code, but unable if we do not work in 2015, than I will certainly download and use VS2015.

Quote
Ideally we'd replace all the MSVC project files in the repo with CMake configs, which is a meta-build system that can generate any version of MSVC project files, GNU makefiles, etc. which is awesome, though it'll be more complicated to set up than a MSVC project.

Unfortunately, this is beyond my current skillset. Although I don't have any problems with the idea, especially if someone else wants to help and document it well.

Quote
Technically speaking, op2ext itself should be ASLR-friendly, but a lot of things aren't, namely anything that uses ForcedExports such as NetFix. If you don't disable ASLR on op2ext then it's entirely possible that op2ext ends up at the address NetFix wants to load at, completely breaking it. I think 0x11000000 was a typical base address for mission DLLs, and 0x17000000 and up are typical bases for mod DLLs, and I think op2ext was 0x13000000? You can check.

Thanks, I'll try to get that documented in the ReadMe and make sure it uses 0x13000000. I'll check and make sure that is the right number as well.

Quote
I've also wanted to review some of the earlier patches to OP2, since the relocation table was not correspondingly updated, and so would have issues if the executable was ever relocated. This happens when you try to load a mission in a debugger. It first loads a dummy exe at the preferred load address before it loads the DLL, which references Outpost2.exe which must then be brought in at a relocated address.

Okay, well that is definetly beyond my skillset. Happy to learn during a pair programming session though. As Arklon mentioned, the ASM Hacking will be very challenging for me at best. Man I'm going to be a terrible partner for the next pair programming session.

As for Vol File naming, I was wanting to capture the original campaign and multiplayer maps in a a separate vol file from OPU content that ships with the game. This way if you want to say replace things in the original campaigns in a mod, it would be fairly future proofed against us adding new maps to future versions of the game. If that makes sense? Perhaps it is a moot point though since I'm not sure anyone can/will want to mess with the stock tech trees and maps in any mods.

What I envisioned is if someone wanted to mod the tech tree in the Plymouth campaign, they could replace the .VOL file containing it. OPU maps making it into the next release of Outpost 2 would be placed in a separate vol file from the original Sierra/Dynamix content. This way when the next release comes out, you can still use the addon and get the new map without the addon needing to be updated. But since no one to my knowledge has done anything like this, maybe it doesn't matter if our new maps are in the same vol file as the original Dynamix maps?

It didn't occur to me that I could create some sort of vol file with all my content and distribute. VagabondsMapPack.vol or something...

Really a decent part of why I'm doing this so we can name a vol file GreenWorld.vol and not have a random empty maps04.vol around. As long as what we decide on naming makes more sense than the current maps0-4.vol, I'll be pretty happy.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Sirbomber on November 07, 2017, 03:02:53 PM
What I envisioned is if someone wanted to mod the tech tree in the Plymouth campaign, they could replace the .VOL file containing it.

Wouldn't it be better to have some sort of system in place where a modder can define file replacements?  Using your example, some file somewhere containing the string "customplymtek.txt replaces plymtek.txt"?  Though I would think most modders would be content with the current system - just drop the modded file in your mod folder and it will supersede any versions of the file in any VOLs.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 07, 2017, 09:10:46 PM
Quote from: Vagabond
I would prefer using the newest stable version of Visual Studio. This way, when the file is reviewed again in 5 years, it is on a 5 year old toolchain instead of a 7 year old toolchain.

Agreed. I'd rather be using the latest release of something.

Quote from: Vagabond
... than I will certainly download and use VS2015.

Can you even still do this? Where do you find the download? After a while it gets hard to find downloads for older versions.

Quote from: Vagabond
Okay, well that is definetly beyond my skillset. Happy to learn during a pair programming session though. As Arklon mentioned, the ASM Hacking will be very challenging for me at best. Man I'm going to be a terrible partner for the next pair programming session.

I would certainly be happy to show what I know, though at this point I'm getting a bit fuzzy on the details. It would be two people stumbling around a bit, and googling documentation.

Quote from: Vagabond
Really a decent part of why I'm doing this so we can name a vol file GreenWorld.vol and not have a random empty maps04.vol around. As long as what we decide on naming makes more sense than the current maps0-4.vol, I'll be pretty happy.
I think that's reason enough. Better file names will help people find the content they want faster.

Quote from: Sirbomber
Though I would think most modders would be content with the current system - just drop the modded file in your mod folder and it will supersede any versions of the file in any VOLs.
Yes, I think the current system works well enough. Things can be updated/replaced if necessary, and without having to edit or delete the originals.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 08, 2017, 07:17:32 PM
Hooman,

https://www.visualstudio.com/vs/older-downloads/

Goes back to VS 2010. This is new though. I couldn't find 2010 a while back when I needed it.

sirbomber,

That makes sense. No reason to worry about it if you can already override individual files.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 11, 2017, 11:36:47 PM
Hooman migrated the code over to GitHub nicely.

I updated ReadMe.txt to test if I could push into the REPO (which I could).

I tagged Ver1.0.0 of the project and put the current version of op2ext.dll on the release page. This way we will always have a copy of the original file in a reasonably logical place. I did add fileinfo to the op2ext.dll using Resource Hacker before posting it.

I then migrated the solution from VC++6 to VS2017. I combed through the settings files some and set ASLR (Dynamic Base Address) to FALSE and set the address listed earlier in this thread.

I then tried to compile, but got the warnings and errors below. Interestingly enough, several errors were from a file missing the definition for 'i' used as a counting variable for a loop.

A couple of errors were associated with having the wrong number of input argument on std library functions. I'm not sure what is going on there, perhaps older definitions have been deprecated and dropped out of the newer Microsoft build environment?

It looks like we are currently using the deprecated versions of several functions (at least deprecated from Microsoft's standpoint). We will have to decide whether or not we want to change the functions used or set _CRT_SECURE_NO_WARNINGS for the project. I'm leaning towards just changing the functions used so the compiler doesn't whine about it.

I also noticed the following 2 files were not included in the project after the migration. Not sure if that is on purpose or not:

 * modlibrary.c
 * public.h

We should get at least a quick test of each modification provided by op2ext before proceeding. Testing the IP address and .VOL code should be quick and easy just by loading the project. Maybe we can try loading multitek2 or another mod to test the mod loader is working after compiling. I don't know if we want to jump into Unit Testing for this (or if it is even feasible on this project). I'm happy either manually testing or using GoogleTest depending on what Hooman/others want.

I uploaded the VS2017 solution to GitHub in case Hooman wanted to review before the session. We will probably want to delete the VC++6 solution files once everything is working sense we will not be updating it with changes.

So first thing during the pair programming will probably be getting things to compile and then testing that the compiled version is working as advertised.



Current compile errors and warnings:

Warning   C4996   '_splitpath': This function or variable may be unsafe. Consider using _splitpath_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   94
   
Warning   C4996   '_snprintf': This function or variable may be unsafe. Consider using _snprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   IniMods.cpp   45   

Warning   C4996   '_snprintf': This function or variable may be unsafe. Consider using _snprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   159   

Warning   C4996   '_snprintf': This function or variable may be unsafe. Consider using _snprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   192

Warning   C4996   '_snprintf': This function or variable may be unsafe. Consider using _snprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   228

Warning   C4477   '_snprintf' : format string '%x' requires an argument of type 'unsigned int', but variadic argument 1 has type 'void *'   op2ext.cpp   192

Warning   C4477   '_snprintf' : format string '%x' requires an argument of type 'unsigned int', but variadic argument 1 has type 'void *'   op2ext.cpp   228
   
Error   C2660   'WritePrivateProfileStringA': function does not take 2 arguments   IpDropDown.cpp   71

Warning   C4996   'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   IniMods.cpp   24

Warning   C4996   'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   IniMods.cpp   50
   
Warning   C4996   'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   20
   
Warning   C4996   'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   34
   
Warning   C4996   'strtok': This function or variable may be unsafe. Consider using strtok_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   63
   
Warning   C4996   'strncpy': This function or variable may be unsafe. Consider using strncpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   17
   
Warning   C4996   'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.   ModMgr.cpp   59

Warning   C4996   'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   IpDropDown.cpp   53

Warning   C4996   'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   IpDropDown.cpp   66
   
Error   C2660   'strcpy': function does not take 1 arguments   IpDropDown.cpp   63
   
Warning   C4996   'strcmpi': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strcmpi. See online help for details.   ModMgr.cpp   31
   
Warning   C4996   'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   50
   
Warning   C4996   'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   ModMgr.cpp   83
   
Warning   C4996   'strcat': This function or variable may be unsafe. Consider using strcat_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   103
   
Warning   C4996   'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   95
   
Warning   C4996   'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   117

Warning   C4996   'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   123

Warning   C4996   'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.   op2ext.cpp   134
   
Warning   C4996   'itoa': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _itoa. See online help for details.   IpDropDown.cpp   20

Error   C2065   'i': undeclared identifier   op2ext   IpDropDown.cpp   62
Error   C2065   'i': undeclared identifier   op2ext   IpDropDown.cpp   63
Error   C2065   'i': undeclared identifier   op2ext   IpDropDown.cpp   70
Error   C2065   'i': undeclared identifier   op2ext   IpDropDown.cpp   71
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 12, 2017, 11:30:06 PM
Hooman and I completed a successful pair programming round today on op2ext.

The new home for the source code is https://github.com/OutpostUniverse/op2ext.

We fixed the compiler errors and edited the code to remove all compiler warnings associated with 'deprecated' (as defined by Microsoft) functions. Most of the deprecated functions were in relation to char* manipulation in various forms.

!!! BREAKING CHANGE !!!
We created a new signature for the function GetGameDir that provides bounds checking. THIS IS A BREAKING CHANGE for other DLLs if they are calling GetGameDir. IF ANYONE IS USING the function GetGameDir IN ONE OF THEIR PROJECTS PLEASE LET US KNOW SO WE CAN WORK THROUGH IT. Since we replaced GetGameDir with a templated method, we are having difficulty thinking of a way to notify the user if they try calling the old message signature in a meaningful manner (trying to avoid just crashing horribly with no explanation.)

The code assumes a specific file structure relationship between your installed Outpost 2 directoy and where op2ext is. You will have to review the post build script before compiling to align the directory structure.

We took a crack at dynamically loading vol files of any name. Unfortunately, we were unsuccessful so far. The code is uploaded to the repository as a separate branch until it becomes functional.

Some Future plans:

* Get dynamically loading vol files working.
* Replace NULL with nullptr where appropropriate in the source code for returning null pointers.
* Add instructions in the ReadMe for how the mod loader is supposed to work.
* Change GetGameDir over to a std::string instead of char* since we are breaking it for the next release
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 13, 2017, 12:23:58 AM
Nice writeup. Thanks for doing these writeups.


To add to what Vagabond said, we added bounds checking to GetGameDir, which requires an additional parameter. The additional parameter means a breaking change to the function signature. If other libraries link to op2ext and call that function, it could cause a crash due to binary incompatibility. This is something I'd like to address before releasing our changes. Perhaps we could address this by using a new name for the updated function.

Related, we added a helper template method that will auto fill in the additional parameter to GetGameDir for the common case where a statically sized buffer is passed as the first argument.

Quote
The code assumes a specific file structure relationship between your installed Outpost 2 directoy and where op2ext is. You will have to review the post build script before compiling to align the directory structure.

To clarify, this only affects developers trying to compile op2ext. The post build script contains a relative path from the op2ext source code folder to a test copy of Outpost 2. This copies the new DLL to the Outpost 2 folder, making it quicker to test changes. Developers would need to adjust this path to match their local environment.


I'm looking forward to working on the future plans.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 13, 2017, 02:56:53 PM
Hooman,

Thanks for clarifying my writeup. I'll try to be a bit more specific next time. :)

In regards to GetGameDir, what do you feel about this plan:

Code: [Select]
||deprecated("GetGameDir was deprecated in op2ext ver1.1.0. Use GetGameDirectory instead.")|| (*Had to replace brackets with pipes to make forum happy)
EXPORT void GetGameDir(char* buffer)
{
    std::string str = GetGameDirectory();
    std::vector<char> writable(str.begin(), str.end());
    writable.push_back('\0');

    buffer = &writable[0];
}

EXPORT std::string GetGameDirectory()
{
    ... [code] ... 
}

This code will throw a warning since std::string cannot be returned and support export to c code. If we are only expecting export to C++ code, it is fine though. Is this okay, or do we need to support interoperability with C code?

https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4190

I cannot seem to get the deprecated attribute to work for some reason. C++14 feature. There seems to also be a DEPRECATED attribute already defined in  WinNls.h which may be conflicting? Not sure.

Basically, GetGameDir would call GetGameDirectory and then convert the string into a char*. This is a little ugly since it cannot return a const char*.

-Brett

------------------------------------------

EDIT: I have ||deprecated|| working now. It has to be placed on the declaration, not the definition of the function. (I thought I tried this earlier, but now it is working, so oh well).
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 13, 2017, 11:20:13 PM
Yes, I think you have the right idea.

The implementation doesn't appear to be correct though. The original function is passed a pointer to a buffer, which is done by copying the pointer (or computed value) from the caller function to the called function parameter list (the pointer itself is passed by-value). The called function then sees the same buffer, using the same pointer value, but through a different copy of the pointer. At the end of the suggested function, an assignment is made to the copy of the buffer pointer (not a write to the buffer itself). This is done just before it goes out of scope, so it would never be used and have no effect.

You'll want to actually write to the buffer, rather than adjust the pointer. You can do so with a strcpy, or even a memcpy from the std::string to the char* buffer.


Also, since we recently talked about it, using the double pointer trick (char** buffer) won't work here. It's assumed the caller owns the buffer (which might very well be a statically sized local variable), so it wouldn't make sense for the called function to assume ownership and try to allocate new space and update a pointer to it. There might not even be a pointer to the source buffer in the caller's stack frame, since it may very well be a computed value (such as a relative offset from the stack pointer to a local variable). Plus, adding the extra * would also be a breaking interface change.


As for the warning, the function could be exported but without specifying "C" linkage. That would export the decorated name (which allows overloads to be exported too). That's possibly a better option if a C++ interface is desired. Note that EXPORT is a macro defined within the project. You'd need to drop the extern "C" part, while keeping the __declspec(dllexport).

modlibrary.c:6:
Code: [Select]
#define EXPORT extern "C" __declspec(dllexport)

To maintain backwards compatibility, you need to preserve existing exports, so you'd probably want to preserve the original macro, and create a new modified macro. Though if you renamed the macro within the source files, it would not affect binary compatibility, so that is an option if you feel it would lead to a cleaner solution.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 14, 2017, 01:25:09 PM
This rabbit hole getting deep.

Hooman, I had to reread your comments about 3 times. Then it made sense though, so that is good. See code below. I think buffer is being used properly now.

I'd like to use C++17 filesystem. My reservation is that Microsoft hasn't taken it out of experimental yet, so there could be some trouble in the future if they change things... I wonder what is taking them so long to get it out of experimental.

I figured out why the attribute deprecated would not work earlier, then started working. The compiler will not allow me to place the attribute on a function if it contains a string before the return of the function. In this case "C", which is defined in the macro EXPORT. By removing "C" from the function declaration, it works. So, I haven't researched this yet, but is there a way to designate a function as being compatible with C code other than using the string C in the function's declaration? If there is a way, can we modify the macro EXPORT to use it without breaking prior compatibility. If not, perhaps there is a different way to use the deprecated attribute?

See https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2059 for error details.

I am not a fan of MACROs. They tend to hide problems that would otherwise be relevant when reading the current line of code.

Code: [Select]
__declspec(dllexport) std::string GetGameDirectory();

[ [ deprecated("GetGameDir was deprecated in op2ext ver2.0.0. Use GetGameDirectory instead.") ] ]
EXPORT void GetGameDir(char *buffer); // DOES NOT WORK, THROWS ERROR C2059.

Code: [Select]
#include <filesystem>
namespace fs = std::experimental::filesystem;

__declspec(dllexport) std::string GetGameDirectory()
{
char moduleFilename[MAX_PATH];
GetModuleFileName(NULL, moduleFilename, MAX_PATH);

// Adding "\\" to end of directory is required for backward compatibility.
return fs::path(moduleFilename).remove_filename().string() + "\\"; */
}

extern __declspec(dllexport) void GetGameDir(char* buffer)
{
std::string gameDirectory = GetGameDirectory();

// Unable to use the newer funciton strcpy_s since we do not know the size of buffer,
// causing a security concern.
#pragma warning( push )
#pragma warning( disable : 4996 ) // Disable warning "The compiler encountered a deprecated declaration."
strcpy(buffer, gameDirectory.c_str());
#pragma warning ( pop )
}
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 14, 2017, 10:46:13 PM
Function attributes tend to be very order specific. Try this way:

Code: C++ [Select]

#define EXPORT extern "C" __declspec(dllexport)

EXPORT [ [deprecated] ]  /* Line modified to prevent forum interpreting double [ */
void GetGameDir(char *buffer);


Also, for reference, since I was testing on Linux, there is a difference to the export syntax, which is one of the reasons why a macro is used. It makes it easier to adjust for compiler differences.
Code: C++ [Select]

#if defined(_MSC_VER)  /* Microsoft (MSVC) */
    #define EXPORT __declspec(dllexport)
#elif defined(__GNUC__)  /* GCC (g++) */
    #define EXPORT __attribute__((visibility("default")))
#else  /* Unknown - Replace macro name with nothing and hope for the best */
    #define EXPORT
    #pragma warning EXPORT not defined for unknown compiler
#endif




Your new implementation looks sensible. I generally hate having to disable compiler warnings, but it makes sense here. You are explicitly doing something unsafe to maintain backwards compatibility. There is no real way around it. By disabling the warnings, you're at least producing source code that can be search/find/frep-ed for the pragma when hunting for problems.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 15, 2017, 02:35:19 AM
Thanks Hooman,

I actually figured this out as well considering attributes and was getting ready to post when I saw your solution! So, now everything is working. What you explains makes some more sense on the MACRO.

I found some more information about vol file loading.

This line of code will cause the corrupted string to appear in the vol file failed to load dialog box.

Code: [Select]
	
std::string mapsString("maps.vol");
vols.AddItem(mapsString.c_str());

This line of code will allow the program to properly load the vol file.

Code: [Select]
char* mapsChar = "maps.vol"
vols.AddItem(mapsChar);

Additionally, this line will cause the code to fail just like the std::string.c_str() did.

Code: [Select]
char mapsChar[]{ "maps.vol" };
vols.AddItem(mapsChar);

Basically, we changed the variable pFileName below from char* to const char*. I'm assuming the p means pointer in Hungarian?

This looks egregious, but we probably bit off on it because the function AddItem(char *volToAdd). This looked to us as if it was taking a char* and not necessarily using it as a pointer?

Code: [Select]
// VOL entry
struct VolSearchEntry {
const char *pFileName;
int unknown1;
int flags;
int unknown2;
};

So, now I'm trying to figure out how to transfer a std::string to a pointer to a char[] where the pointer is not destroyed when the std::string leaves scope???

Below worked, but really isn't practical. I think it works because the vector will stay in scope until after the vol files are loaded since it exists outside the function. I might be able to skip the vector and use a std::string outside of function scope, but I didn't test this since it really isn't the approach we want.

Code: [Select]
std::vector<char> chars;
void LoadVolFiles(std::string directory)
{
    std::string mapsString("maps.vol");
    chars = std::vector<char>(mapsString.c_str(), mapsString.c_str() + mapsString.size() + 1u);
    vols.AddItem(&chars[0]);
}

Perhaps the answer is to modify the VolList class to take a std::string that it steals the pointer from. This way the string stays in scope until VolList is done with it. I'm not convinced this is the right solution though.

Thoughts?

Perhaps this is what we deserve for rushing and going overtime in the last session of the pair programing session. :)

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 15, 2017, 02:57:39 AM
Ahh, very good investigative work. Based on what you've written, it looks like an ownership issue, and use of the string buffer after it's been freed. The std::string holds the buffer, and passes it by pointer to the AddItem method, which records the pointer so it can access the buffer later. But inside the loop that loads the vol file, that string variable gets reused.

I looked back at the original code to see why it wasn't a problem before. Before it was using hardcoded string constants for the main vols. Those strings will be stored in the read-only data section of the exe, so they'll never be freed or invalid. For the addons, it did re-use the string buffer to load multiple vol files, but I think this code was probably never tested with more than one addon vol. In other words, I think the bug was essentially pre-existing, just never discovered.

I think your last example works for the same reason. It's only trying to load one vol file.

Your solution is correct to use std::string in place of a raw char*. That will allow ownership to pass into the struct, so we don't have to worry about using buffers after they've been freed. It also nicely modernizes the API a bit. The alternative would be to allocate space and strcpy or similar. Not such a great alternative, and doesn't nothing to modernize the code. I'd say go ahead and update the struct to use std::string.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 15, 2017, 12:48:51 PM
Err, hold on. I just remembered that struct is declared to match the memory layout used in Outpost2.exe. You can't just change the char* to a std::string, or it will break things. I think you'll need to do some manual memory management on this one.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 15, 2017, 08:13:57 PM
Hooman,

Thanks for the tips. I'm not following 100% of what you are saying yet.

I refactored VolList significantly, improving its encapsulation and creating some new private functions to try to reduce the number of things each function is responsible for.

My plan is to provide VolList with a std::string volPath. Then volList will deal with whatever manual memory management is necessary.

This is where I'm having trouble. I'm storing each string in a vector so it will not go out of scope. Then I'm providing VolSearchEntry with a pointer to the first element of the std::string (I think by passing &volPath[0]). However, the code is not loading the volFile and is just showing jibberish in the error window.

So, I'm doing something wrong hear, but I'm not sure what that is. It is probably related to what you are saying about the buffer and manual memory management, but not sure?

I'm uploading the code on the spare branch on GitHub so you can look at it and give me a diagnosis. A couple of other minor improvements I'd like to consider are:

 * Improving the documentation in the function LoadVolFiles. (I don't understand it well enough to figure it out on my own.)
 * Changing #ifndef VolList_H to just #pragma once. Is there any reason not to use #pragma once? It seems simpler to me to use #pragme once.
 * For making GetGameDirectory available externally to the dll for C++ linkage, is there any reason to decorate it with the extern keyword?IE extern __declspec(dllexport)?

Thanks.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 16, 2017, 10:18:31 PM
Ahh, I see what you're doing. You've added the vector as a member of the VolList class. That extends the lifetime of those strings to the lifetime of the VolList class, which being a global static variable, is the lifetime of the program. That will keep the strings in memory.

The problem then, is the code isn't using the copy of the string from the volPaths vector. It's using the volPath parameter, which has limited lifetime.

Code: [Select]
	volPaths.push_back(volPath);

InitializeVolSearchEntry(&volPath[0]);

You need to update that second line to read from the vector. The entries in the volPaths vector will be copy initialized from the volPath parameter (by the first line), so they'll have independent lifetimes. Try using something like volPaths[lastEntry].c_str().



Go ahead and use #pragma once. It's much cleaner looking. The main objection to it was the syntax is not standard, however, it's use is now so widespread that you don't really need to worry about it. Previously, back in my day *leans on cane and whistles through teeth* it wirrrked on Wiiiirndows with MSssssVCeeeerr, but not on Linux with GCC, hence it used to cause headaches with cross platform compilation. It's now supported by GCC though, and most other major compilers:
https://en.wikipedia.org/wiki/Pragma_once (https://en.wikipedia.org/wiki/Pragma_once)



You can drop the extern part if you're not using the "C" part. Using extern on it's own is still useful forward declares (without including header files), but that's not being used in this project.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 17, 2017, 12:35:56 AM
Hooman,

Looks like I was working on the code while you were replying. :)

The custom filename vol loading is now working! I tried some variations of your recommendation, but was unable to get the code working by storing the strings. However, I did get it to work when storing a vector<vector<char>>. But that is a bit uglier than just storing a vector<string>. Please check out the new pushed code.

We have accomplished our initial task!

Quote
Ahh, I see what you're doing. You've added the vector as a member of the VolList class. That extends the lifetime of those strings to the lifetime of the VolList class, which being a global static variable, is the lifetime of the program. That will keep the strings in memory.

Well, my plan was to keep them in memory until the game finished loading the vol files and then delete the instance of VolList. I hadn't got this far yet though. So basically make VolList a member variable of the function InitializeOP2Ext. Is that a bad idea for some reason? If it is a bad idea, I'd like to document that VolList must exist for the full life of the executable to work properly.

Commit Message

* Fix VolList to load all vol files from OP2's root directoy and addon directory

Loading vol files is no longer dependent on specific filenames. The Game Directory is now found inside of LocateVolFiles instead of being passed into the function. Since Outpost2 requires vol filenames be passed in relative form, there is no reason to pass the absolute path into the function.

volFilenames are being stored within a vector<vector<char>> now. Storing the filenames as a vector<string> would cause the vol filename to be corrupted after being passed to Outpost 2 for loading vol files. Unsure why storing them as a vector<char> works instead of a string. This should be reviewed by someone else before next release of op2ext.

Additional changes:

 * Renamed DoError to PostErrorMessage. Renamed PostErrorMessage arguments to more meaningful names.
 * Replaced NULL with nullptr where appropriate in ModMgr.cpp.
 * Refactored initialization code inside DllMain into a subroutine named InitializeOP2Ext. This simplifies DllMain.
 * Created a function SetLoadOffset that removes some implementation details from InitializeOP2Ext.
 * Added brackets around 1 line if statements.
 * Removed old commented out code.


I went ahead and merged the DynamicVolLoading branch back into master since it is working.

Current tasks:

 * Continue replacing NULL with nullptr where appropriate
 * Replace #ifndef with #pragma once.
 * Continue adding blocks to single line if statements
 * Refactor order of includes to go from local to standard library

Also, I'd like to see the documentation improved on the function below, but don't feel qualified to do so. Any reason to keep the commented out memcopy line in the function?

Code: [Select]
void VolList::LoadVolFiles()
{
EndList();

VolSearchEntry *vol = &volSearchEntryList[0];
int *vol2 = &volSearchEntryList[0].unknown1;
int *vol3 = &volSearchEntryList[numberOfVolFiles].unknown1;
VolSearchEntry *vol4 = &volSearchEntryList[numberOfVolFiles - 1];

// Change operand of the MOV instruction
Op2MemSetDword((void*)0x00471070, vol);

// VolumeList+4 a bunch of other subs want. Better change those
//memcpy((void*)0x00471142, &vol2, 4);
Op2MemSetDword((void*)0x004711DA, vol2);
Op2MemSetDword((void*)0x00471206, vol2);
Op2MemSetDword((void*)0x0047136C, vol2);
Op2MemSetDword((void*)0x004713AA, vol2);
Op2MemSetDword((void*)0x004713D1, vol2);
Op2MemSetDword((void*)0x00471439, vol2);
Op2MemSetDword((void*)0x00471474, vol2);

// The 2nd element of the null entry some stuff wants
Op2MemSetDword((void*)0x0047126E, vol3);
Op2MemSetDword((void*)0x0047128B, vol3);
Op2MemSetDword((void*)0x00471389, vol3);
Op2MemSetDword((void*)0x004713E8, vol3);
Op2MemSetDword((void*)0x004713EF, vol3);
Op2MemSetDword((void*)0x00471408, vol3);
Op2MemSetDword((void*)0x00471457, vol3);

Op2MemSetDword((void*)0x0047111F, vol4);
}

Thanks,
Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 17, 2017, 02:14:27 AM
Quote
Well, my plan was to keep them in memory until the game finished loading the vol files and then delete the instance of VolList. I hadn't got this far yet though. So basically make VolList a member variable of the function InitializeOP2Ext. Is that a bad idea for some reason? If it is a bad idea, I'd like to document that VolList must exist for the full life of the executable to work properly.

Unknown. The strings are probably only needed in memory until the game tries to open and parse the VOL files. It wouldn't be wrong to keep the string around for longer though, and it might even be expected. I would expect those strings to stay around as long as the game was running and had the VOL files open.



I think these lines will do what you want:
Code: [Select]
	volPaths.push_back(volPath);

InitializeVolSearchEntry(volPaths.back().c_str());

Assuming volPaths is declared as std::vector<std::string>, and appropriate const modifiers are added where the compiler complains. That includes the parameter to the InitializeVolSearchEntry method, and on pFileName in the VolSearchEntry struct.



Quote
Also, I'd like to see the documentation improved on the function below, but don't feel qualified to do so. Any reason to keep the commented out memcopy line in the function?
The commented out line points within the ResManager::Shutdown() method. I don't know why it was commented out. It's been like that since the initial commit. It looks like it should be active. Perhaps there was a problem with crashing during shutdown because of this patch, and not cleaning up properly avoids the crash? When shutting down, the OS will clean up anyway, so you can get away with it.

Try activating it and see what happens.  :)
(Probably best to use the same format as the other lines.)
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 17, 2017, 08:35:38 PM
Today's work was mostly refactoring. Below are the half dozen or so commits made.

* Exchange all NULL defines with nullptr
* Use back() function instead of size() - 1 on vector in VolList
* Replace manual definitions with #pragma once in header files
* Make PostErrorMessage arguments std::string and improve message format
* Hook MaxVolFileCountReached into PostErrorMessage function
* Add path of vol file to error message when too many vol files loaded
* Encapsulate VolList class variables VolSearchBufferSize and buffer as private members
* Add .ini files to ignore list
* Made VolSearchBufferSize an unsigned int since it cannot be a negative number

Hooman,

I wasn't able to get the vector<string> to work in VolList. See code below for what I tried. I think it still needs a pointer and not just a c_str.

Code: [Select]
std::vector<std::string> strings;
void VolList::AddVolFile(std::string volPath)
{
if (MaxVolFileCountReached()) {
return;
}

strings.push_back(volPath);
InitializeVolSearchEntry(strings.back().c_str());

... MORE CODE ...
}

struct VolSearchEntry {
const char* pFilename;
int unknown1;
int flags;
int unknown2;
};



How are we wanting to comment on functions? I'm learning the Microsoft's intellisense only picks it up if the comment is above the function's definition. I was trying on the declaraction in the header file since that made sense to me, but it wouldn't work.

We can use the Microsoft specific way like this:

Code: [Select]
/// <summary>
/// Prepares all vol files found within the supplied relative directory from the Outpost 2 executable
/// for inclusion in Outpost 2. Does not recursively search subdirectories.
/// </summary>
/// <param name="relativeDirectory">A directory relative to the Outpost 2 exectuable. Default value is an empty
/// string</param>
/// <returns></returns>

Or something more like this (I think this is sort of Doxygen, but not really sure).

Code: [Select]
/**
Prepares all vol files found within the supplied relative directory from the Outpost 2 executable
for inclusion in Outpost 2. Does not recursively search subdirectories.

@param relativeDirectory A directory relative to the Outpost 2 exectuable. Default value is an empty string.
*/

Either is fine with me. Microsoft's way integrates a little nicer with Intellisence, but since we are interested in cross platform code in general, maybe better to not go with it. Not sure what Linux compilers do with the syntax?



Any idea what the following function does? It is not called from anywhere else in op2ext. I tried hardcoding a value in and compiling and running Outpost 2. The version string in the credits section and the version string in the preferences menu during gameplay remained unchanged...

Code: [Select]
char *verStrAddr = (char*)0x004E973C;
EXPORT void SetSerialNumber(char num1, char num2, char num3)
{
if (modStarting || num1 < 0 || num1 > 9 || num2 < 0 || num2 > 9 || num3 < 0 || num3 > 9) {
PostErrorMessage("op2ext.cpp", __LINE__, "SetSerialNumber failed. Invalid mod serial number or was called after game startup.");
}
else {
char buffer[8];
_snprintf_s(buffer, sizeof(buffer), "%i.%i.%i.%i", 0, num1, num2, num3);
Op2MemCopy(verStrAddr, buffer, sizeof(buffer));
}
}



Current tasks:

 * Continue adding blocks to single line if statements
 * Refactor order of includes to go from local to standard library
 * Fix warning 4200 in VolLists

We are getting close to finishing the originally tasked workload with op2ext. If people have other suggested additions or changes, not would be a good time to add them since everything is fresh. I'm happy to help with implementation. Except for the part where one has to dive into Outpost 2 to figure out where to hook things in.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 19, 2017, 12:13:54 AM
I put a lot of time into the project Today. Made a lot of progress, especially in the Mod Manager class.

Unfortunately, NetHepler is current throwing an exception every time Outpost 2 exits while I'm in debug mode. It doesn't happen when I run Outpost 2 normally without debugging using the newly compiled op2ext.dll. So I think it is maybe related to the working directory. I haven't investigated further than recording the exception details below (see the last commit).

COMMITS
* Add Version Info Resource to project

* Add Version Info Resource files to project and fix array compiler error in VolList.cpp.

 * Current implementation of VolSearchEntry buffer[] was Microsoft specific. Fixed by making VolSearchBufferSize static so buffer could be set to the same size by the compiler on any instance of VolList.

 * Added resource files so the version information can be set inside Visual Studio 2017 and not manually amended later through Resource Hacker.

* Add *.aps files to ignore list

 * A binary file designed to reduce load time of resources. Should be produced locally on first compilation of source code.

* Rewrote ModMgr's function GetCurrentModDir() to use modern C++ features

 * Improved error messaging to user when improper command line arguments or switches provided.
 * Started prepping ModMgr.h/.cpp for refactor into a class. (Would like to encapsulate private functions).

I'm not entirely happy with the new code. It is lengthy and not easy to follow when reading through. I think it is an improvement over the last iteration because it removes the deeply nested token search statement. While there are more lines of code overall, they are separated into smaller functions. GetCurrentModDir itself is still too lengthy.

This function could probably be improved by using exceptions, but we are not using exceptions anywhere else. So I limited exception use to just where required when accessing functions in the std library that leverage exceptions.

I'd like to rename ModMgr.h/.cpp to ModManager.h/.cpp. I'm unsure if this would affect c linkage to the function? I don't believe it would though.

Function tested reasonably thoroughly through different failure conditions (bad path, no path, wrong spelled switch, etc) without finding bugs.

* Encapsulate ModMgr in a class.

Currently, the DEBUG section of the Outpost2.ini file is not being destroyed. I've noticed an empty Outpost2.ini file is created in the source code directory when debugging. The DEBUG section is added properly to the game directory on initialization. My theory is that the wrong directory is being used for destruction, but have not tested in code.

* Use absolute directory of Outpost.exe for calls to access/modify outpost2.ini

Previously we were using the current working directory to access outpost2.ini. I believe the working directory is being changed to the directory of the source code when attaching the debugger and attempting to debug. This is causing problems with applying the addons netfix and nethelper.

Originally, when using a fresh copy of outpost.ini from the repository, NetFix would load on the first attempt. On subsequent debug sessions, NetHelper would not load. Also, a file named outpost2.ini with nothing inside would be created in the source code directory. Hooman witnessed this with me on our last pair programming session, but we got it to work with some fiddling, so we moved on.

Now, whenever op2ext's dllmain function is called, the working directory is manually set to the location of the Outpost 2 executable. This fixed all issues with NetFix. I also set all calls inside op2ext attempting to access/modify outpost2.ini to the proper absolute directory instead of relying on relative directories.

I think it would be best not to rely on the working directory for any calls.

Unfortunately, NetHelper.dll is currently throwing an exception after exiting out of Outpost 2. This occurs before DllMain is called in op2ext.h. My hunch is this may be related to NetHelper attempting to use the working directory set to the wrong location, but I haven't examined anything to verify that.

=== Exception Details ===

Exception thrown at 0x25005E9C (NetHelper.dll) in Outpost2.exe: 0xC0000005: Access violation reading location 0x000000B8.

DISASSEMBLY INFO
25005E92  call        25005A0E
25005E97  mov         ebx,eax
25005E99  mov         eax,dword ptr [edi+10h]
25005E9C  mov         esi,dword ptr [eax+ebx*8]
25005E9F  push        ebx
25005EA0  lea         eax,[ebp-8]
25005EA3  push        eax
25005EA4  mov         ecx,edi
25005EA6  mov         dword ptr [ebp-4],esi
25005EA9  call        25005D48

CALL STACK
>   NetHelper.dll!25005e9c()   Unknown   No symbols loaded.
    [Frames below may be incorrect and/or missing, no symbols loaded for NetHelper.dll]      Annotated Frame
    NetHelper.dll!250066a9()   Unknown   No symbols loaded.
    NetHelper.dll!250068d4()   Unknown   No symbols loaded.
    NetHelper.dll!25006a2c()   Unknown   No symbols loaded.
    NetHelper.dll!25006aae()   Unknown   No symbols loaded.
    NetHelper.dll!250069fa()   Unknown   No symbols loaded.
    NetHelper.dll!250050d4()   Unknown   No symbols loaded.
    NetHelper.dll!25005177()   Unknown   No symbols loaded.
    NetHelper.dll!25005220()   Unknown   No symbols loaded.
    NetHelper.dll!250053e8()   Unknown   No symbols loaded.
    NetHelper.dll!25005521()   Unknown   No symbols loaded.
    NetHelper.dll!2500767b()   Unknown   No symbols loaded.
    NetHelper.dll!25007797()   Unknown   No symbols loaded.
    NetHelper.dll!25007814()   Unknown   No symbols loaded.
    [External Code]      Annotated Frame
    Outpost2.exe!004c781f()   Unknown   No symbols loaded.
    Outpost2.exe!004c772e()   Unknown   No symbols loaded.
    Outpost2.exe!004c4261()   Unknown   No symbols loaded.
    [External Code]      Annotated Frame


Current tasks:

   * Figure out why NetHelper is throwing an exception on closing Outpost 2
   * Figure out what EXPORT void SetSerialNumber(char num1, char num2, char num3) does (or is supposed to do?)
   * Continue adding blocks to single line if statements
   * Refactor order of includes to go from local to standard library
   * Decide how to handle comments on external functions.

-------------------------------------------------
EDIT:

I did some research on Visual Studio, and it does default to setting the working directory to the folder containing the project, not the executable when debugging a DLL with an application in a different directory. I learned how to tell the debugger to use a custom directory as the working directory when debugging. This solved the issues with NetFix, and allowed me to remove the code setting the working directory to the Outpost 2 executable. However, it didn't fix the error explained about in NetHelper.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 20, 2017, 03:00:35 PM
* Encapsulate ModMgr in a class.

Currently, the DEBUG section of the Outpost2.ini file is not being destroyed. I've noticed an empty Outpost2.ini file is created in the source code directory when debugging. The DEBUG section is added properly to the game directory on initialization. My theory is that the wrong directory is being used for destruction, but have not tested in code.
Uhh, you must not be using the latest version of the op2ext code. A while back I changed it so it doesn't use the DEBUG ini setting anymore, among other things like allowing ini mods to call cleanup functions on exit, and moved as much logic out of DllMain as possible. http://arklon.outpost2.net/other/op2ext_1-3-6.zip

Yeah, the op2ext code on GitHub looks out of date. I'd suggest busting out a 3-way code diff/merge tool. KDiff (http://kdiff3.sourceforge.net/) is a decent free one.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 20, 2017, 08:40:40 PM
Quote
Uhh, you must not be using the latest version of the op2ext code. A while back I changed it so it doesn't use the DEBUG ini setting anymore, among other things like allowing ini mods to call cleanup functions on exit, and moved as much logic out of DllMain as possible. http://arklon.outpost2.net/other/op2ext_1-3-6.zip

Yeah, the op2ext code on GitHub looks out of date. I'd suggest busting out a 3-way code diff/merge tool. KDiff is a decent free one.

It would have been nice if this newer op2ext code had been committed overtop of the op2ext project in the SVN repository or Hooman and I told about it before we cloned the subversion repository. I'm starting to understand how Leeor posted 1.3.6 with an outdated version of op2ext.dll. Oh well...

I quickly scanned the code Arklon posted, and from a cursory pass, the changes look as though they should be doable to merge in with the current work Hooman and I have done.

I've used KDiff before with Mercurial. Oddly, it didn't occur to me that KDiff could be used with any files and not just within TortoiseHg.

I may wait for another pair programming session before working the merge. Let me look at it a little closer and make sure I feel comfortable and understand the new code well enough before committing to doing it on my own.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 20, 2017, 09:16:21 PM
It would have been nice if this newer op2ext code had been committed overtop of the op2ext project in the SVN repository or Hooman and I told about it before we cloned the subversion repository. I'm starting to understand how Leeor posted 1.3.6 with an outdated version of op2ext.dll. Oh well...
I told Leeor about it when we were putting together 1.3.6, and I thought he committed it to the SVN, but I guess that never ended up happening? Supposedly it went in on Nov 15 2016.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 22, 2017, 04:39:40 AM
Wow. You've been busy.

I'm not seeing anything specifically wrong with the vector to std::string change. Does it give a compile error, or a runtime error? What exactly goes wrong?

I don't have a strong preference about the comments. I guess I like the Doxygen format a bit better, since they look more compact and easier to type. The cross platform nature is also tempting. Though I've never really used Doxygen, so in that sense, the benefits are somewhat irrelevant. What about Visual Studio plugins? Is there a way to get Visual Studio to recognize the Doxygen format like it's native comment format?

The commenting is really more of a tooling concern than a strict compiler concern. That may give the Microsoft format increased weight here.

The SetSerialNumber function changes the version string ("1.3.0.4") in the .data section of Outpost2.exe. I just checked the address it was using. I believe there were a few different version strings used in different places, so I'm not sure what exactly it affects.



I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 22, 2017, 12:49:22 PM
I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.
I agree, but I when I was talking about him supposedly committing it to the SVN, I was talking about a year ago before we had the ball rolling on GitHub.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 22, 2017, 01:38:24 PM
Quote
I agree, but I when I was talking about him supposedly committing it to the SVN, I was talking about a year ago before we had the ball rolling on GitHub.

All the more reason why I'm expecting things will improve in the future.  :)
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 22, 2017, 06:35:45 PM
Quote
Quote from: Hooman on Today at 04:39:40 AM
I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.

I agree, but I when I was talking about him supposedly committing it to the SVN, I was talking about a year ago before we had the ball rolling on GitHub.

After being fairly grumpy about having to merge the 2 different versions, it actually wasn't that difficult using KDiff and relying on all the time I've already put into op2ext. I learned a fair amount about how to use KDiff for comparing changes between any file, not just in the strict confines of TortoiseHG, so it was actually pretty useful to do. (However I have a compile issue below, so I'm hoping it is easy to solve and I don't eat crow on this paragraph...)

Inside of op2ext.cpp, we have the following class declaration:

Code: [Select]
class __declspec(dllimport) TApp
{
public:
int Init();
void ShutDown();
};

However, the member functions Init and Shutdown do not appear to be defined within op2ext code base (unless I missed it). I'm getting LNK2019 Linker Tools Error for both functions (https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2019) on compilation.

Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: int __thiscall TApp::Init(void)" (__imp_?Init@TApp@@QAEHXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
   
Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: void __thiscall TApp::ShutDown(void)" (__imp_?ShutDown@TApp@@QAEXXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)

I'm not sure if maybe a reference is missing in the GitHub copy of op2ext or what? If we can solve this, I can finish the compilation and start testing the merge.

Current code is commited to the REPO as the branch Mod_initialization_and_cleanup_function_hooks. https://github.com/OutpostUniverse/op2ext/tree/Mod_initialization_and_cleanup_function_hooks. (I couldn't get it to cooperate with spaces for some reason so used underscores).



Hooman, regarding documentation. Check out the two attached screenshots. Basically Visual Studio creates some metadata (in XML form I think) that allows specific comments to only show up for each argument into a function as you are typing it. If we go with something else like Doxygen, then comments on arguments and return values will just be included in the body of the intellisense tooltip. So, you are not missing any data not using the Microsoft way.

In fact I like having the argument comments in the body of the tooltip instead of separated out, that way I don't have to start typing out the function and arguments just to read the comments associated with the arguments, so wouldn't mind skipping the Microsoft specific comments.

I'm not looking to document all the functions this formally, just if it isn't obvious what is going on, or the public functions available to other DLLS.



Quote
The SetSerialNumber function changes the version string ("1.3.0.4") in the .data section of Outpost2.exe. I just checked the address it was using. I believe there were a few different version strings used in different places, so I'm not sure what exactly it affects.

Is SetSerialNumber where it checks to see if versions of the game are compatible before initiating a multiplayer game?

I'd like to get all the different version strings standardized for sanity purposes. SetSerialNumber doesn't affect the serial number that I'm trying to clean out of the settings area of in game play, so that must exist outside of op2ext.

Thanks,
-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 22, 2017, 09:58:31 PM
Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: int __thiscall TApp::Init(void)" (__imp_?Init@TApp@@QAEHXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
   
Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: void __thiscall TApp::ShutDown(void)" (__imp_?ShutDown@TApp@@QAEXXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
It needs to link against the OP2 SDK .lib file.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 23, 2017, 01:42:41 AM
Indeed. TApp is one of the exported classes from Outpost2.exe. Hence why op2ext is declaring it with dllimport. You need a reference to Outpost2.exe in the import table of op2ext.dll to use it. You can include the same .lib file the level DLLs use to create that reference.

I'd never heard "eat crow" before. I had to look that up.  ::)

Sounds like you might also have a preference for Doxygen format? If so, go ahead and use it. You'll be taking the lead on this decision.  :)

I don't remember enough detail on the version/serial numbers to answer your question.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 23, 2017, 03:05:03 AM
Arklon, Hooman,

Thanks!  I linked to Outpost2DLL.lib, set Safe Exception Handling to FALSE, and everything worked great. (Outpost2DLL.lib rquires Safe Exception Handling to be false, otherwise I would leave it enabled). Initial test loaded NetFix properly and NetHelper didnít throw an exception on application exit. Iíll hold off pushing this commit into the GitHub repo until we decide how we want to include the reference to Outpost2DLL on GitHub.

When I saw __declspec in front of the definition for TApp, I just assumed it was __declspec(dllexport). Dumb on my part. Glad it was an easy fix though.

We need to decide how op2ext will link to Outpost2DLL.lib in the GitHub repository. Outpost2DLL in the SVN repository is linked to by all the custom scenarios in the SVN repo. It would be nice if it was placed on GitHub so when placing scenarios on GitHub in the future, one could add Outpost2DLL project as a submodule to the scenario.

In this case, we could add Outpost2DLL as a submodule of op2ext, which I think would be the cleanest approach.

Or we could just add a copy of Outpost2DLL.lib (380kb) to the project, annotate its version number in the ReadMe and where to check for a newer release in case Outpost2DLL was ever messed with.

If we donít want Outpost2DLL on GitHub for some reason, we will just have to state clearly in the ReadMe where to fetch it.



Op2ext.h included in all op2ext subcomponents

From an overall design perspective, op2ext is a bit flawed in that all the subcomponents (like the mod manager, ipdropdown, etc) all include op2ext.h. op2ext.h then includes all the submodules. It would be nicer if none of the subcomponents referenced op2ext.h. To do that, we would need to create a new class and a new .h/.cpp file.

    * A class that handles op2 memory management. Maybe OP2Memory. Then we could rename Op2MemSetDword to MemSetDword and Op2MemSet to just MemSet. Then when calling these functions, you would write OP2Memory.MemSetDword(), and so on. I think this would look nice, group the memory setting functions and variables nicely in their own place, and hide some details of the process from the rest of the application. OP2Memory could be a static class or maybe an instance that is passed into constructors of the subcomponents. I took an hour stab at making this work myself, but it ended up majorly breaking everything so yeah, this is definitely an area Iím weak in.

    * A subfile that contains the random system level declarations. It would include the functions PostErrorMessage(), GetGameDirectory(), and the typedefs DBG and EXPORT. I thought maybe SystemHelper.h/.cpp. Name doesnít do a lot for me though. This might be better as just a loose list of functions and macros instead of an actual class.

If we implement both of those changes, we could eliminate includes to op2.h from all the subcomponents. Plus, I think it would be easier to understand the purpose of the code contained in those filenames than when including op2ext.h in the subcomponents.

ThoughtsÖ Great Idea? Bad Idea? Could be good with some changes?

I also want to talk about public.h inside op2ext.h, but two topic at a time I suppose.

Thanks,
-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 23, 2017, 05:42:02 AM
Quote
When I saw __declspec in front of the definition for TApp, I just assumed it was __declspec(dllexport).
I figured as much  ;)

Quote
We need to decide how op2ext will link to Outpost2DLL.lib in the GitHub repository. Outpost2DLL in the SVN repository is linked to by all the custom scenarios in the SVN repo. It would be nice if it was placed on GitHub so when placing scenarios on GitHub in the future, one could add Outpost2DLL project as a submodule to the scenario.

Yes, I was thinking now is about the right time to look into moving that code over.

Quote
In this case, we could add Outpost2DLL as a submodule of op2ext, which I think would be the cleanest approach.
That's an excellent idea. That would be the cleanest way to allow for proper linking.

As an aside, and an unrelated next step, we should also combine the Outpost2DLL and Outpost2App projects into a single project, with perhaps different subfolders for the two sections. Both projects rely on the same .lib file to link to Outpost2.exe. Also nice to note, the version locking with Git submodules would isolate dependent projects from any breaking structural changes during such a merge.



Quote
From an overall design perspective, op2ext is a bit flawed in that all the subcomponents (like the mod manager, ipdropdown, etc) all include op2ext.h. op2ext.h then includes all the submodules.
That does seem like a flawed design. We should try to improve the structure there.



I'm getting a sense of feature creep with the op2ext work here. It's getting hard for me to follow all of what's going on. It feels like changes are started, they hit a blocking problem (or just a suboptimal design), and then a new set of changes are started. Maybe we need to pause for a moment and think about the overall design, what changes are necessarily dependent, versus what can be separated out into multiple independent stages. Right now, I get the sense a bunch of unrelated things are all being changed at once. I kind of want to avoid a stream of unrelated partial changes that are all intermixed. That makes it hard to understand the evolution of the project, which can make future maintenance harder.

At any rate, don't stop what you're doing. I'm just saying take a moment to consider where things are going.

Also, this might be a really good point to learn about rewriting Git history. ;)


I'd also like to point out that currently, op2ext is kind of a mix bag of patches. This can make it tempting to use op2ext as a bit of a dumping ground for many different things. Maybe the project needs restructuring. Maybe we should consider moving some of those patches out into extensions loaded by op2ext. That might give the project more focus.



Meanwhile, I should look into getting the SDK up on GitHub.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 24, 2017, 02:32:10 AM
Hooman, I agree this thread is starting to blow up into a lot more than originally intended. (Especially you bringing up combining Outpost2DLL and Outpost2App :-) )

Iíd like to start winding down the actual work on op2ext soon. Here is sort of a list remaining.

CURRENT PLAN (subject to change of courseÖ)



I went ahead and merged the branch Mod_initialization_and_cleanup_function_hooks with the master branch. It contains Outpost2DLL.lib. Once Hooman gets Outpost2DLL transferred to GitHub, we can go back and add a proper submodule and delete the Outpost2DLL.lib file from op2ext.

SetSerialNumber Function

I played with the function SetSerialNumber. When joining a multiplayer match, if the host and joining player do not have the same serial number set, a message is given to both parties saying what each serial number is and that they are incompatible, kicking the player from joining the session. This is actually really great feature. See attached screenshots.

SetSerialNumber had a bug in it where the wrong digits were being set. Fix has already been pushed.

Iím getting the impression this is not being set as new versions are being rolled out. The current version in the repo reads 1.3.4. Iíd like to set the Serial Number to 1.3.7 since we are pushing a new version of the multitek tech tree with this release. Iíll post about it in the overall 1.3.7 I made to discuss https://forum.outpost2.net/index.php/topic,6043.0.html since it isnít related to vol files..



op2ext public.h

How exactly is public.h used or supposed to be used? I noticed it isnít actually a member of the op2ext project. Also, it doesnít look like it is #include in either NetHelper or NetFix.

Code: [Select]
 
// OP2Ext.dll preliminary interface. May be subject to change!

#define OP2EXT_API extern "C" __declspec(dllimport)

// Returns a pointer to a buffer of [MAX_PATH+1] length to retrieve the parameter to
// /loadmod that the game was invoked with.
// You should free() the buffer when you are done with it.
OP2EXT_API char* GetCurrentModDir();

// Pass a buffer of [MAX_PATH+1] length to retrieve the game directory
// without trailing slash.
OP2EXT_API void GetGameDir(char *buffer);

// Pass null terminated string to add a VOL file to the search list.
OP2EXT_API void AddVolToList(char *volName);

// Pass 3 digits, 0-9 each, to set a unique ID for this specific version of the mod.
// The OP2 version string will be rewritten to prevent other versions or other mods
// from joining the game.
// These should be numeric values and NOT the ASCII characters '0', '1', so on!
OP2EXT_API void SetSerialNumber(char major, char minor, char revision);

Quote
Also, this might be a really good point to learn about rewriting Git history.

Rewriting history would likely stall this project for good, so no thank you. :)
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on November 25, 2017, 12:37:23 AM
SetSerialNumber Function

I played with the function SetSerialNumber. When joining a multiplayer match, if the host and joining player do not have the same serial number set, a message is given to both parties saying what each serial number is and that they are incompatible, kicking the player from joining the session. This is actually really great feature. See attached screenshots.

SetSerialNumber had a bug in it where the wrong digits were being set. Fix has already been pushed.

Iím getting the impression this is not being set as new versions are being rolled out. The current version in the repo reads 1.3.4. Iíd like to set the Serial Number to 1.3.7 since we are pushing a new version of the multitek tech tree with this release. Iíll post about it in the overall 1.3.7 I made to discuss https://forum.outpost2.net/index.php/topic,6043.0.html since it isnít related to vol files..
Back in 1.3.4, if a mod was loaded, the game version (for MP purposes) was set to 0.0.1, and then client-side mods (e.g. Color Mod) were required to set the game version to 1.3.4 as a way of making them MP-compatible, except that was horrible for all sorts of reasons including the op2ext API function being broken - which, by the way, led to a hacky workaround being put in place in Color Mod instead, which makes things even worse because we can't even properly short circuit this old deprecated design requirement so no action needs to be taken to maintain compatiblity. I could just rewrite the Color Mods which would be pretty trivial even though the source code is lost, but good luck convincing me to do that when I could just be lazy instead.

I think Leeor did indeed miss this instance of the game version in the exe, he changed all other references. We really needed to bump it all the way back in 1.3.5 (I really don't even know why we deliberately decided to leave it at 1.3.4) because the 2x unit limit patch obviously breaks compatibility with 1.3.4 clients, but eh.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 25, 2017, 03:26:44 AM
Quote
but good luck convincing me to do that when I could just be lazy instead.
Hah! At least you're honest about it. Good to know yourself, I suppose.


Looks like public.h is supposed to be the import header for projects that use op2ext. Importing a minimal header is faster and leads to less namespace pollution than importing the full header. It's not much use though if it's not used.

Maybe public.h would make more sense after reorganizing the project, and getting rid of circular includes.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 25, 2017, 09:15:45 PM

Today I deleted the references to OP2MapImager, OP2Archive, op2ext, and OP2Utility in the SVN repository. Since all of these projects are now fully functional on GitHub, I didn't want people grabbing the old source code. (Of course the history still exists in SVN if you really want to pull them.)

I also threw a ReadMe in the root directory of the Outpost 2 Repository explaining where all the old source code was going.

As a followon project, I went into the Outpost Universe Wiki and updated the repositories section to include info on both repositories. There are four pages total. An overview, a SVN page, a Git page, and a SVNtoGit repo transfer page. The Git page and SVNtoGit page need more work. The rest looks pretty reasonable. I don't know if Hooman or leeor_net would be interested in expanding the SVNtoGit stuff, but it is tough for me to do without having actually performed the commands myself. https://wiki.outpost2.net/doku.php?id=opu:repositories:repositories



Quote
I could just rewrite the Color Mods which would be pretty trivial even though the source code is lost, but good luck convincing me to do that when I could just be lazy instead.

I think Leeor did indeed miss this instance of the game version in the exe, he changed all other references. We really needed to bump it all the way back in 1.3.5 (I really don't even know why we deliberately decided to leave it at 1.3.4) because the 2x unit limit patch obviously breaks compatibility with 1.3.4 clients, but eh.

Well, if you find the time to rewrite the colormod, I would be pretty interested in checking it out. Too bad the source code is lost now.

I'd like to write up a DevReadMe somewhere in the GameDownload section of the SVN repository that goes over all the steps for preparing Outpost 2 for release. Things like updating the versioninfo resources, checking if the latest version of op2ext and latest version of all mods shipped with the main download, and all places where the version string needs to be updated in game. I could probably get a start on the document, but would need a little help with some of the specifics I'm not aware of yet.



Quote
Looks like public.h is supposed to be the import header for projects that use op2ext. Importing a minimal header is faster and leads to less namespace pollution than importing the full header. It's not much use though if it's not used.

Maybe public.h would make more sense after reorganizing the project, and getting rid of circular includes.

Okay, so I think you are saying that we should not have exported functions like GetGameDir declared in two places. Instead we can reference the public.h file within the rest of op2ext when needing these functions. I could create a public.cpp file that contained their definitions. Would it make sense to name this op2extPublic.h/.cpp?

 
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 26, 2017, 02:30:00 PM
Quote
I don't know if Hooman or leeor_net would be interested in expanding the SVNtoGit stuff, but it is tough for me to do without having actually performed the commands myself. https://wiki.outpost2.net/doku.php?id=opu:repositories:repositories
I took a quick peak. Not sure what else to say. I barely know what I'm doing with that stuff currently, so I can't expand on it much either.  :P

Quote
I'd like to write up a DevReadMe somewhere in the GameDownload section of the SVN repository that goes over all the steps for preparing Outpost 2 for release. Things like updating the versioninfo resources, checking if the latest version of op2ext and latest version of all mods shipped with the main download, and all places where the version string needs to be updated in game. I could probably get a start on the document, but would need a little help with some of the specifics I'm not aware of yet.
This would be good to have. It's been so long since that last release, I think people have forgotten how to do this. Version control diffs maybe?  ;)

I remember I once had a text document that detailed a lot of the patches. I think it's been lost though. I went looking for it once and couldn't find it. :( If anyone happens upon it in a lost corner of the forums, or odd folder in the repo, please let me know.

Quote
Okay, so I think you are saying that we should not have exported functions like GetGameDir declared in two places. Instead we can reference the public.h file within the rest of op2ext when needing these functions. I could create a public.cpp file that contained their definitions. Would it make sense to name this op2extPublic.h/.cpp?
It probably makes the most sense to name the public export header after the project itself: op2ext.h. When people try to use the library, that's what they'll try importing. An internal master header file should probably be named something else. Though if the headers are split and organized nicely, there might not be a master header file.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 27, 2017, 02:23:03 PM
Quote
It probably makes the most sense to name the public export header after the project itself: op2ext.h. When people try to use the library, that's what they'll try importing. An internal master header file should probably be named something else. Though if the headers are split and organized nicely, there might not be a master header file.

I've moved enough code out of op2ext.h/.cpp, that something like this is now possible, but there are some big hurdles still.

Minus the includes, op2ext.h now reads as follows
Code: [Select]
extern VolList vols;
extern IniModuleLoader iniModuleLoader;

// NLS for OP2
//void LocalizeStrings();
void ConvLangStr(char *instr, char *outstr);

void LocateVolFiles(std::string relativeDirectory = "");

EXPORT void AddVolToList(char *volFilename);
EXPORT void SetSerialNumber(char num1, char num2, char num3);

// Prototype for patch to LoadLibrary, where it loads OP2Shell.dll
HINSTANCE __stdcall LoadLibraryNew(LPCTSTR lpLibFileName);

What is the line below for? It related with localization, but I don't know what NLS is and the comment is pretty vague. I'd like to at least improve the comment so it is more clear.

Code: [Select]
// NLS for OP2
//void LocalizeStrings();
void ConvLangStr(char *instr, char *outstr);

I'm having trouble understanding what is going on with LoadLibraryNew:

Code: [Select]
// Prototype for patch to LoadLibrary, where it loads OP2Shell.dll
HINSTANCE __stdcall LoadLibraryNew(LPCTSTR lpLibFileName);

It says it is a prototype, but we are actually using it. I think we just need to remove the word prototype and maybe expand the comment a little more to say what is going on?



Moving public.h to op2ext.h...

I'm thinking that we create a new file called dllMain.cpp or main.cpp that handles actually running the op2ext dll. Then we have op2ext.h/.cpp for use as the public export header.

I can move the declarations of IniModuleLoader, LocateVolFiles, and LoadLibraryNew into dllMain.cpp since no other component cares about them.

The EXPORT function AddVolToList(char *volFilename) requires access to the VolList class. I think we can extern declare VolLists so the body of the function AddVolToList can access VolLists. Not sure, would need to test...

GetCurrentModDir is a loose function in ModMgr.cpp. However, it is not required for use of ModMgr. It is just exists to allow others access to the Module directory. So, we could probably move the definition to op2ext.cpp and try another extern declare for ModMgr. This function is also possibly poorly written. It instantiates a new version of ModMgr every time it is called, which is wasteful at best. Perhaps with the extern call, this constant instantiation could be removed. (fixed now)

GetGameDir now lives in FileSystemHelper. This is our problem function for backwards compatibility. It actually calls FileSystemHelper's function GetGameDirectory and then copies the resulting std::string to the char* buffer that has no size passed alongside it. Again, I could look at maybe calling extern on GetGameDirectory, so the defintion of GetGameDir can live in op2ext.cpp and call it.

Umm, okay, I'm having trouble keeping up with what I'm talking about, so it is probably pretty confusing to read. Perhaps this is getting beyond my current ability/knowledge of C++? I can start experimenting and moving in this direction, but I didn't want to procede down the wrong rabbit hole.



Rewrite of IniMods

Additionally, I finished up a rewrite of IniMods. It is now named IniModuleLoader. (I'm thining about switching this to IniModuleManager since it also unloads the mods). It is now a class and hides all variables and functions as private except

 - void LoadModules();
 - bool UnloadModules();

I wrote a function that checks if the char[] returned from fetching data from the ini file is full. If it is full, it re-fetches the data with a larger char[] until it isn't full. I don't think any of the data sections in the .ini file will be greater than 1024 characters in the near future, but it is also nice not to worry about it.

Summary of commits:

* Minor cleanup of code in IpDropDown.cpp.

* Refactor IniMods.cpp into the class IniModuleLoader

 - Now supports std::string instead of char*
 - Reduced size of function LoadIniMods
 - Renamed variables in attempt to be more specific in their purpose

Code tested and it compiles and loads mods. Since this is an extensive rewrite, will need further testing for different error conditions like missing DLL, etc. Plan to review some more changes, test, then request a code review on my work since this is a critical piece of op2ext.

* Make GetPrivateProfileStdString dynamically resize buffer as required

 - Allows returning strings from Outpost2.ini that are larger than 1024 characters.

* Attempting to remove code from op2ext.cpp that is used throughout the application

 - Created a FileSystemHelper.h/.cpp and GlobalDefines.h/.cpp to facilitate.

Not necessarily happy with the name GlobalDefines though.


-Brett

-----------------------------
Edit:

I forgot to mention earlier but I've noticed that you can only include 1 mod using the command line (in addition to all the mods in the .ini file). Some references in the source code allude to loading multiple modules through the command line. Unless we specifically want to work towards supporting this, I'd like to edit the source to be more clear that only one mod is being loaded via command line.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 28, 2017, 03:38:47 PM
I don't know anything about the NLS or Localization stuff. BlackBox would be a better source of info on that.

For LoadLibraryNew, I believe "prototype" is being used in the sense of "function declaration" (as opposed to function definition). I'd have to look at the code to know what it's doing, though it's pretty common practice to hook LoadLibrary when patching stuff in memory and writing extension modules. It's the earliest and most reliable point at which you can hook and control a dynamically loaded library.

Quote
I forgot to mention earlier but I've noticed that you can only include 1 mod using the command line (in addition to all the mods in the .ini file). Some references in the source code allude to loading multiple modules through the command line. Unless we specifically want to work towards supporting this, I'd like to edit the source to be more clear that only one mod is being loaded via command line.
This strikes me more as a limitation of the user interface and parsing of the command line, rather than a backend issue. The backend might very well support loading multiple mods. In particular, the INI mod loader does this. You'd just need a way of specifying multiple modules at the command line. Maybe the option can take a list, or maybe the option can be specified multiple times.



Quote
Umm, okay, I'm having trouble keeping up with what I'm talking about, so it is probably pretty confusing to read. Perhaps this is getting beyond my current ability/knowledge of C++? I can start experimenting and moving in this direction, but I didn't want to procede down the wrong rabbit hole.
Yes, things are getting hard to follow. I think maybe a code review would help.

Quote
Code tested and it compiles and loads mods. Since this is an extensive rewrite, will need further testing for different error conditions like missing DLL, etc. Plan to review some more changes, test, then request a code review on my work since this is a critical piece of op2ext.
Indeed.

We may want to consider switching to a branching work model on this project, with pull requests for code review. Though that could be a painful transition when making extensive changes to so many areas of the project. A painful transition that will likely teach you about separating concerns and packaging commits nicely.  ;) 

I had a few concerns come to mind recently. While converting the OP2Helper project, I found I was able to run the source through the MinGW compiler, but it wouldn't link due to differences in name mangling. This got me thinking, there is a reason the interface for op2ext uses C rather than C++, the C ABI is highly portable, and allows modules to be written in a variety of languages using a variety of compilers. The C++ ABI is much less portable, and unlikely to interface well with other langues, and possibly not even with other C++ compilers.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on November 29, 2017, 03:43:56 PM
Quote
I don't know anything about the NLS or Localization stuff. BlackBox would be a better source of info on that.

For LoadLibraryNew, I believe "prototype" is being used in the sense of "function declaration" (as opposed to function definition). I'd have to look at the code to know what it's doing, though it's pretty common practice to hook LoadLibrary when patching stuff in memory and writing extension modules. It's the earliest and most reliable point at which you can hook and control a dynamically loaded library.

Thanks. It didnít occur to me that prototype means function declaration. All the localization references are commented out, so Iíll just document a bit more and leave it where it is in case it gets picked up on again later.

Quote
This strikes me more as a limitation of the user interface and parsing of the command line, rather than a backend issue. The backend might very well support loading multiple mods. In particular, the INI mod loader does this. You'd just need a way of specifying multiple modules at the command line. Maybe the option can take a list, or maybe the option can be specified multiple times.

Yes, this is what Iím getting at. It wouldnít be difficult to expand to accept multiple mods as a list. The real question is how many mods do we want to allow loading through the command line? There is a lot of potential for making a mess by combining them. Besides the obvious mod 1 and mod 2 both touch the same piece of the game, I was thinking about multiplayer sync.

What if someone rewrites the color mod and assigns it to 0.0.1 and then someone makes a multitek2 multiplayer scenario and assigns it 0.0.2. Both mods may be compatible, but which SerialNumber do they use when both are loaded? I think the right answer is that a third mod is created, say multitek2Color that takes 0.0.3. It can then deal with integrating them both as needed.

We can warn devs in the op2ext ReadMe of these problems. We can also limit to one mod to keep it less likely that conflicts arrive. Some mods may be pretty safe to combine if they affect different parts of the game and donít really create a multiplayer sync issue.

All that being said, Iím okay punting multiple command line mods until a different release even if we want it (unless it is a big deal to support for someone in the near future). I can go in and clean the code some more so it is clear that only one mod is getting loaded. From there it should not be too difficult to expand as needed. (I did rewrite the code to use std::string among other things, but the original code also only loaded one mod.)

Quote
I had a few concerns come to mind recently. While converting the OP2Helper project, I found I was able to run the source through the MinGW compiler, but it wouldn't link due to differences in name mangling. This got me thinking, there is a reason the interface for op2ext uses C rather than C++, the C ABI is highly portable, and allows modules to be written in a variety of languages using a variety of compilers. The C++ ABI is much less portable, and unlikely to interface well with other langues, and possibly not even with other C++ compilers.

Iíve actually been thinking about this as well. I noticed that FreeImageís public interface only uses the C ABI, probably for similar reasons as you brought up. Currently all of the EXPORT functions remain C functions, except for the new GetGameDirectory.  Iíd like to remove the dllexport from this function. Then we can create a new version of the function that uses the C ABI for export, taking into account input for the buffer size.



In regards to the EXPORT functions, if we eliminate public.h, and declare the EXPORT functions in op2ext.h instead, would this be considered a breaking change? If so are we okay with this?

The other reason Iím not a fan of public.h is because we are declaring the functions in 2 separate places. Iím not sure if there is precedence for this, but it seems like a possible source of some insidious bugs.

If we want to remove public.h, Iím happy to work this, if not, then Iím happy to leave it alone for this release.



Quote
We may want to consider switching to a branching work model on this project, with pull requests for code review. Though that could be a painful transition when making extensive changes to so many areas of the project. A painful transition that will likely teach you about separating concerns and packaging commits nicely.

I would probably fair better working with branches and waiting on code reviews than you are giving me credit for. Iím fine with code reviews and branches, but it would require someone who can reliably compile the code be available to review the code on a regular basis.

Perhaps if we rework the filesystem code to something compatible with minGW, you could fill this role? Is anyone interested? Being hobby time for everyone, this might be too much of a commitment regularly?

I'm looking to wind down work on op2ext, so besides working on eliminating public.h (if we want to do that) I'm looking at moving into more cleaning and testing than expanding functionality and ripping apart.

I would like to create a sample module simply designed to test most functionality of op2ext, like the EXPORT functions and the different initialization and cleanup hooks. Maybe this should be the next pair programming session? I should be able to squeak this in on the Sunday, 17DEC if someone is interested. I'll post in Pair Programming about it in the near future.



IpDropDown

I am having difficulty forming IpDropDown into a class. Something to do with the code below. I donít really understand what is happening as it looks like we are assigning functions to DWORDs (unsigned int) and then pushing them into Outpost 2ís executableís memory. Anyways, I can get the class to compile, but the program would blow up when running.

Code: [Select]

BOOL __stdcall EnableWindowNew(HWND hWnd, BOOL bEnable)
{
    . . . MORE CODE . . .
}

unsigned long __stdcall inet_addrNew(const char* cp)
{
    . . . MORE CODE . . .
}

// Data constants for InstallIpDropDown
DWORD newEnableWindowAddr = (DWORD)EnableWindowNew;
DWORD newInetAddr = (DWORD)inet_addrNew;
DWORD* populateComboBoxAddr = (DWORD*)0x004197C1;
DWORD* saveIpTextAddr = (DWORD*)0x004C0E36;
void* nopDataAddr = (void*)0x0041988F;

void InstallIpDropDown()
{
// patch the call to EnableWindow so we can add strings.
Op2MemSetDword(populateComboBoxAddr, (int)&newEnableWindowAddr);
Op2MemSetDword(saveIpTextAddr, (int)&newInetAddr);
Op2MemSet(nopDataAddr, 0x90, 14);
}

All that being said, perhaps it would be best to just leave IpDropDown alone. As Hooman mentioned earlier, it would probably make more sense to make it a separate module that op2ext loads into Outpost 2 as opposed to including it in op2ext. Maybe a project for another release?

So, Iím looking at dropping further work on IpDropDown off the list.



Hooman, is the Outpost2DLL code on GitHub stable yet? If so, Iíll work on setting up a submodule.

Iíll place the submodule in ./Outpost2DLL. Unless we want some sort of different folder structure like ./submodules/Outpost2DLL. Iím happy with either setup.



Current task list

 * Remove public.h and push declarations for EXPORT functions to op2ext.h (if this is what we want?)
 * Include in ReadMe.txt a section on recommended use of SetSerialNumber for mods involving multiplayer maps that will not interfere with incrementing the main gameís version number over time.
 * Include more details in ReadMe.txt on how to load modules using op2ext.
 * Include details in ReadMe.txt on available features in op2ext for developing and hooking modules into Outpost 2
 * Write a post build script that auto-zips the ReadMe.txt, dll, and PDB file for posting on GitHub.
 * Set Outpost2DLL project as a submodule once it exists on GitHub.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on November 29, 2017, 08:30:51 PM
Quote
All that being said, Iím okay punting multiple command line mods until a different release even if we want it (unless it is a big deal to support for someone in the near future). I can go in and clean the code some more so it is clear that only one mod is getting loaded.

Yeah, let's not worry about supporting multiple command line mods at this point. My concern here is more to avoid spreading comments throughout the codebase saying only 1 mod is supported if there is a chance that might change in the future. It's fine to document, but hopefully only in one place, and probably in relation to the command line parsing code.

Quote
Currently all of the EXPORT functions remain C functions, except for the new GetGameDirectory.  Iíd like to remove the dllexport from this function. Then we can create a new version of the function that uses the C ABI for export, taking into account input for the buffer size.

That sounds like a good plan.

Quote
In regards to the EXPORT functions, if we eliminate public.h, and declare the EXPORT functions in op2ext.h instead, would this be considered a breaking change? If so are we okay with this?

This would not break binary compatibility. Existing compiled code would continue to run. This could break source code compatibility, if they are requiring public.h and the file disappears. Though that is an easy fix, updated the #include line, so in practice not a big concern.

Quote
The other reason Iím not a fan of public.h is because we are declaring the functions in 2 separate places. Iím not sure if there is precedence for this, but it seems like a possible source of some insidious bugs.

Yes, we want to avoid this sort of repetition. A divergence of two copies of function signatures could lead to all sorts of hard to diagnose crashing bugs. The DRY (Don't Repeat Yourself) principle applies here.

Quote
I would probably fair better working with branches and waiting on code reviews than you are giving me credit for. Iím fine with code reviews and branches, but it would require someone who can reliably compile the code be available to review the code on a regular basis.

I'm speaking more from personal experience here. :P It's that waiting on code review part that can be killer. It's irritating to finish a patch that depends on a previous patch, only to find the previous patch still hasn't been accepted into master yet.

Quote
Perhaps if we rework the filesystem code to something compatible with minGW, you could fill this role?

Probably, yes.

Quote
I am having difficulty forming IpDropDown into a class. Something to do with the code below. I donít really understand what is happening as it looks like we are assigning functions to DWORDs (unsigned int) and then pushing them into Outpost 2ís executableís memory. Anyways, I can get the class to compile, but the program would blow up when running.

First, what is the benefit of making it a class?

I went through a stage where I wanted to make everything a class. Eventually I found things don't always fit that model so well. Like in Java, which I've long thought was silly for it's insistence on the use of classes, to the point you can't have free floating functions. Things like sin and cos had to be wrapped in a class. What does that add? They just end up being declared static. If you have a class full of static methods, you're effectively just using the class as a namespace.

Consequently, that would also be the solution to your problem. Declare the methods as static. Moving a method into a class changes the calling convention, and adds the hidden this parameter. The casts are probably silencing the real source of the error here, assuming you get the casts to work, since compilers tend to be very picky about casting of pointer-to-member-functions.

Quote
Hooman, is the Outpost2DLL code on GitHub stable yet? If so, Iíll work on setting up a submodule.
No, there are some things I still haven't worked out yet with my second conversion attempt.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 05, 2017, 06:10:06 PM
Quote
First, what is the benefit of making it a class?

It has several functions that should not be called by code external to ipDropDown.cpp

BOOL __stdcall EnableWindowNew(HWND hWnd, BOOL bEnable);
unsigned long __stdcall inet_addrNew(const char* cp);
void WriteAddressesToIniFile();

Okay, so this is only sort of true, because I think inet_addrNew and EnableWindowNew get injected into Outpost2's memory. I'm guessing they shouldn't be encapsulated. However, I inet_addrNew should be separated into several more private functions since it is almost 50 lines long and handles all the sorting when new IP addresses are noted.

Also, it has member variables that should only be updated by ipDropDown.cpp. In particular:

char ipStrings[10][47];
int numIpStrings = 0;

So, I'm not really sure the right way forward reconciling the fact that ipDropDown manipulates Outpost2's memory but also contains functions and variables that should be encapsulated. Again though, this can just be left alone for now too. It works and is fairly organized.

If we move the declaration of a function or a variable to the .cpp file, is this considered encapsulated? It might be a nice alternative. Since the rest of op2ext doesn't need to call WriteAddressesToIniFile, we could move its declaration to the .cpp file. Would this be a positive move?



Quote
This would not break binary compatibility. Existing compiled code would continue to run. This could break source code compatibility, if they are requiring public.h and the file disappears. Though that is an easy fix, updated the #include line, so in practice not a big concern.

In this case, I'd like to move the version to 2.0.0 on this release since it would be a breaking change for source code compatibility. I'd also like to leave some wiggle room in case someone ever wants to go back and assign version numbers to previous releases that I/we? don't really have enough documentation on right now.



I finished a rewrite of the ReadMe. Please let me know if there are any mistakes or if it is too detailed, not detailed enough, etc. I'd like to take a final pass reviewing it once everything else is completed anyways. It is in the repo, but below is a copy for ease of access.

Code: [Select]
Outpost 2 Extension Module
------------------------------------------

Outpost 2 extension modules (op2ext) is used to implement various extensions to the game Outpost 2. The most current version of Outpost 2 is maintained by the Outpost 2 community and may be found at https://www.outpost2.net/.

Authors: op2hacker (aka BlackBox)
         Hooman
         Arklon
         Brett208 (Vagabond)

Summary
------------------------------------------

Outpost 2 Extension is a dynamic link (DLL) library written in C++. Source code may be found at https://github.com/OutpostUniverse/op2ext. OP2Ext compiles against x86 Windows. The current solution/project files use Visual Studio.

The following extensions are implemented for Outpost 2:

 * Addon a single module using the command line argument /loadmod

    - Allows for modifying the behavior of Outpost 2 by placing modified files in a folder and calling '/loadmod directoryName' when executing Outpost 2. For example, Outpost2.exe /loadmod multitek2.
    - Designed for modules the user wants to include in Outpost 2 sometimes, but not on a semi-permanent basis.

 * Addon modules using ini file

    - Allows for adding modules to Outpost 2 without calling a command line switch.
    - Allows setting properties for modules in the Outpost2.ini file.
    - Designed for modules the user wants to include in Outpost 2 on a permanent or semi-permanent basis.

 * Extra VOL file loading

    - Allows loading any vol file contained in the root Outpost 2 directory and in the mod folder.
    - Outpost 2 may only load a maximum of 31 vol files, including vol files loaded by modules.

 * IP dropdown box for net play

    - Allows saving previously used IP address in a drop down menu for easy selection when joining repeat matches.


Writing Custom Modules
------------------------------------------
op2ext.dll is included in releases of Outpost 2 on Outpost Universe. This allows modules to be written and integrated into anyone's copy of Outpost 2. Custom modules must be designed to hook into op2ext.dll through functions that export using the C Application Binary Interface (ABI). See modlibrary.c for a basic template.

Each new module for Outpost 2 should be placed in a separate directory that resides in the root directory of Outpost 2. The module should be designed to interface with Outpost 2 by either being added as an argument to the command line when opening Outpost 2 or by being specified in the Outpost2.ini file.

If the module contains configurable settings, history, or other data, and it is designed to be loaded through the Outpost2.ini file, it should store those settings in the Outpost2.ini file

Available functions for a module to export to op2ext.dll (see modlibrary.c of details):

 - mod_init()
 - mod_run()
 - mod_destroy()

op2ext also provides the following functions for use by the custom module (see public.h for details):

 - void GetCurrentModDir_s(char* buffer, int bufferSize);
 - [DEPRECATED] char* GetCurrentModDir();
 - void GetGameDir(char* buffer);
 - void AddVolToList(char* volName);
 - void SetSerialNumber(char major, char minor, char revision);


Module Serial Numbers
------------------------------------------
Outpost 2 contains an internal serial number that is used during multiplayer to ensure both the host's and guest's copies of Outpost 2 are compatible. This check occurs when the guest initially joins the match's chatroom. If the copies are not compatible, the guest will be immediately booted, and the 2 incompatible version numbers are listed for reference.

When an Outpost 2 module affects part of the game that could cause problems during multiplayer matches, the module must change the internal serial number with the function SetSerialNumber. This will prevent someone from joining the match that does not have the mod and possibly causing crashing bugs.

The serial number is represented by three single digit numbers (0-9). Serials 0.0.0 to 0.9.9 are reserved for new modules affecting multiplayer.

Using SetSerialNumber does not change the in game serial number listed in Outpost 2 under the about section.

 
Vol Files
------------------------------------------
Vol files are a custom Outpost 2 storage format used to store in game files like maps, tech trees, tile sets (wells), etc. Editing vol files requires custom software. See the Outpost 2 mapper or OP2Archive (https://github.com/OutpostUniverse/OP2Archive) for vol file manipulation.

Vol files must be loaded into Outpost 2 before the game initializes. A module should accomplish this by adding vol files to Outpost 2 within the function mod_init by calling AddVolToList.

Outpost 2 only supports 31 vol files. If loading more than 31 vol files is requested, op2ext will not load any vol file past 31 and will warn the user on which vol files were not loaded. Outpost 2 will continue to load, but depending on what data was contained in the discarded vol file(s), the program may or may not be able to function normally.


Change Log
------------------------------------------

Version 2.0.0

 * Add ability to load vol files with any name into Outpost 2.
 * Deprecated function GetGameDir(char* buffer).
 * Allow retrieving data from Outpost2.ini greater in size than 1024 characters.
 * Improve error messages when Outpost 2 has difficulty loading a module or vol files.

Version 1.0.0

 * Several releases of op2ext were made prior to 2.0.0 which are not currently documented.



My work on Outpost 2 is currently getting crowded out with other priorities. I wouldn't expect more than some minor lifting on my part until some time in January. Sorry guys. This just took longer than I had available. (Probably because I kept working on random parts of op2ext not related to loading any vol file... but oh well).

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: leeor_net on December 05, 2017, 09:26:43 PM
Want to suggest (again I think?) that markdown is probably a better format to use for readme's.

Also minor edit to your post, I put the readme file into a code tag for the sake of readability.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 05, 2017, 10:21:06 PM
Quote
It has several functions that should not be called by code external to ipDropDown.cpp

Ok, now how do classes differ from namespaces? ;)
(A question I didn't think to ask for many years)

Quote
Also, it has member variables that should only be updated by ipDropDown.cpp. In particular:

char ipStrings[10][47];
int numIpStrings = 0;

That's perhaps closer to the reason. A class ties functions together with instance data.

Now, how many instances will there be of this class? I'd say this sounds like a singleton class. Which might be implemented as a class with static methods and variables. Which is much like a namespace with private global variables (excuse the contradictory sounding terminology).

Actually, if those methods are used to hook internal Outpost2.exe code, you can't move them into a class without also declaring them static, since otherwise you'd be changing the function signature to use an incompatible binary interface.

Quote
If we move the declaration of a function or a variable to the .cpp file, is this considered encapsulated? It might be a nice alternative. Since the rest of op2ext doesn't need to call WriteAddressesToIniFile, we could move its declaration to the .cpp file. Would this be a positive move?

That would be a good idea. Whenever possible, I like to keep things only in the .cpp file rather than the .h file. It reduces namespace pollution, and can improve compile times. Moving a function from a .h to a .cpp doesn't exactly make it private, but it does make it less visible. Another file could still access the function by explicitly declaring its signature.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 07, 2017, 01:03:47 AM
After saying there would be no time for major updates until January, I found some time. Life is funny like that. :)

First up, function declarations in IpDropDown not meant for public consumption were moved from IpDropDown.h to IpDropDown.cpp.

Full content of IpDropDown.h
Code: [Select]
#pragma once

void InstallIpDropDown();

Quote
Ok, now how do classes differ from namespaces?
(A question I didn't think to ask for many years)

Namespaces are a tool for preventing naming collisions and organizing code into logical sections. Classes are for grouping code with a similar responsibility into an object that has a clearly defined public interface and hidden internal workings. Well, that is the answer from my C# hobby perspective anyways... (I didn't look that up, just came out of my brain so it is probably a bit off).



The public interface has been moved from public.h to op2ext.h.

Additionally, I wrote bool GetGameDir_s(char* buffer, unsigned int bufferSize). It returns false if the provided buffer size is too small for the path, but still fills the buffer with as much of the path as possible. Looking for feedback if the bool return is a good addition or not...

op2ext.h's new contents
Code: [Select]
// Outpost 2 Extensions (op2ext) external interface. 
// Exposes functions for use when applying modules to Outpost 2 via the C ABI.
// See ReadMe for specific use instructions. For help, post on the Outpost Universe Forum (https://www.outpost2.net/).

#pragma once

#define EXPORT extern "C" __declspec(dllexport)

// Retrieves the current absolute directory of the Outpost 2 executable with a trailing slash.
// If bufferSize is smaller than required to copy entire path, buffer is provided as much of path as possible and false is returned.
EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize);


// DEPRECATED as of version 2.0.0. Use GetGameDir_s instead.
// Retrieves the current absolute directory of the Outpost 2 executable with a trailing slash.
// @buffer Pass a buffer of size MAX_PATH length.
EXPORT||deprecated("GetGameDir was deprecated in op2ext ver2.0.0. Use GetGameDir_s instead.")||
void GetGameDir(char* buffer);


// Returns a pointer to a buffer of [MAX_PATH+1] representing the parameter(s) passed into Outpost 2 after the /loadmod argument.
// The consumer must free the buffer when finished with it.
EXPORT char* GetCurrentModDir();


// Allows loading a vol file into Outpost 2. volFilename must be passed before Outpost 2
// initializes by passing by calling this function from mod_init.
// @param volFilename The vol filename ending in a null terminated string.
//                    Must be a relative path from the directory containing the Outpost 2 executable.
EXPORT void AddVolToList(char* volFilename);


// Overwrites the default Outpost 2 version string.
// Required if the module affects multiplayer to detect incompatibilities between different copies of Outpost 2.
// See the ReadMe for detailed usage. Each variable must be a numeric value between 0-9 and not an ASCII character.
EXPORT void SetSerialNumber(char num1, char num2, char num3);

The one area I'm not sure about here is that I had to include 3 extern calls in op2ext.cpp to bring in the mod loader, and volList, and one bool variable. It doesn't seem too clean, but in order to give EXPORT functions access to the different functions like adding vol files, getting the game directory, and the mod directory, op2ext.cpp will need access to this code.

Also, can anyone shed light on what EXPORT int StubExt = 0 does? It is available externally, but only declared and defined in op2ext.cpp. It should probably be declared in op2ext.h if it is truly meant for consumption outside of the op2ext dll...

op2ext.cpp
Code: [Select]
. . . Includes . . .

extern ConsoleModuleLoader consoleModLoader;
extern VolList volList;
extern bool modStarting;

EXPORT int StubExt = 0;

EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize)
{
    . . .
}

EXPORT void GetGameDir(char* buffer)
{
    . . .
}

EXPORT char* GetCurrentModDir()
{
    . . .
}

EXPORT void AddVolToList(char* volFilename)
{
    . . .
}

char *verStrAddr = (char*)0x004E973C;
EXPORT void SetSerialNumber(char num1, char num2, char num3)
{
    . . .
}



Listing of recent commits. Maybe I need to take what Hooman said to heart and try to reduce the number of commits that I make a little by combining them locally first before pushing to the remote repository...

* Improve encapsulation of IpDropDown

 - Moved function declarations not meant for public consumption to .cpp file.
 - Moved includes to .cpp file since they are no longer required in IpDropDown.h.

* Move all non EXPORT declares from op2ext.h into op2ext.cpp

 - Preparation work for moving all EXPORT function declarations into op2ext.h

* Move non-EXPORT code from op2ext.cpp into DllMain.cpp

In preparation for moving all EXPORT functions into op2ext.h/op2ext.cpp.

* Move public interface for op2ext from public.h to op2ext.h

 - Added EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize)
    - Allows for a safer version of GetGameDir that is compatible with the C ABI
 - Moved definitions of all EXPORT functions to op2ext.cpp
 - Improved comments on EXPORT functions

* Rename ModMgr to ConsoleModuleLoader

* Remove dllexport from GetGameDirectory function

It returns a string and is not compatible with the C ABI. See GetGameDir_s for new EXPORT function.

* Remove unnecessary extern call for a modManager in op2ext.cpp

* Rename class CommandLineModuleLoader to ConsoleModuleLoader

Lines up with new filename ConsoleModuleLoader.h/ConsoleModuleLoader.cpp and is a little shorter in length when typing class name.

Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 07, 2017, 03:10:56 AM
Quote
Namespaces are a tool for preventing naming collisions and organizing code into logical sections. Classes are for grouping code with a similar responsibility into an object that has a clearly defined public interface and hidden internal workings.

Sounds like you've said the same thing twice. What's the difference? "organizing code into logical sections" versus "grouping code with similar responsibliity". Having a clearly defined public interface and hidden internal workings could also be said of namespaces: Wrap the private stuff in a nested anonymous namespace.

To me, the main difference is instance data being tied to functions.

Quote
Additionally, I wrote bool GetGameDir_s(char* buffer, unsigned int bufferSize). It returns false if the provided buffer size is too small for the path, but still fills the buffer with as much of the path as possible. Looking for feedback if the bool return is a good addition or not...

Yes, this is a proper way to handle this. Personally, I've come to detest success/failure return values of methods that are supposed to do something (as opposed to true/false return values of methods designed to check something). Having to always check success/failure at the point of call quickly leads to messy code. The cleaner way is to use exceptions. However, due to C interface restrictions on the exported functions, an exception is not possible here. Hence you have to fall back to the more C way of doing things with a success/failure return value.

Code: [Select]
#define EXPORT extern "C" __declspec(dllexport)

Careful about putting this in the header used for importing. ;)

Often macros like this are conditionally defined for a shared import/export header. If nothing special is done, the default is typically dllimport. If a project specific define is set, it can use dllexport. Another option is to try moving the dllexport part to the implementation file, which may actually make more sense if you can get it to work.

Quote
The one area I'm not sure about here is that I had to include 3 extern calls in op2ext.cpp to bring in the mod loader, and volList, and one bool variable. It doesn't seem too clean, but in order to give EXPORT functions access to the different functions like adding vol files, getting the game directory, and the mod directory, op2ext.cpp will need access to this code.

You can potentially use a master internal header file. Perhaps op2ext-internal.h, which includes op2ext.h, and then adds additional declarations for project private methods. Though that is mostly useful if those same extra methods are used among multiple files. If they're only used in one file (while possibly being defined in another), writing a declaration once in a header versus once in the only source file they are used is still the same amount of typing. Having to create tiny headers to export 1 item often doesn't feel worth it. A declaration versus an include is still one line. If you're declaring 3 items all from the same external file, maybe it's time to consider a header file for those methods, or perhaps consider if the functionality is well encapsulated.

Quote
Also, can anyone shed light on what EXPORT int StubExt = 0 does? It is available externally, but only declared and defined in op2ext.cpp. It should probably be declared in op2ext.h if it is truly meant for consumption outside of the op2ext dll...

Hmm, curious indeed. This probably need to be documented better. I grepped the source and found no uses of it. I also checked git blame, and it was never modified since the project's initial checkin. With no documentation, and no use, I initially assumed it was dead code that had accidentally been left in and should be removed. Though being external, it could be used by another project. With no documentation, you can't much expect a mod to use it. Though a quick check of Outpost 2:
Code: [Select]
grep -rn "StubExt" *
Binary file op2ext.dll matches
Binary file OP2Shell.dll matches
Binary file Outpost2.exe matches

It's likely a dummy export to satisfy a linking requirement from Outpost2.exe and OP2Shell.dll. Try removing it and see if the op2ext DLL fails to load at runtime. This is also likely why it's just defined as an int which is set to 0. There is no type information nor type checks with exported symbols. It's just a name and an address. Declaring an int, would be an easy way to generate an export with the required name.

I've also verified there are no "StubExt" matches with an original CD install. I think BlackBox added a dummy entry to the import table of Outpost2.exe to force loading op2ext.dll. Likely no other reason for that entry.

In which case, it doesn't need to be public, so doesn't need to be in op2ext.h. You can move it to the .cpp file.

Quote
Listing of recent commits. Maybe I need to take what Hooman said to heart and try to reduce the number of commits that I make a little by combining them locally first before pushing to the remote repository...

I don't really object to lots of small commits. They are easy to verify that way. Though once you push, you can't really amend things easily. Hence, my suggestion is more to not push your changes right away after each commit. Wait until the end of your programming session to push. Plus, you'll have a better sense of how you should have done the work by then, so you can repackage your commits nicer if you realize any were botched or out of order.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 07, 2017, 01:36:13 PM
leeor,

Sorry I forgot to address your comment earlier. I'm split about the use of markdown syntax. While I like the nice formatting when reviewing the ReadMe on GitHub, when I download the repo onto my computer, there isn't a standard program to open a .md file. So I end up having to manually tell Windows 10 to use Notepad/Wordpad or something. These programs open the file in raw format (without the markdown formatting applied). So now I have to review the readme in raw form or hunt around the internet for a program that shows the markdown formatted. Fortunately, the markdown is fairly clear to read even in raw form, but this is really annoying to me. Just using a .txt file seems to be most compatible.



Quote
Often macros like this are conditionally defined for a shared import/export header. If nothing special is done, the default is typically dllimport. If a project specific define is set, it can use dllexport. Another option is to try moving the dllexport part to the implementation file, which may actually make more sense if you can get it to work.

I'm not exactly following you here. Maybe a short example would be helpful. Are you saying that instead of EXPORT, we should use OP2EXT_API for the macro in op2ext.h?

Code: [Select]
#define OP2EXT_API extern "C" __declspec(dllimport)

None of the functions marked for EXPORT are used within op2ext.



When commenting out SubExt, I get the dialog box with the following error and the game does not load:

Entry Point Not Found
The procedure entry point StubExt could not be located in the DLL Outpost2.exe.


I added the following comment, and StubExt was already in the .cpp file.

Code: [Select]
//Dummy export for linking requirements from Outpost2.exe and OP2Shell.dll. Forces Outpost2.exe to load op2ext.dll.
EXPORT int StubExt = 0;

Please update the comment if I worded it wrong.



Quote
You can potentially use a master internal header file. Perhaps op2ext-internal.h, which includes op2ext.h, and then adds additional declarations for project private methods.

op2ext.h is not used anywhere in the rest of the application, so it wouldn't need to be included in op2ext-internal.h. op2ext.h is currently strictly for consumption by external modules, not within op2ext.

Both dllMain.cpp and op2ext.cpp would #include op2ext-internal.h. Then the declarations for the static instances of the following variable and classes would exist inside op2ext-internal.h instead.

op2ext-internal.h
Code: [Select]
#include "ConsoleModuleLoader.h"
#include "VolList.h"

extern ConsoleModuleLoader consoleModLoader;
extern VolList volList;
extern bool modStarting;

Moving the declaration of volList from VolList.h to op2-internal.h and consoleModLoader from ConsoleModLeader.h to op2ext-internal.h seems like a good idea to me. I think we are getting there...

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 12, 2017, 08:51:14 PM
Quote
Are you saying that instead of EXPORT, we should use OP2EXT_API for the macro in op2ext.h?

That would perhaps be a better descriptive name. The macro will need to expand to dllexport when compiling op2ext, and expand to dllimport when compiling a project that links to op2ext. Expansion can be controlled by a second macro, which is set in the op2ext project settings, under preprocessor definitions. If the macro is set, you should use dllexport. A client project which uses op2ext won't have this macro set, so it's absence would indicate dllimport is desired.

Code: [Select]
#ifdef OP2EXT_PROJECT_SPECIFIC_MACRO  // Defined in op2ext project settings
  #define OP2EXT_API __declspec(dllexport)
#else
  #define OP2EXT_API __declspec(dllimport)
#endif

Quote
None of the functions marked for EXPORT are used within op2ext.

That's fine, and largely irrelevant. There is a distinction between the function declarations and the function definitions. The function definitions (in the .cpp file) will always be exported. The function definitions are only ever seen when compiling op2ext. The function declarations (in the .h file) may need to change. Only the header file is used by both the op2ext project, and other projects that use op2ext. The function declarations in the header will need to specify import, when compiling from other projects.

Note that I left out what the header file should say when compiling the op2ext project itself. It should be correct to specify export. That certainly won't conflict with the function definitions. I suspect it might be fine to not declare export, if perhaps forward declares don't need to match perfectly. Or maybe you can choose just one of either the definition or the declaration to mark as export. I'm not actually sure. In fact, you shouldn't even need to use the header file at all when compiling op2ext, since you're not using those functions internally, and so don't need to see them to call them from anywhere. Think back to the public.h that existed previously, but wasn't used within the project itself. Though I would recommend a #include for the header in the file that define the functions, since projects change over time, and the compiler can warn if the declarations and definitions drift apart, provided it can see both.



Maybe:
Code: [Select]
// Dummy export for linking requirements from Outpost2.exe and OP2Shell.dll. Outpost2.exe and OP2Shell.dll reference this dummy entry, causing op2ext.dll to load. It is not used in any way, but must exist to prevent Windows loader errors.

Really though, I'm wondering why Outpost2.exe and OP2Shell don't just reference one of the standard export functions. I'm also wondering why both have references, rather than just one of them.



You may be right about moving those extern declarations. It seems a bit messy to define an extern of a type in the same header file that declares that type. Though it can make sense in the case of singletons.

Btw, don't take the header file name I used too seriously. It's probably not a great name, and the format is not consistent with other header files.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 20, 2017, 10:53:14 PM
I spent some more time on op2ext today.

I implemented the macro for switching between dllimport and dllexport within op2ext.h.

Code: [Select]
#ifdef OP2EXT_INTERNAL_BUILD  // Defined in op2ext project settings
#define OP2EXT_API extern "C" __declspec(dllexport)
#else
#define OP2EXT_API extern "C" __declspec(dllimport)
#endif

I implemented a new testModule subproject within op2ext. It reports info to the IDE output window to prove the op2ext public interface is working. Shell is in place, but more work will be required to get everything testing.

I noticed that we call different functions for console loaded versions of the modules and .ini versions of modules.

Console module vs .ini module
mod_init          InitMod
mod_run           ?? No equivalent
mod_destroy       DestroyMod


I'm thinking that we should unify the names by switching the conosole mod names over to the .ini module names. I don't think anyone is currently using a mod that calls any of the functions from console loading, so this should be an okay breaking change.

I'd also like to create a new class that handles the actual loading of mods. The current ConsoleModuleLoader and IniModuleLoader classes will become classes that simply list the locations of modules defined either by a console switch or in the .ini file. They will then pass these modules to the new class for loading.

This will prevent the naming and use of the public module functions from diverging again.



Recent Commits

Hooman will be pleased to learn that I made my first partial commit of a file today using TortoiseGit. Doesn't get much nerdier than that. Sigh.

* Remove bug from ConsoleModuleLoader that prevented loading proper directory

* Remove outdated Visual Studio workspace and project file

* Update comment on StubExt to be more specific

* Add macro to change external interface from dllexport to dllimport

 - When op2ext.h is compiled/linked to within op2ext project, external functions are declared for export.
 - When op2ext.h is compiled/linked to by an extension module, external functions are declared for import.

* Create TestModule project

 - Designed to test the public interface of op2ext.
 - Serves as another template for how to link a module to Outpost 2 using op2ext.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 21, 2017, 04:33:10 AM
Quote
I'm thinking that we should unify the names by switching the conosole mod names over to the .ini module names. I don't think anyone is currently using a mod that calls any of the functions from console loading, so this should be an okay breaking change.

Right. I'd forgotten about that difference. Interesting observation as to use. What about Renegades? I think it was loaded using the module system.

It might also be possible to internally map the two function names to the same internal function pointer for each module. Ideally, a module should be loadable using either the ini file or a command line switch.

Quote
I'd also like to create a new class that handles the actual loading of mods. The current ConsoleModuleLoader and IniModuleLoader classes will become classes that simply list the locations of modules defined either by a console switch or in the .ini file. They will then pass these modules to the new class for loading.

A common base class might work. That might require less code restructuring.

Quote
Hooman will be pleased to learn that I made my first partial commit of a file today using TortoiseGit. Doesn't get much nerdier than that. Sigh.

 :)

I assume this means you used the Git Index to commit only come changed lines, rather than all changes to a file.

Very good work.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 22, 2017, 01:47:34 AM
I had a question in op2ext that perhaps someone can help explain. Running Outpost 2 with a command line switch module enabled adds the following line to Outpost2.ini (the weird directory call is because I've got the debugger attached from another folder than the game's executable):

Code: [Select]
ART_PATH=C:\Users\Brett\Documents\OutpostUniverseGitHub\op2ext\..\..\Outpost2SVN\GameDownload\Outpost2\trunk\TestModule

ART_PATH seems to allow multitek2 to load it's modified sheets.vol file. However, it doesn't load other randomly named vol files that I stuff into the module's directory. Below is the comment on the code that adds ART_PATH to the .ini file. I wanted to clean up the comment to be more useful, but whatever it does seems to be external to op2ext's code. If someone knows about it, I would appreciate a step in the right direction.

Code: [Select]
// Set that magic DEBUG\ART_PATH value in the .ini
WritePrivateProfileString("DEBUG", "ART_PATH", modDir.c_str(), GetOutpost2IniPath().c_str());



Test Module Improvements

The TestModule is a lot more robust now. See below for what it spits into the output window of Visual Studio. It allows testing that all the dllexternal functions from op2ext are called and provide reasonable results.

 * It reports when each of the module's function hooks (mod_init, mod_run, and mod_destroy) are called.

 * There is a function for testing what happens when a missing vol filename is provided. (Brings up a popup window and warns user properly).

 * There is a function for testing attempting to load too many vol files. (Brings up a popup for each vol file past the limit allowed in loading Outpost 2.) Curiously, this function causes NetHelper to crash (see below for message). This function doesn't allow for the normal vol files to load (maps.vol, story.vol, sheets.vol, etc), because it overloads Outpost 2 with a bunch of empty vol files. Not sure how that is connected to NetHelper crash. At least the user receives a report that they loaded too many vol files and reducing the number will fix the problem.

NetHelper Error Message when too many vol files loaded

Exception thrown at 0x25005E9C (NetHelper.dll) in Outpost2.exe: 0xC0000005: Access violation reading location 0x000000B8.

25005E99  mov         eax,dword ptr [edi+10h] 
25005E9C  mov         esi,dword ptr [eax+ebx*8] 
25005E9F  push        ebx 


Test Module Output

Test Module mod_init called.
GetGameDir_s reports: C:\Users\Brett\Documents\OutpostUniverseGitHub\op2ext\..\..\Outpost2SVN\GameDownload\Outpost2\trunk
GetGameDir reports: C:\Users\Brett\Documents\OutpostUniverseGitHub\op2ext\..\..\Outpost2SVN\GameDownload\Outpost2\trunk
GetCurrentModDir reports: C:\Users\Brett\Documents\OutpostUniverseGitHub\op2ext\..\..\Outpost2SVN\GameDownload\Outpost2\trunk\TestModule
VolList::AddItem("moduleTestFail.vol");
Test Module mod_run called.
Test Module mod_destroy called.


Next up is hooking the testmodule into the .ini file. Then I'll look at cleaning up/combining parts of the .ini and console module loaders. Also, I'll look at testing Renegades to see if it uses the console module loader function hooks or not.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 22, 2017, 06:06:36 AM
The ART_PATH setting from the DEBUG section of the ini file is an internal debug feature in Outpost2.exe, which some people discovered and used. I don't remember quite what it does, but I think it allowed loading resources from an alternate path. This may not work in conjunction with how op2ext chooses to load VOL files.

Checking Outpost2.exe, this config setting is accessed in two places ResManager::CreateStream, and ResManager::GetFilePath. Headers declaring these functions likely exist in the Outpost2App project, if you can find it, though I doubt it will contain much if anything in the way of details.


No idea about the NetHelper error, though the source is up on GitHub, so you could potentially build it and get debug info.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 23, 2017, 02:47:27 AM
Thanks Hooman, your comments on ART_PATH helped a lot. You are correct that it adds a second directory to search for resources/media files. It looks like the game searches the ART_PATH first for the file, then the game's executable second if the file is not found at ART_PATH. This applies to all .vol files, .avi files (movies), the .clm file (music), and the main menu music track (music1.wav). Determined by testing, not code inspection. I'll add some thorough notes about it to the ReadMe. I've improved the comment associated with ART_PATH in the source code as well on my local machine and will push probably tomorrow.

I was confused last post on how the game was loading sheets.vol from the module. Turns out, op2ext found sheets.vol by searching the root directory of Outpost 2 for all vol files in the directory. It then passed that directory in relative form to Outpost 2 for loading. Outpost 2 then looked first in the ART_PATH folder for the file and found it so didn't pull the one from the primary directory. (Hopefully that makes sense, it is kind of confusing.)

If I recall correctly Outpost 2 requires relative directories when loading .vol files. I had to set them as relative when pushing from op2ext into outpost2.exe for Outpost 2 to consume them, which is probably because Outpost 2 supports loading vol files from different relative locations.

This means unless there is some way to make ART_PATH take a list of directories, we are stuck supporting only one module from the console switch. sirbomber's multitek2 requires this function to work. (It wouldn't be difficult to create a dll to route sheets.vol instead of using ART_PATH for multitek2 though).

As for overloading vol files crashing NetHelper, I think this has more to do with crashing Outpost 2. When disabling NetHelper, if maps.vol is not loaded, then Outpost 2 itself just closes out before getting to the main menu. So Outpost 2 closing out is possibly why NetHelper doesn't work (or even if it worked, Outpost 2 isn't anyways). I'm assuming it is from specific file(s) from inside maps.vol that are required for loading Outpost 2, but this is probably enough testing for now on the issue. When overloading Outpost 2 with too many vol files, but ensuring maps.vol makes the cut, NetHelper loaded fine.

I'm in the middle of creating ModuleManager.cpp/.h (more refactoring then creating from scratch really). This class will actually load modules and their associated dlls after being passed the details of where they are located from either the .ini module loader and the console module loader.

Since there is some memory management involved in ModuleManager, I will probably leave it as a loose set of functions and variables inside the global namespace instead of encapsulating in a class, just like the IPDropDown code. I'll leave declarations for what should be private fields in the .cpp file only to discourage their external calling.

Once this work is completed, I will be looking at another pass at the readme, more testing and general refactoring, and wrapping up op2ext. I'll set aside time to work on something else when done with op2ext to give others time to review the code before the final build is made for Outpost 2 1.3.7.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 29, 2017, 11:01:02 AM
Quote
I was confused last post on how the game was loading sheets.vol from the module. Turns out, op2ext found sheets.vol by searching the root directory of Outpost 2 for all vol files in the directory. It then passed that directory in relative form to Outpost 2 for loading. Outpost 2 then looked first in the ART_PATH folder for the file and found it so didn't pull the one from the primary directory. (Hopefully that makes sense, it is kind of confusing.)

That is very interesting. Good note to have, we should document that. That might actually be helpful behaviour, though it is rather unintuitive. That basically means the ART_PATH variable does work in conjunction with how op2ext builds a list of VOL files to load.

Quote
This means unless there is some way to make ART_PATH take a list of directories, we are stuck supporting only one module from the console switch.

You're right. I don't think there is a way to support multiple modules from the command line without breaking pieces of the existing design. At least not without additional hacks to the game to support more search folders. More simply, you could have it still set ART_PATH, knowing that only one will actually be effective, which is fine if only one module actually needs that setting, and the modules are loaded in a set order.

We could also deprecate the old design, and perhaps have a module that supports loading old style modules.  :P



As for an alternative design, not every module will need to override the load path. For the ones that do, there are alternative solutions. For instance, for VOL files, the module itself could add VOL files to the load list using the op2ext exported functions. Modules are loaded before the game directory and "Addon" folder are scanned for VOL files. Hence VOL files set by modules will come first in the VOL list. For other resources, the module itself can set the ART_PATH in its initialization function, or hack the game in some other way. This means only modules that absolutely need to use ART_PATH would modify it, thus reducing contention. Essentially, push setting ART_PATH out of op2ext and into the modules, which is of course a breaking change with the original design, and would require adapting and recompiling modules.

As for mutual module compatibility with such a design, op2ext could monitor for changes to ART_PATH when loading modules, and warn of incompatible modules if more than one module tries to set it during initialization.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 29, 2017, 05:19:13 PM
Quoting myself:

Quote
I'm in the middle of creating ModuleManager.cpp/.h (more refactoring then creating from scratch really). This class will actually load modules and their associated dlls after being passed the details of where they are located from either the .ini module loader and the console module loader.

This work went poorly. The completed code was larger (more lines) and more complex than I envisioned. Complexities arose beyond the expected double names for each module exported function (e.g. mod_destory & DestroyMod). I had forgotten that the ini version of InitMod(char* iniSectionName) had the sectioName variable while the console module did not. So I had to add separate calls to the ini function based on where the module originated. Also, the ConsoleModuleLoader still had to deal with setting the Art_Path and destroying the Art_Path in the DEBUG section of the .ini module.

Beyond this, my local copy of op2ext currently crashes when calling the hooked module functions (like mod_run). I think the problem is associated with how I'm checking if the function exists, but not sure and I don't know how to fix it.

I'm considering canning the idea of consolidating loading modules into a single ModuleManager class. Considering the variances involved in loading the modules from the 2 sources, perhaps it is better to leave them as separate processes (and frankly, it currently works). If anyone feels otherwise, we can schedule a pair programming session to debug my current work.



Quote
We could also deprecate the old design, and perhaps have a module that supports loading old style modules.

I'm not necessarily opposed, but I don't see working towards deprecating current functionality of this as being in the cards right now. I'd like to get the current work finished. I've documented art_path briefly in the source code. A more detailed blurb will also exist in the ReadMe file soon. Hopefully with the improved documentation, it will be a bit more approachable if someone new (or familiar with Outpost 2) wants to take a stab at providing a new module.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on December 29, 2017, 07:34:55 PM
Did you notice the old vs. new op2ext code that you had to merge earlier makes a slight change to the console mod loader setting ART_PATH: it hooks the 2 places where OP2 reads that INI setting and fills in the mod path, instead of actually writing ART_PATH to the ini and deleting it every time you ran the game with a mod, just because that felt silly to me. :P I should look at those 2 places in OP2 again.

Code: [Select]
/* The forum's code highlighter plugin sucks */
char modDir[MAX_PATH+1];

int __fastcall GetArtPath(void*, int, char*, char*, char *destBuffer, int bufferSize, char *defaultValue) {
  strcpy_s(destBuffer, bufferSize, modDir);
  return strlen(modDir);
}

...

void ApplyMod(char *modDir) {
  ...

  /* Insert hooks to make OP2 look for files in the mod directory */
  /* In ResManager::GetFilePath */
  Op2MemSetDword((void*)0x004715C5, (DWORD)&GetArtPath - (loadOffset + (DWORD)0x004715C5 + sizeof(void*)));
  /* In ResManager::CreateStream */
  Op2MemSetDword((void*)0x00471B87, (DWORD)&GetArtPath - (loadOffset + (DWORD)0x00471B87 + sizeof(void*)));

  ...

Edit: Apparently this code isn't in the version on GitHub. Was that intentional or did you forget to merge ModMgr.cpp?

ResManager::GetFilePath() would be pretty easy to hook to check multiple mod directories, as long as you don't care about the limitation of only 1 mod can override a particular file (e.g. sheets.vol) - it'd be cool to handle that in a more sophisticated way and allow searching inside multiple VOLs, but that'd probably be a huge pain and I'm not sure if there'd be any need for that right now.

ResManager::CreateStream()'s ART_PATH check looks more contrived than the other one, but that shouldn't be hard to hook either.

You could implement it in a quick and dirty way by just taking my code above and changing GetArtPath to iterate over all mod dirs until you find a folder where the file exists, and if not then strcpy defaultValue to destBuffer (or set it to an empty string if defaultValue == nullptr).
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on December 30, 2017, 07:57:20 PM
Quote
Edit: Apparently this code isn't in the version on GitHub. Was that intentional or did you forget to merge ModMgr.cpp?

This is definitely an oversight. I prefer how you are modifying Art_Path over using the .ini file as well. I need to sort through a couple of commits while ripping out the code I don't like on my local copy. Then I'll get your change posted. Should be done within a couple of days.

Geez, Hopefully I didn't miss anything else..

I like your suggestion on hooking ResManager::GetFilePath(). It isn't something that I'm planning to do in the short term though.

I'm curious what the reasoning is behind marking GetArtPath __fastcall? I don't really know what __fastcall is used for.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on December 30, 2017, 11:22:59 PM
I'm curious what the reasoning is behind marking GetArtPath __fastcall? I don't really know what __fastcall is used for.
__fastcall is the calling convention (__cdecl is default, though __fastcall is pretty common too). The differences between calling conventions is a topic that goes down the assembly rabbit hole, in case you're wondering. The code patching I'm doing just replaces the pointers in a couple function calls to CConfig::GetString (that would normally read the ini setting). The calling convention and signature needs to match the original function or else everything explodes. I'm actually using __fastcall with a dummy 2nd parameter to emulate __thiscall, which works because reasons that also involve going down the assembly rabbit hole.

Did you notice that last bit in my post about the quick and dirty way to implement multiple mod directories? That'd be a really easy way out.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on December 31, 2017, 04:54:57 AM
Quote
/* The forum's code highlighter plugin sucks */

lol
Agreed. I would love to have better code highlighting on the forums.

The missed changes that Arklon pointed out should be merged in. Not really sure where to find that version of the code actually.

I think other changes to the module system can be left out for now. The two systems are a bit of a mess. I get the sense each person wanting to write modules had different goals and requirements, which is why we've ended up with two incompatible module systems in the first place. We should put some effort into documenting the various goals, and use that to guide a design for the module system. We might want to put more effort into this than simple code refactoring.



There are three main calling conventions to be aware of: __cdecl, __thiscall, __fastcall
__cdecl: All parameters are passed on the stack
__thiscall: First parameter is passed in the ECX register, remaining parameters passed on the stack
__fastcall: First 2 parameters are passed in ECX and EDX registers, remaining parameters passed on the stack

You can't simple declare __thiscall though, only imply it by making a function a member function of a class or struct. The first parameter is then the hidden "this" pointer. If you want to replace or otherwise write a function signature compatible with __thiscall, you need to account for "this" being passed in ECX. One hack, is to declare a function as __fastcall, and then ignore the second parameter corresponding to EDX.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on January 01, 2018, 04:35:56 PM
The missed changes that Arklon pointed out should be merged in. Not really sure where to find that version of the code actually.
Definitely not from somewhere else in this very thread, nope. :P

* Encapsulate ModMgr in a class.

Currently, the DEBUG section of the Outpost2.ini file is not being destroyed. I've noticed an empty Outpost2.ini file is created in the source code directory when debugging. The DEBUG section is added properly to the game directory on initialization. My theory is that the wrong directory is being used for destruction, but have not tested in code.
Uhh, you must not be using the latest version of the op2ext code. A while back I changed it so it doesn't use the DEBUG ini setting anymore, among other things like allowing ini mods to call cleanup functions on exit, and moved as much logic out of DllMain as possible. http://arklon.outpost2.net/other/op2ext_1-3-6.zip

Yeah, the op2ext code on GitHub looks out of date. I'd suggest busting out a 3-way code diff/merge tool. KDiff (http://kdiff3.sourceforge.net/) is a decent free one.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 02, 2018, 02:43:22 AM
Okay, GetArtPath function hook is now committed to the online repository. I tested it using multitek2 and it seems to be working fine.

Thanks for the info on __fastcall. I was reading online how people were trying to use it to improve speed of their function calls, but improvements were fickle and difficult to achieve for them. Sounds like we are using it to maintain compatibility with how the function exists within Outpost 2.

Quote
Did you notice that last bit in my post about the quick and dirty way to implement multiple mod directories? That'd be a really easy way out.

When pushing the code in, I looked at this. It would be pretty easy to implement like you are saying. I'm hesitant to add it though because it would mean we would have to build a list of console loaded modules just like the .ini modules. I would rather see all the modules unified into one list than creating two separate lists.

Also, we have an external function called OP2EXT_API char* GetCurrentModDir(). This returns the current directory of the console module. It gets weird if we start loading multiple modules. Perhaps if no one is using this function, we could just delete the function though from the public interface.

Quote
I think other changes to the module system can be left out for now. The two systems are a bit of a mess. I get the sense each person wanting to write modules had different goals and requirements, which is why we've ended up with two incompatible module systems in the first place. We should put some effort into documenting the various goals, and use that to guide a design for the module system. We might want to put more effort into this than simple code refactoring.

Agreed, to unify module management between Console and .ini at this point will take breaking changes no matter how it is sliced (from my perspective anyways). It was tough throwing out my code attempting to unify them.

As Arklon pointed out, I already had the code from his post earlier. Just missed it when merging the projects.



I'd like to get a list of available modules up on the wiki or somewhere else (unless this already exists and I just need to be pointed to it). It would give us an idea of what we are breaking before making major changes to the public interface.

I've ran across

Color Mod 1 (CM1) - sirbomber/Arklon (console loaded)
Color Mod 2 (CM2) - sirbomber/Arklon (console loaded)
Multitek2 - sirbomber (Console loaded, depends on ART_PATH only)
NetFix - Hooman (.ini loaded)
NetHelper - Arklon (.ini loaded)
Renegades(??? need to test if this actually uses op2ext or not) - EddyB and team

Are there other modules out there besides these ones that people are using? If we can document what parts of the interface they all use, we could maybe see what can be changed without actually bothering anyone.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 03, 2018, 07:58:25 AM
The game recorder was another module.

To complicate things, I think Renegades was once shipped with its own version of op2ext. I believe it needed a fix, which had made it incompatible with a previous version of op2ext. Not sure what the current state is.



The op2ext changes, were they kept separate because of SVN write permission restrictions? I know that had sometimes been a problem in the past. It wasn't obvious where the updated code was, and less obvious that there even was updated code. That's maybe closer to what I was thinking.

Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on January 03, 2018, 05:10:26 PM
The game recorder was another module.
And that thing never really worked anyway so I don't think anyone cares if we break compatibility with it. I have the source code somewhere anyway, but I'm not convinced there's any demand for making a new build of it for a new mod API.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 03, 2018, 09:14:56 PM
I spent a lot of time cleaning my work in op2ext. I think it is pretty much done now. Next I'd like to spend a bit of time on the Sample code, probably adding a sample for .ini modules and creating a Samples directory. Then clean up/review the now included test module.

Some more mods that I stumbled on in the repository that seem to be complete are below:

OP2DisasterlessMod - Arklon (Ini/Console)
OP2GPPatch - ZigZagJoe (Ini/Console)
OP2ScrollFix - ZigZagJoe (Ini/Console)
SSUtil2 (Screenshot Utility) - TH300 (Ini/Console)

I think we are suffering some by not having some sort of central listing of modules. I would have enjoyed using the color mods a lot earlier, but I didn't know they existed. The screenshot mod would have been useful as well. Without deep diving the forum, it is tough to know what is available. We have an addons section of the Wiki that I think is supposed to address this, but it needs a lot of attention. (Probably because Dave and I tag teamed it and didn't know about all the mods.) I can look at giving it an index with short descriptions/links to each mod. Some of the addons like Greenworld were using the wiki for managing tasks and detailed descriptions. That is cool, but I would be looking to cover more of just pointing to where more info on each mod can be found in the forum. Open to other suggestions than the Wiki if there are other ideas.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 03, 2018, 11:59:00 PM
I would actually be interested in having a working game recorder. That was a pretty cool idea. From what I remember, it sort of worked, but not reliably, with playbacks desyncing. Maybe the CommandPackets were being recorded too early in the game loop, which could have allowed them to change after being recorded. Or perhaps some state data was missed.

I had forgotten about those other mods too. You're right, it would be nice to have a standard place for mods. Though since people are free to write their own, and also free to not release the source code, there is no real way we can enforce that. At best, we can have a standard place for supported mods where we'd make some effort to maintain them in the case of breaking changes. In SVN there was a common folder. On GitHub we could tag the projects. A wiki page could be written, but the problem with a wiki page is it will inevitably get stale. The wiki requires someone to put in the extra effort to maintain it and keep it up to date. Unless perhaps we had a way to scan GitHub projects, and pull information from them to build a wiki page.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on January 04, 2018, 03:06:39 PM
I would actually be interested in having a working game recorder. That was a pretty cool idea. From what I remember, it sort of worked, but not reliably, with playbacks desyncing. Maybe the CommandPackets were being recorded too early in the game loop, which could have allowed them to change after being recorded. Or perhaps some state data was missed.
I agree, it would be cool to have a working one, but for that we'd be better off rewriting one from scratch. The old recorder had some amazingly incomprehensible bugs, which me nor BB were never able to find any obvious root cause for, such as the recording suddenly repeating the same set of X commands that happened over 50 marks or whatever forever.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 04, 2018, 09:55:01 PM
Quote
...such as the recording suddenly repeating the same set of X commands that happened over 50 marks or whatever forever.

That sounds like a huge clue. The game maintains a circular buffer of CommandPackets, with a delay between when they are issued (interpreting player commands) and when they are executed. I noticed this when working on a simple AI test after working on NetFix. When forcing a CommandPacket to be issued at a certain point in the game cycle, there were desyncs, but hooking a different point to issue commands worked fine. It was all about when the command was issued relative to the point at which commands became final and could be exchanged safely over the network. This could be related.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 05, 2018, 01:54:38 AM
Quick update,

I finally added Outpost2DLL from GitHub as a submodule to op2ext. I deleted OP2DLL.lib from op2ext since it isn't needed anymore when building against the Outpost2DLL source code. I made some minor commits to Outpost2DLL like giving it an ignore file.

The EXPORT macro had been placed in the wrong place in the sample module long ago. I fixed it and created a separate iniSampleModule. Both samples are now in a directory called Samples.

I finished cleaning the TestModule. It now attempts to pull some values from the .ini file to make sure everything is hooked properly.

Next up is working the ReadMe. Then some final module testing.

All the details are in the commit messages which should be easy to pull. I usually post them all here, but it is probably just as easy to read through them in Git/on GitHub for anyone interested in the details.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 06, 2018, 11:05:34 AM
Ahh, nice. I like the submodule change.


I noticed something a bit misleading in the ini sample module:
Code: [Select]
LoadAddons = ". . . other modules . . ., ModuleSectionName"
. . .

[TestModule]
Dll = "ModuleSectionName\ModuleDllName.dll"

The "TestModule" string is what should be in the LoadAddons line, while the directory name in the Dll key value is completely arbitrary (though will probably match just by convention).


I was also thinking the parameter to InitMod should be marked as const. This should not affect source nor binary compatibility, assuming modules have only ever read that string. It would be surprising if any modules exist that write to the string, and such use should be considered unsupported.


For the TestModule folder, would it be possible to test VolList overflow by adding the same VOL file multiple times? If not, perhaps the test itself do the file copying. That brings up another question. Should we specify behaviour if the same VOL file is added multiple times?
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 06, 2018, 06:59:28 PM
Thanks for the proofreading.

That [TestModule] was a copy/paste omission on my part. Fixed now in the public repo.

Quote
while the directory name in the Dll key value is completely arbitrary (though will probably match just by convention).

Completely agree.

Quote
For the TestModule folder, would it be possible to test VolList overflow by adding the same VOL file multiple times? If not, perhaps the test itself do the file copying. That brings up another question. Should we specify behaviour if the same VOL file is added multiple times?

Currently op2ext just passes all vol files to Outpost 2 for loading without checking for duplicate entries. Outpost 2 seems able to handle multiple files with the same name without crashing. I haven't investigated to determine exactly what it does. Since we are in no danger of hitting 31 vol files even with duplicates right now, I am happy not rejecting same filenames in the op2ext VolList class. It gets complicated to check since you would need to resolve relative filepaths, and do a non-case sensitive search. I think, relative filepaths resolve in 2 different locations if a Console Module is used. (Due to adding the alternate search location from Outpost2.exe to the ART_PATH directory).

I'm hesitant to reload the same filename repeatedly, because I don't know what happens internally in Outpost 2 when it encounters multiple vol filenames that are the same or resolve to the same location on the file system. I want to make sure that the VolList class is actually preventing too many vol files from overloading Outpost 2, which they may not if the vol file is the same.

So, if we want to reload the same vol file 32 times to test if VolList is working properly, I would want to investigate internal to Outpost 2 what is happening closer.


EDIT: Let's just test by loading the same vol file 32 times. It will reduce the number of files in the repository. When I think more about the problem, we are simply only allowed to provide 31 filenames to Outpost 2 for loading vol files. It doesn't matter whether these files are rejected or not once Outpost 2 has the filenames.

Quote
I was also thinking the parameter to InitMod should be marked as const. This should not affect source nor binary compatibility, assuming modules have only ever read that string. It would be surprising if any modules exist that write to the string, and such use should be considered unsupported.

Updated as suggested. Tested against NetFix, since it pulls the iniSectionName using ModInit, and it seemed to work. Since I'm already moving to version 2.0.0, best to add 'breaking' changes now.

I've been looking over the modules that others have created that I listed earlier. The situation isn't as rosy as I made it out to be. It appears several of them have bugs and/or no longer have an available compiled DLL due to link rot in the forums. I was going to use them for testing, but it appears my options are a lot more limited than I thought. Someone could recompile source code for some of them, but this is something I don't want to take ownership of right now.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 08, 2018, 12:44:07 AM
I put a lot of time into updating TestFunctions.h/cpp. Now only one vol file is needed to test among other improvements.

I noticed that VolList only accepts 30 vol files. I was saying 31 for some reason earlier. It is now a lot easier to tell that the TestFunction is testing by overloading 30 vol files instead of my bad swag at it earlier.

I reordered the precedence in which vol files are loaded into Outpost 2. The new priority is:

 - vol files loaded through ART_PATH via a Console Module. (loose vol files in root directory of console module)
 - vol files in ./Addon directory
 - vol files specified in console module's DLL (if DLL exists)
 - vol files specified in ini module DLLs
 - vol files in the root directory of Outpost 2

This made the most sense to me. If I'm putting things in the addon folder, I want them first. Then the console module takes priority over all the .ini modules. The ART_PATH is stuck on top sense Outpost 2 bypasses everything by checking in that directory first for all game assets.



Renegades

I played a little with Renegades. It uses the Console module loader to work. However, the installer mentions that it will install a patch to fix a bug in the console module loader (old name modmgr). No idea what the bug is though.

As a side not on Renegades, when I start a new campaign game in Renegades and then try to exit Outpost 2, Outpost 2 crashes. Happens with both op2ext 1.3.6 and the new op2ext dll. Exiting via the main menu is working though.



ART_PATH

I tested ART_PATH a little more. I verified ART_PATH takes precedence when loading loose .map and tech tree (.txt) files as well as the other mentioned game resources. Made sense, but thought I would be thorough.

I think this is already known and accepted behavior, but I tested loading the same name vol files from both the ./addon folder and the root directory. Outpost 2 will select the vol file from the ./addon directory and load it instead of the one in the root directory, allowing volumes to be overwritten. Just testing mostly to make sure the newer op2ext is behaving itself.



I'm done with my edits and refactoring now. My plan is to sit on it a day and if nothing comes up, I'll compile a release build and distribute in case anyone else wishes to test on their copies of Outpost 2 with it or against their modules. I'll freeze my own work (minus making suggested improvements by others) to give others time to review the source code for problems/improvements. I think Hooman is already looking through some of it which is appreciated.

Thanks,

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 08, 2018, 01:36:40 AM
Quote
EDIT: Let's just test by loading the same vol file 32 times. It will reduce the number of files in the repository. When I think more about the problem, we are simply only allowed to provide 31 filenames to Outpost 2 for loading vol files. It doesn't matter whether these files are rejected or not once Outpost 2 has the filenames.

Yes, the tests are more about the behaviours we care to specify, rather than what might actually be happening in full detail. Plus, there are two separate behaviours here, which could be tested separately. What happens if more than X number of files are specified, and what happens if duplicate filenames are specified.

Quote
I noticed that VolList only accepts 30 vol files. I was saying 31 for some reason earlier. It is now a lot easier to tell that the TestFunction is testing by overloading 30 vol files instead of my bad swag at it earlier.

This kind of sounds like a bug actually. I assume the array has room for 32 entries, the last of which will be the empty slot will null entries used to terminate the list. Assuming that's how it works, an array of size 32 should be able to support 31 valid entries. There might be an off-by-1 bug somewhere.

The VolList size is configured by a compile time constant. To avoid making the test brittle, it should depend on the same constant, rather than a hardcoded value. Unfortunately, that constant is now a private member of a class with the current design. Maybe it should become a public read-only constant, or an accessor method added.


I think mods should have higher priority than addons.

The ART_PATH seems like another dimension to the load order. The load order would resolve relative to the ART_PATH first, than relative to the game folder, within each priority level.


Not too sure what to suggest about Renegades. We don't have the source code for it. A crash on exit isn't too terrible though. The end result is essentially the same. Though it is a bit ugly and affects the user experience.

I also don't know what bug in op2ext Renegades was trying to patch.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 08, 2018, 11:47:54 PM
Quote
This kind of sounds like a bug actually. I assume the array has room for 32 entries, the last of which will be the empty slot will null entries used to terminate the list. Assuming that's how it works, an array of size 32 should be able to support 31 valid entries. There might be an off-by-1 bug somewhere.

Fixed to allow 31 vol files. Didn't break in testing either.

Quote
The VolList size is configured by a compile time constant. To avoid making the test brittle, it should depend on the same constant, rather than a hardcoded value. Unfortunately, that constant is now a private member of a class with the current design. Maybe it should become a public read-only constant, or an accessor method added.

I'm testing using a module DLL. This test suite is designed to ensure the external functions all work as advertised when called from an external DLL. I got carried away by adding the tests for too many vol files and the missing vol file conditions. It would be enough to just test that an external DLL can specify a vol file for loading. We would have to dllexport the max number of vol files for the DLL to understand the value.

The long term correct solution would be to create a new unit test project designed to test the innards of op2ext. And yes, make a public Get field of some sort for max number of vol files.

Doing the unit tests right would take more time than I'd like to devote to the effort right now. Kicking the can down the road. If testing for too many vol files in this way is giving too much heartache to Hooman or others, I can just delete that test for now.

Quote
I think mods should have higher priority than addons.

Edited to act like this. I don't have much of an opinion one way or the other.



I updated the expected location of post build scripts to assume that the Outpost 2 game code is in the root directory of op2ext (./Outpost2). I think this is more appropriate. Earlier, the scripts were copying files from op2ext to the SVN repo where I had Outpost 2. This directory scheme will likely be different for other developers.

With this new expectation, new directories and files are only created within op2ext, which seemed a lot more appropriate. This way if you compile without checking the post build scripts/readme, your only surpise would be a new Outpost 2 directory insdie op2ext as opposed to files strewn horizontally and randomly on your hard drive.

Perhaps if Visual Studio had a way to specify post build scripts in the .user files instead of the project file, it would be more helpful.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 10, 2018, 06:18:03 AM
Quote
Perhaps if Visual Studio had a way to specify post build scripts in the .user files instead of the project file, it would be more helpful.

Agreed.



I took a look through some of the code.

In VolList.h I don't think there is a need to include <windows.h>, nor for the WIN32_LEAN_AND_MEAN macro. Neither seem to be used here. It doesn't look like it's needed in VolList.cpp either.

In VolList.cpp, in function VolList::InitializeVolSearchEntry, the index can be incremented after struct fields are set.

The method name MaxVolFileCountReached seems odd. A typical convention for functions returning bool is to use an "Is" prefix or similar on the name. This convention can be seen in the methods exported from Outpost 2. Though in this case, it's more than just a boolean method, since it actually does something, namely popping up a dialog box in case the limit is reached. This isn't clear from the name, nor the use of checking a boolean return value. As this method is only called from one place, I would recommend just inlining the code in the caller and getting rid of the method. I think that would be more clear, and cuts down if tests.

It would be nice if the DBG macro could be reduced to 1 line, though I can kind of see why it was called over 3 lines for simplicity. I also noticed the DBG macro defined in GlobalDefines.h calls OutputDebugString, but there is no declaration nor include of that method in that header file.

In op2ext.h and op2ext.cpp, for method GetGameDir_s, consider declaring bufferSize as size_t rather than unsigned int. I've never been a big fan of the size_t type name, but I think it would be more correct.

In op2ext.h and op2ext.cpp, method SetSerialNumber, perhaps name the parameters more descriptively, such as "major", "minor", and "patch". Note that changing parameter names in method declarations (.h) has no impact on compatibility. They are only for documentation purposes. Changes to parameter names on the method definition (.cpp) has only local implications.

Do we need IpDropdown.h?

In IniModuleLoader.h, was there a reason for using std::list as opposed to std::vector for moduleList? It should be the same length as the std::vector returned by GetModuleNames.

I'm uncertain if the typedefs for the module function exports make the most sense in the header file or the implementation file. Thoughts?

IniModuleLoader::CallModuleInitialization has a memory leak. Since InitMod now takes a const char* pointer, the string copy is entirely frivolous. The original string should be passed, and the copy code removed.

Does CallModuleInitialization need to be it's own method? I'm thinking once the string copy code is removed, it might make sense to just inline it. That would also feel more consistent with how the DestroyMod method is looked up.

What's the point of the return value on IniModuleLoader::UnloadModules? For that matter, what is the point of the return value on DestroyMod? This value doesn't seem very actionable.

Should LoadModuleDll be responsible for displaying the error message? It's convenient, because that's where information is available, but something about it feels a bit odd. A cleaner solution might be to format and throw an exception, and have the caller display a dialog box or otherwise handle or log the error.

Inconsistency: IniModuleLoader uses HINSTANCE, while ConsoleModuleLoader uses HMODULE.


I'm going to stop here for now. There are a few sections I didn't look at in much detail.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Arklon on January 10, 2018, 05:11:36 PM
I updated the expected location of post build scripts to assume that the Outpost 2 game code is in the root directory of op2ext (./Outpost2). I think this is more appropriate. Earlier, the scripts were copying files from op2ext to the SVN repo where I had Outpost 2. This directory scheme will likely be different for other developers.

With this new expectation, new directories and files are only created within op2ext, which seemed a lot more appropriate. This way if you compile without checking the post build scripts/readme, your only surpise would be a new Outpost 2 directory insdie op2ext as opposed to files strewn horizontally and randomly on your hard drive.

Perhaps if Visual Studio had a way to specify post build scripts in the .user files instead of the project file, it would be more helpful.
CMake!

In IniModuleLoader.h, was there a reason for using std::list as opposed to std::vector for moduleList? It should be the same length as the std::vector returned by GetModuleNames.
No particular reason, change it if you want.

Quote
What's the point of the return value on IniModuleLoader::UnloadModules? For that matter, what is the point of the return value on DestroyMod? This value doesn't seem very actionable.
Result code. The main reason for this would be to indicate if cleanup of code hooks, etc. failed, so if we had an in-game mod manager - which is one of the 71248 things we talk about over and over and never actually make happen - it could be aware if the game is stuck in a dirty state and needs to restart. The other functions would need to have result codes to facilitate a mod manager as well, but doing that would be a non-backwards-compatible interface change. I wanted to establish right away that DestroyMod() returns a value, though making true (1) be the "ok" code was probably not a good idea if that needs to change to an enum type, not that changing it would be a big deal since the only thing that currently uses DestroyMod() is NetHelper.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 11, 2018, 12:11:18 AM
Thanks for the feedback.

Hooman stuffed a lot of comments in that post. I'm fairly busy with other things right now and will need some time to digest, code, test, write about, etc.

Thanks Arklon for the amplifying info on some of the code.

So, I'm working on this and will post in days to come.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 13, 2018, 01:02:39 AM
Again, thanks for the feedback Arklon and Hooman. I just quoted large sections of Hooman's feedback and then replied underneath each section. I'll push an rc2 in the other thread shortly.

Quote
In VolList.h I don't think there is a need to include <windows.h>, nor for the WIN32_LEAN_AND_MEAN macro. Neither seem to be used here. It doesn't look like it's needed in VolList.cpp either.
   

It was required in VolList.cpp in order to call the macro DBG. Otherwise, I would get the following error:

Error   C3861   'OutputDebugString': identifier not found

Windows.h should have been in VolList.cpp and not the header file though. Mute point now since I changed the MACRO DBG into a function called OutputDebug. See below for more info.

Quote
It would be nice if the DBG macro could be reduced to 1 line, though I can kind of see why it was called over 3 lines for simplicity. I also noticed the DBG macro defined in GlobalDefines.h calls OutputDebugString, but there is no declaration nor include of that method in that header file.

I changed the macro out for a function named OutputDebug. This way Windows.h could be included in GlobalDefines.cpp and cover the function's use of the Windows header instead of forcing all calling files to include Windows.h. I also removed the 3 line call to the macro sense it is now a function.

I think this is another case of macros making C++ code less intuitive. No reason not to just use a function here.

Quote
In op2ext.h and op2ext.cpp, for method GetGameDir_s, consider declaring bufferSize as size_t rather than unsigned int.

Fixed.

Quote
In op2ext.h and op2ext.cpp, method SetSerialNumber, perhaps name the parameters more descriptively, such as "major", "minor", and "patch".


Fixed.

Quote
Do we need IpDropdown.h?

IpDropDown.h shows which functions from the .cpp file are meant for public exposure to the rest of the program. The functions declared only in the .cpp file are meant for internal to IpDropDown use. This seems like a pretty useful convention to me. Additionally, I thought separating code into .h/.cpp files in general is supposed to speed compilation. Could you explain why you want to get rid of the .h file?

Quote
In IniModuleLoader.h, was there a reason for using std::list as opposed to std::vector for moduleList? It should be the same length as the std::vector returned by GetModuleNames.

Looking over the code, I didn't see any benefit treating the modules as a std::list over a std::vector (not that there is really anything wrong with it being a std::list). I changed it over to a vector.

Quote
I'm uncertain if the typedefs for the module function exports make the most sense in the header file or the implementation file. Thoughts?

The DestroyModFunc typedef is used in the struct IniModuleEntry which is declared in the header file. IniModuleEntry is used by a couple of functions and must remain in the header file. Only the InitModFunc could be moved to the .cpp file.

Quote
IniModuleLoader::CallModuleInitialization has a memory leak.

Good point. Memory leak fixed and strcpy function removed. Thanks for finding the memory leak!

Quote
Does CallModuleInitialization need to be it's own method? I'm thinking once the string copy code is removed, it might make sense to just inline it. That would also feel more consistent with how the DestroyMod method is looked up.

In my opinion, it is better to keep the implementation details involved in how the function is called separated out from the LoadModules code. I created a new CallModuleDestruction function to remove the implementation details of module destruction from UnloadModules.

Quote
What's the point of the return value on IniModuleLoader::UnloadModules? For that matter, what is the point of the return value on DestroyMod? This value doesn't seem very actionable.

Quote
Should LoadModuleDll be responsible for displaying the error message? It's convenient, because that's where information is available, but something about it feels a bit odd. A cleaner solution might be to format and throw an exception, and have the caller display a dialog box or otherwise handle or log the error.

What do you mean by caller in this case? op2ext as a whole or the module's DLL? I think it is appropriate for the loader to create the dialog box.

I think I would be a fan of us creating a logger for op2ext. It could report on any errors when using the modules. This could give a purpose to the DestroyMod function bool return. Logging the date/time that a module was unable to properly destroy/unload itself. However, I'm also not sure this is a project I want to tackle right now.

So, guess I'm happy leaving status quo for now...

Quote
Inconsistency: IniModuleLoader uses HINSTANCE, while ConsoleModuleLoader uses HMODULE.

Codebase updated to use HInstance only for standardization.

Thanks,

Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 13, 2018, 08:13:05 AM
Quote
It was required in VolList.cpp in order to call the macro DBG. Otherwise, I would get the following error:

Error   C3861   'OutputDebugString': identifier not found

That's about what I expected.

Quote
I changed the macro out for a function named OutputDebug. This way Windows.h could be included in GlobalDefines.cpp and cover the function's use of the Windows header instead of forcing all calling files to include Windows.h. I also removed the 3 line call to the macro sense it is now a function.

I think this is another case of macros making C++ code less intuitive. No reason not to just use a function here.

That's a really good way of solving the problem. Doesn't clutter up a global header with a large include. Also results in very nice syntax at the call point.

One difference I want to point out, the macro only expanded when DEBUG was set, otherwise it was silently swallowed by the preprocessor. This resulted in no call overhead, nor argument computation in release mode. I don't think we need to replicate that behaviour though. I think it makes more sense to have debug output available even in release mode builds. It may even make more sense to log that information now that it is dynamic.

Quote
Quote
In op2ext.h and op2ext.cpp, for method GetGameDir_s, consider declaring bufferSize as size_t rather than unsigned int.

Fixed.

I just noticed size_t is declared in the <stddef.h> header. I added an include for this. There is also a <cstddef> header, which declares std::size_t. We want to stick to using a C interface for the publicly exported stuff though.

We should consider adding a .c test file, so the compiler can warn if we use C++ features in op2ext.h.

Quote
IpDropDown.h shows which functions from the .cpp file are meant for public exposure to the rest of the program. The functions declared only in the .cpp file are meant for internal to IpDropDown use. This seems like a pretty useful convention to me. Additionally, I thought separating code into .h/.cpp files in general is supposed to speed compilation. Could you explain why you want to get rid of the .h file?

The contents of IpDropDown.h consists of:
Code: [Select]
void InstallIpDropDown();

We could just replace the #include line with that contents of the file. It's only used in DllMain.cpp, which already contains plenty of other forward declares. Though I do accept the reason of convention and documentation.

Quote
The DestroyModFunc typedef is used in the struct IniModuleEntry which is declared in the header file. IniModuleEntry is used by a couple of functions and must remain in the header file. Only the InitModFunc could be moved to the .cpp file.

Ahh, I see it now. Good point. Yeah, keep them together then for consistency.

Quote
In my opinion, it is better to keep the implementation details involved in how the function is called separated out from the LoadModules code. I created a new CallModuleDestruction function to remove the implementation details of module destruction from UnloadModules.

Hmm, I'm looking at this now and wondering why we cache the DestroyMod function. We already have a handle to the module, which is kept to be passed to FreeLibrary later. The function is only called once. It can simply be looked up when it is needed, similar to the InitMod function. That would reduce the amount of data being stored in memory at runtime. I like the idea of being able to cache values alongside the module handle, so maybe preserve the struct, it may be useful if we extend the module system later.

I'm still on the fence about this part of the code, but I think it's actually more because of the inconsistency between InitMod and DestroyMod.

Quote
Quote
Should LoadModuleDll be responsible for displaying the error message? It's convenient, because that's where information is available, but something about it feels a bit odd. A cleaner solution might be to format and throw an exception, and have the caller display a dialog box or otherwise handle or log the error.

What do you mean by caller in this case? op2ext as a whole or the module's DLL? I think it is appropriate for the loader to create the dialog box.

Yes, op2ext should display that dialog box. I think it would be better display at the point of call, in the LoadModules function. The LoadModuleDll function could format the dllName into a string, and throw that as a runtime_error, which LoadModules could catch, and display in the contents of a dialog box. That separates out error detection from error reporting. It gives the caller more flexibility in how errors are reported, or if they are collated before reporting.

I see MaxVolFileCountReached was cleaned up. It's now a proper boolean function. I still find the name a bit wordy and unclear though. Perhaps IsFull?
Code: [Select]
if (!volList.IsFull())
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 15, 2018, 11:24:07 AM
I'm limited on time to work on this, so sorry for the delays in getting back. After looking into throwing runtime_errors for missing DLLs, I'd like to unroll the actual final version of op2ext for this release. So this is sort of coming to a last chance to write on changes before I'm done working.


Quote
Yes, op2ext should display that dialog box. I think it would be better display at the point of call, in the LoadModules function. The LoadModuleDll function could format the dllName into a string, and throw that as a runtime_error, which LoadModules could catch, and display in the contents of a dialog box.

I'll look at implementing this shortly. Shouldn't be too difficult.

Quote
I see MaxVolFileCountReached was cleaned up. It's now a proper boolean function. I still find the name a bit wordy and unclear though. Perhaps IsFull?

Changed, good suggestion for the name function's name.

Quote
I just noticed size_t is declared in the <stddef.h> header. I added an include for this. There is also a <cstddef> header, which declares std::size_t. We want to stick to using a C interface for the publicly exported stuff though.

We should consider adding a .c test file, so the compiler can warn if we use C++ features in op2ext.h.

I've never had to include a header to use size_t. Probably because it is always getting included through Windows.h or vector or other headers. Thanks for fixing. Nice to have to pull changes for once. :)

Including a C Test module seems like a good idea. I do not want to repeat all the tests in the current TestModule in both C++ and C code though. Perhaps the correct thing to do would be to rewrite the C++ test module as a C TestModule. I have not written a lick of C code beyond working on these C ABI interfaces. So, for myself to do the work would probably take a lot longer than expected as I muddled through it, which again is something I'm looking to avoid.

From my very brief googling on the subject, it is generally easier to port C code to other languages than C++. For example, it looks like there is a module within Python that allow creating an interface between Python and C code using CMake, but not between Python and C++ code. So in theory, one could use CMake to code a module for Outpost 2 in Python and call/send the correct functions to interface with op2ext.

I'm not advocating writing a module in Python or anything, I'm just saying that if we are intending to use the C ABI for our external interface, it should probably be tested in C code to ensure full compatibility. Even if I'm not doing the work in the short term.

Thanks,

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 16, 2018, 10:20:02 AM
Quote
The LoadModuleDll function could format the dllName into a string, and throw that as a runtime_error, which LoadModules could catch, and display in the contents of a dialog box.

Done.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 17, 2018, 04:18:06 AM
It's very likely size_t was included indirectly through another header file. That's a common issue with #include, and how it basically does a public import, so you end up with everything recursively included in your namespace.


Quote
Including a C Test module seems like a good idea. I do not want to repeat all the tests in the current TestModule in both C++ and C code though. Perhaps the correct thing to do would be to rewrite the C++ test module as a C TestModule. I have not written a lick of C code beyond working on these C ABI interfaces. So, for myself to do the work would probably take a lot longer than expected as I muddled through it, which again is something I'm looking to avoid.

At bare minimum, have an empty .c file which includes the header, and make sure the compiler doesn't throw any errors. By changing the extension, hopefully the compiler is smart enough to compile it in C mode, and throw errors for C++ specific features. I don't think we need to test calling in C mode, or rewrite any tests, just make sure the interface passes basic parsing in C mode (and that C++ features don't pass parsing).



I tried this quickly from Linux (the lazy way, without a .c file):
Code: [Select]
i686-w64-mingw32-gcc -DOP2EXT_INTERNAL_BUILD op2ext.h

I quickly discovered a few small issues.

One error being that extern "C" is not valid C code. I moved that to it's own #ifdef __cplusplus block that wraps the whole header. That seems to be a common practice for designing a header that is used from both C and C++.

Another issue is the use of C++ type bool as the return type for GetGameDir_s. We should consider changing the return type, with int being the most logical candidate. That could still hold a true/false result, or it might also make sense to return the required size if the supplied buffer is too small.

The C compiler also objected to the deprecated part added to GetGameDir. I don't have a suggested workaround for that. (Edit: Using __declspec(deprecated("Message")) is compatible with C.)



I also tested on Linux with a C++ compiler (using an alias for the Mingw compiler name):
Code: [Select]
mingw -std=c++11 -DOP2EXT_INTERNAL_BUILD *.cpp

It doesn't build due to some platform differences, though the output looks mostly clean.

There are some errors about <filesystem>. That's not surprising.

It picks up an error in BOOL EnableWindowNew(HWND, BOOL) in IpDropdown:29:
Code: [Select]
error: too few arguments to function Ďerrno_t _itoa_s(int, char*, size_t, int)í
   _itoa_s(i, tmpStr, 10);
...
/usr/share/mingw-w64/include/sec_api/stdlib_s.h:18:27: note: declared here
   _CRTIMP errno_t __cdecl _itoa_s(int _Value,char *_DstBuf,size_t _Size,int _Ra

That last one surprised me a little. Might be worth taking a quick look at, just to make sure the code is really doing what we think it's doing. Or it might be the method is non-standard, and has different signatures and implementations between platforms, which is not really such an issue, since we only expect to build this from Windows.

The Microsoft documentation on _itoa_s (https://msdn.microsoft.com/en-us/library/0we9x30h.aspx) suggests this form is being used:
Code: [Select]
template <size_t size>  
errno_t _itoa_s( 
   int value, 
   char (&buffer)[size], 
   int radix   
); // C++ only

C++ Reference - itoa (http://www.cplusplus.com/reference/cstdlib/itoa/) says the method is non-standard, but suggests it is common. It suggests using sprintf as an alternative.

I replaced it with the following, which will be consistent with other uses of sprintf-like calls in the code.
Code: [Select]
_snprintf_s(tmpStr, sizeof(tmpStr), "%d", i);
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 19, 2018, 03:38:17 PM
Hooman,

I had to update the StubExt variable in op2ext.cpp to work with your modifications of op2ext.h. Easy fix though.

I'm glad we took the time to move all the "C" code to a single isolated header. It made your changes trivial to make it compatible with both C++ and C.

While testing the StubExt, I found a small error in the project settings for the TestModule when compiled in RELEASE mode. Not a big deal since it is a test case anyways, but fixed.

Quote
Another issue is the use of C++ type bool as the return type for GetGameDir_s. We should consider changing the return type, with int being the most logical candidate. That could still hold a true/false result, or it might also make sense to return the required size if the supplied buffer is too small.

Returns an int now.

I sort of like returning the required size, but it gets a bit weird. I would prefer to return 0 if the size is large enough, but this would look like returning FALSE, which is confusing. If I always return the size of the buffer used, you would have to test the returned int to make sure it is smaller than the supplied buffer, which is awkward as well. So, probably best to just return the INT acting like a C++ bool value and let the user guess how much bigger to make their buffer.



Test C file added to codebase. Only included in the project on DEBUG builds. I tested that it is compiling in C with the bool value. The compiler complained about it until changing GetGameDir_s was changed to int return.



I'm done contributing for this release but will continue to compile on Windows and test until everyone else is finished as well. Mostly because Hooman is running Linux which cannot compile code for Windows using the filesystem namespace yet.

I will be waiting on the final version of op2ext to rerrange the vol files and push the newer map and tech tree files as this relies on op2ext being able to load vol files with any names.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 19, 2018, 07:02:02 PM
Good catch with StubExt. That's very easy for me to miss with my setup.

Perhaps move the test .c file out of the project and into the test module. It is essentially just test code anyway, not something that actually gets compiled into the project. Also, either way, there is no benefit from the DEBUG restriction, since no code is emitted anyway.

Quote
I sort of like returning the required size, but it gets a bit weird. I would prefer to return 0 if the size is large enough, but this would look like returning FALSE, which is confusing. If I always return the size of the buffer used, you would have to test the returned int to make sure it is smaller than the supplied buffer, which is awkward as well. So, probably best to just return the INT acting like a C++ bool value and let the user guess how much bigger to make their buffer.

I was also thinking return 0 if the buffer is large enough. That's much easier to check against. I don't think it's odd, or confusing. The function name doesn't imply any sort of semantics for the return value. Even if you assumed 0 meant FALSE, does that mean success or failure? I'd say, given the function name, you'd need to check documentation to know how to interpret the return value.

It'd be nice to make a proactive decision on this. Once the new op2ext is packaged with a new game release, breaking changes become a much bigger concern.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 20, 2018, 04:40:36 PM
Quote
Even if you assumed 0 meant FALSE, does that mean success or failure?

If a function named something like GetGameDir_s returned false, it would be unintuitive to me if it didn't mean problem/failure. I would not check documentation on the bool unless it didn't perform as expected. I'm not experienced enough with C coding to know if the user would just assume int here really meant bool, although I might see myself falling into this logic?

How about size_t GetGameDir_s(char* buffer, size_t bufferSize)?

Seeing size_t would indicate to me a range value and not a bool. It would also make more sense comparing it against the passed size_t bufferSize.

Moving the test c code to the testModule is a nice call. This would render the DEBUG guard useless and allow testing in both DEBUG and RELEASE mode.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: leeor_net on January 20, 2018, 06:55:39 PM
In C generally speaking a function returning an int other than 0 indicates failure. But, this is also subjective and dependent on API (though I can't think of an API I've used in C or C++ that indicates failure via 0).

Often times for 'boolean' values preprocessor definitions are used (or in the case with my Quake 2 based project an enumeration is used) where false == 0 and true == anything else.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 20, 2018, 09:53:03 PM
Quote
size_t GetGameDir_s(char* buffer, size_t bufferSize)

Yes, that would actually make the most sense. If it returns a required buffer size, it should be size_t.

Quote
In C generally speaking a function returning an int other than 0 indicates failure.

I would agree that is often the case. Many functions return 0 on success, and an error code on failure. That allows many possible error cases to be distinguished.

An example is program error codes returned to the shell (the value returned from main). Usually 0 is returned upon successful completion, while a non-zero value is assumed to be an error code. A lot of command line processing tools will terminate batch processing when they encounter a non-zero result, based on the assumption that indicates an error.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Vagabond on January 21, 2018, 04:06:48 PM
Out of the infinite numbers that mean TRUE by default, we have collectively chosen the only number that means FALSE to indicate TRUE (more specifically that something occurred successfully/without error). Nice.

Guess this is showing my lack of C/C++ experience and lower system level design.

I'll plan to make the GetGameDir_s and C test code location changes Monday or Tuesday and then make a release candidate compilation. This op2ext RC will be pushed into the SVN Outpost2 copy. I'll wait to tag the op2ext 2.0.0 release until after a final round of testing once all other changes are made in case anything unfavorable is discovered. If everything is good, the final RC will just become 2.0.0.

-Brett
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: leeor_net on January 21, 2018, 06:23:51 PM
Out of the infinite numbers that mean TRUE by default, we have collectively chosen the only number that means FALSE to indicate TRUE (more specifically that something occurred successfully/without error). Nice.

Welcome to the C branch of languages where nothing makes sense and you can trash the stack and bring an operating system to its knees just by typo-ing.

Anyway, a better way to look at it with return values is 0 indicates nothing out of the ordinary and anything else is some sort of status code vs. true/false. But we may be beating a dead horse now -- it's why I usually opt to indicate error codes via named enumarations vs. int values.
Title: Re: Allowing Outpost2 to load any vol file (*.vol)
Post by: Hooman on January 24, 2018, 12:52:11 PM
Returns bool failed, which could be true in many possible ways ;)

Having a return value of true to indicate success is just as arbitrary as equating 0 with false. It's all just a matter of convention and practicality.

Btw, the convention in Bash is to actually reverse the values of true and false. Try running the true and false commands (actual executable files), and checking their result code (return value from main) with $?:
Code: [Select]
true; echo $?   # => 0
false; echo $?  # => 1

This stems from having typical program return values indicate an error code, while wanting to preserve the notion of using the return value as meaning bool success. You have one sentinel value to indicate one response, while all other values indicate the other response.