Author Topic: Allowing Outpost2 to load any vol file (*.vol)  (Read 52339 times)

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Allowing Outpost2 to load any vol file (*.vol)
« on: November 05, 2017, 01: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

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1269
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #1 on: November 05, 2017, 12:20:34 PM »
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.
« Last Edit: November 05, 2017, 12:26:22 PM by Arklon »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #2 on: November 06, 2017, 01: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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #3 on: November 07, 2017, 01: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
« Last Edit: November 07, 2017, 01:50:16 AM by Vagabond »

Offline Sirbomber

  • Hero Member
  • *****
  • Posts: 3238
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #4 on: November 07, 2017, 04: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.
"As usual, colonist opinion is split between those who think the plague is a good idea, and those who are dying from it." - Outpost Evening Star

Outpost 2 Coding 101 Tutorials

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #5 on: November 07, 2017, 10: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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #6 on: November 08, 2017, 08: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

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #7 on: November 12, 2017, 12:36:47 AM »
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

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #8 on: November 13, 2017, 12:30:06 AM »
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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #9 on: November 13, 2017, 01: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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #10 on: November 13, 2017, 03: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).
« Last Edit: November 13, 2017, 10:46:43 PM by Vagabond »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #11 on: November 14, 2017, 12:20:13 AM »
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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #12 on: November 14, 2017, 02: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 )
}

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #13 on: November 14, 2017, 11: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.
« Last Edit: November 14, 2017, 11:56:31 PM by Hooman »

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #14 on: November 15, 2017, 03: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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #15 on: November 15, 2017, 03: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.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #16 on: November 15, 2017, 01: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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #17 on: November 15, 2017, 09: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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #18 on: November 16, 2017, 11: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



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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #19 on: November 17, 2017, 01: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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4955
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #20 on: November 17, 2017, 03: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.)

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #21 on: November 17, 2017, 09: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.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #22 on: November 19, 2017, 01: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
« Last Edit: November 20, 2017, 12:45:05 AM by Vagabond »

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1269
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #23 on: November 20, 2017, 04: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 is a decent free one.
« Last Edit: November 20, 2017, 07:18:44 PM by Arklon »

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1015
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #24 on: November 20, 2017, 09: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