Allowing Outpost2 to load any vol file (*.vol)

[Vagabond]

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.

[Hooman]

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.

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.

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.

[Vagabond]

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

[Hooman]

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.

[Vagabond]

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

[Hooman]

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.

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. 


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.

[Vagabond]

Quoting myself:

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.

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

[Arklon]

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. 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).

[Vagabond]

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

[Arklon]

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.

[Hooman]

/* 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.

[Arklon]

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.

* 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 is a decent free one.

[Vagabond]

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.

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.

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

[Hooman]

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.

[Arklon]

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.

[Vagabond]

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

[Hooman]

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.

[Arklon]

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.

[Hooman]

...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.

[Vagabond]

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

[Hooman]

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?

[Vagabond]

Thanks for the proofreading.

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

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

Completely agree.

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.

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

[Vagabond]

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

[Hooman]

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.

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.

[Vagabond]

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.

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.

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