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:
||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).
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:
#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.
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.
__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.
#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 )
}
#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.
#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.
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.
std::string mapsString("maps.vol");
vols.AddItem(mapsString.c_str());
This line of code will allow the program to properly load the vol file.
char* mapsChar = "maps.vol"
vols.AddItem(mapsChar);
Additionally, this line will cause the code to fail just like the std::string.c_str() did.
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?
// 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.
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
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.
volPaths.push_back(volPath);
InitializeVolSearchEntry(&volPath[0]);
You need to update that second line to read from the vector. The entries in the volPaths vector will be copy initialized from the volPath parameter (by the first line), so they'll have independent lifetimes. Try using something like volPaths[lastEntry].c_str().
Go ahead and use #pragma once. It's much cleaner looking. The main objection to it was the syntax is not standard, however, it's use is now so widespread that you don't really need to worry about it. Previously, back in my day *leans on cane and whistles through teeth* it wirrrked on Wiiiirndows with MSssssVCeeeerr, but not on Linux with GCC, hence it used to cause headaches with cross platform compilation. It's now supported by GCC though, and most other major compilers:
https://en.wikipedia.org/wiki/Pragma_once (https://en.wikipedia.org/wiki/Pragma_once)
You can drop the extern part if you're not using the "C" part. Using extern on it's own is still useful forward declares (without including header files), but that's not being used in this project.
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!
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?
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
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:
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.
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.)
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.
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:
/// <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).
/**
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...
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.
Quote from: Hooman on Today at 04:39:40 AM
I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.
I agree, but I when I was talking about him supposedly committing it to the SVN, I was talking about a year ago before we had the ball rolling on GitHub.
After being fairly grumpy about having to merge the 2 different versions, it actually wasn't that difficult using KDiff and relying on all the time I've already put into op2ext. I learned a fair amount about how to use KDiff for comparing changes between any file, not just in the strict confines of TortoiseHG, so it was actually pretty useful to do. (However I have a compile issue below, so I'm hoping it is easy to solve and I don't eat crow on this paragraph...)
Inside of op2ext.cpp, we have the following class declaration:
class __declspec(dllimport) TApp
{
public:
int Init();
void ShutDown();
};
However, the member functions Init and Shutdown do not appear to be defined within op2ext code base (unless I missed it). I'm getting LNK2019 Linker Tools Error for both functions (https://docs.microsoft.com/en-us/cpp/error-messages/tool-errors/linker-tools-error-lnk2019) on compilation.
Error LNK2019 unresolved external symbol "__declspec(dllimport) public: int __thiscall TApp::Init(void)" (__imp_?Init@TApp@@QAEHXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
Error LNK2019 unresolved external symbol "__declspec(dllimport) public: void __thiscall TApp::ShutDown(void)" (__imp_?ShutDown@TApp@@QAEXXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
I'm not sure if maybe a reference is missing in the GitHub copy of op2ext or what? If we can solve this, I can finish the compilation and start testing the merge.
Current code is commited to the REPO as the branch Mod_initialization_and_cleanup_function_hooks. https://github.com/OutpostUniverse/op2ext/tree/Mod_initialization_and_cleanup_function_hooks. (I couldn't get it to cooperate with spaces for some reason so used underscores).
Hooman, regarding documentation. Check out the two attached screenshots. Basically Visual Studio creates some metadata (in XML form I think) that allows specific comments to only show up for each argument into a function as you are typing it. If we go with something else like Doxygen, then comments on arguments and return values will just be included in the body of the intellisense tooltip. So, you are not missing any data not using the Microsoft way.
In fact I like having the argument comments in the body of the tooltip instead of separated out, that way I don't have to start typing out the function and arguments just to read the comments associated with the arguments, so wouldn't mind skipping the Microsoft specific comments.
I'm not looking to document all the functions this formally, just if it isn't obvious what is going on, or the public functions available to other DLLS.
The SetSerialNumber function changes the version string ("1.3.0.4") in the .data section of Outpost2.exe. I just checked the address it was using. I believe there were a few different version strings used in different places, so I'm not sure what exactly it affects.
Is SetSerialNumber where it checks to see if versions of the game are compatible before initiating a multiplayer game?
I'd like to get all the different version strings standardized for sanity purposes. SetSerialNumber doesn't affect the serial number that I'm trying to clean out of the settings area of in game play, so that must exist outside of op2ext.
Thanks,
-Brett
Hooman, I agree this thread is starting to blow up into a lot more than originally intended. (Especially you bringing up combining Outpost2DLL and Outpost2App :-) )
I’d like to start winding down the actual work on op2ext soon. Here is sort of a list remaining.
CURRENT PLAN (subject to change of course…)
- Remove circular relationship of op2ext.h with sub components of op2ext.
- Reduce size of LoadIniMods function (about 50 lines of code right now) and generally clean LoadIniMods.cpp
- Make IpDropDown.h/.cpp a class.
- Look at updating IpDropDown.h/.cpp to using std::string instead of char*
- Include in ReadMe.txt a section on recommended use of SetSerialNumber for mods involving multiplayer maps that will not interfere with incrementing the main game’s version number over time.
- Include more details in ReadMe.txt on how to load modules using op2ext.
- Include details in ReadMe.txt on available features in op2ext for developing and hooking modules into Outpost 2
- Write a post build script that auto-zips the ReadMe.txt, dll, and PDB file for posting on GitHub.
- Set Outpost2DLL project as a submodule once it exists on GitHub.
I went ahead and merged the branch Mod_initialization_and_cleanup_function_hooks with the master branch. It contains Outpost2DLL.lib. Once Hooman gets Outpost2DLL transferred to GitHub, we can go back and add a proper submodule and delete the Outpost2DLL.lib file from op2ext.
SetSerialNumber Function
I played with the function SetSerialNumber. When joining a multiplayer match, if the host and joining player do not have the same serial number set, a message is given to both parties saying what each serial number is and that they are incompatible, kicking the player from joining the session. This is actually really great feature. See attached screenshots.
SetSerialNumber had a bug in it where the wrong digits were being set. Fix has already been pushed.
I’m getting the impression this is not being set as new versions are being rolled out. The current version in the repo reads 1.3.4. I’d like to set the Serial Number to 1.3.7 since we are pushing a new version of the multitek tech tree with this release. I’ll post about it in the overall 1.3.7 I made to discuss https://forum.outpost2.net/index.php/topic,6043.0.html since it isn’t related to vol files..
op2ext public.h
How exactly is public.h used or supposed to be used? I noticed it isn’t actually a member of the op2ext project. Also, it doesn’t look like it is #include in either NetHelper or NetFix.
// OP2Ext.dll preliminary interface. May be subject to change!
#define OP2EXT_API extern "C" __declspec(dllimport)
// Returns a pointer to a buffer of [MAX_PATH+1] length to retrieve the parameter to
// /loadmod that the game was invoked with.
// You should free() the buffer when you are done with it.
OP2EXT_API char* GetCurrentModDir();
// Pass a buffer of [MAX_PATH+1] length to retrieve the game directory
// without trailing slash.
OP2EXT_API void GetGameDir(char *buffer);
// Pass null terminated string to add a VOL file to the search list.
OP2EXT_API void AddVolToList(char *volName);
// Pass 3 digits, 0-9 each, to set a unique ID for this specific version of the mod.
// The OP2 version string will be rewritten to prevent other versions or other mods
// from joining the game.
// These should be numeric values and NOT the ASCII characters '0', '1', so on!
OP2EXT_API void SetSerialNumber(char major, char minor, char revision);
Also, this might be a really good point to learn about rewriting Git history.
Rewriting history would likely stall this project for good, so no thank you. :)
It probably makes the most sense to name the public export header after the project itself: op2ext.h. When people try to use the library, that's what they'll try importing. An internal master header file should probably be named something else. Though if the headers are split and organized nicely, there might not be a master header file.
I've moved enough code out of op2ext.h/.cpp, that something like this is now possible, but there are some big hurdles still.
Minus the includes, op2ext.h now reads as follows
extern VolList vols;
extern IniModuleLoader iniModuleLoader;
// NLS for OP2
//void LocalizeStrings();
void ConvLangStr(char *instr, char *outstr);
void LocateVolFiles(std::string relativeDirectory = "");
EXPORT void AddVolToList(char *volFilename);
EXPORT void SetSerialNumber(char num1, char num2, char num3);
// Prototype for patch to LoadLibrary, where it loads OP2Shell.dll
HINSTANCE __stdcall LoadLibraryNew(LPCTSTR lpLibFileName);
What is the line below for? It related with localization, but I don't know what NLS is and the comment is pretty vague. I'd like to at least improve the comment so it is more clear.
// NLS for OP2
//void LocalizeStrings();
void ConvLangStr(char *instr, char *outstr);
I'm having trouble understanding what is going on with LoadLibraryNew:
// Prototype for patch to LoadLibrary, where it loads OP2Shell.dll
HINSTANCE __stdcall LoadLibraryNew(LPCTSTR lpLibFileName);
It says it is a prototype, but we are actually using it. I think we just need to remove the word prototype and maybe expand the comment a little more to say what is going on?
Moving public.h to op2ext.h...
I'm thinking that we create a new file called dllMain.cpp or main.cpp that handles actually running the op2ext dll. Then we have op2ext.h/.cpp for use as the public export header.
I can move the declarations of IniModuleLoader, LocateVolFiles, and LoadLibraryNew into dllMain.cpp since no other component cares about them.
The EXPORT function AddVolToList(char *volFilename) requires access to the VolList class. I think we can extern declare VolLists so the body of the function AddVolToList can access VolLists. Not sure, would need to test...
GetCurrentModDir is a loose function in ModMgr.cpp. However, it is not required for use of ModMgr. It is just exists to allow others access to the Module directory. So, we could probably move the definition to op2ext.cpp and try another extern declare for ModMgr. This function is also possibly poorly written. It instantiates a new version of ModMgr every time it is called, which is wasteful at best. Perhaps with the extern call, this constant instantiation could be removed. (fixed now)
GetGameDir now lives in FileSystemHelper. This is our problem function for backwards compatibility. It actually calls FileSystemHelper's function GetGameDirectory and then copies the resulting std::string to the char* buffer that has no size passed alongside it. Again, I could look at maybe calling extern on GetGameDirectory, so the defintion of GetGameDir can live in op2ext.cpp and call it.
Umm, okay, I'm having trouble keeping up with what I'm talking about, so it is probably pretty confusing to read. Perhaps this is getting beyond my current ability/knowledge of C++? I can start experimenting and moving in this direction, but I didn't want to procede down the wrong rabbit hole.
Rewrite of IniMods
Additionally, I finished up a rewrite of IniMods. It is now named IniModuleLoader. (I'm thining about switching this to IniModuleManager since it also unloads the mods). It is now a class and hides all variables and functions as private except
- void LoadModules();
- bool UnloadModules();
I wrote a function that checks if the char[] returned from fetching data from the ini file is full. If it is full, it re-fetches the data with a larger char[] until it isn't full. I don't think any of the data sections in the .ini file will be greater than 1024 characters in the near future, but it is also nice not to worry about it.
Summary of commits:
* Minor cleanup of code in IpDropDown.cpp.
* Refactor IniMods.cpp into the class IniModuleLoader
- Now supports std::string instead of char*
- Reduced size of function LoadIniMods
- Renamed variables in attempt to be more specific in their purpose
Code tested and it compiles and loads mods. Since this is an extensive rewrite, will need further testing for different error conditions like missing DLL, etc. Plan to review some more changes, test, then request a code review on my work since this is a critical piece of op2ext.
* Make GetPrivateProfileStdString dynamically resize buffer as required
- Allows returning strings from Outpost2.ini that are larger than 1024 characters.
* Attempting to remove code from op2ext.cpp that is used throughout the application
- Created a FileSystemHelper.h/.cpp and GlobalDefines.h/.cpp to facilitate.
Not necessarily happy with the name GlobalDefines though.
-Brett
-----------------------------
Edit:
I forgot to mention earlier but I've noticed that you can only include 1 mod using the command line (in addition to all the mods in the .ini file). Some references in the source code allude to loading multiple modules through the command line. Unless we specifically want to work towards supporting this, I'd like to edit the source to be more clear that only one mod is being loaded via command line.
I don't know anything about the NLS or Localization stuff. BlackBox would be a better source of info on that.
For LoadLibraryNew, I believe "prototype" is being used in the sense of "function declaration" (as opposed to function definition). I'd have to look at the code to know what it's doing, though it's pretty common practice to hook LoadLibrary when patching stuff in memory and writing extension modules. It's the earliest and most reliable point at which you can hook and control a dynamically loaded library.
Thanks. It didn’t occur to me that prototype means function declaration. All the localization references are commented out, so I’ll just document a bit more and leave it where it is in case it gets picked up on again later.
This strikes me more as a limitation of the user interface and parsing of the command line, rather than a backend issue. The backend might very well support loading multiple mods. In particular, the INI mod loader does this. You'd just need a way of specifying multiple modules at the command line. Maybe the option can take a list, or maybe the option can be specified multiple times.
Yes, this is what I’m getting at. It wouldn’t be difficult to expand to accept multiple mods as a list. The real question is how many mods do we want to allow loading through the command line? There is a lot of potential for making a mess by combining them. Besides the obvious mod 1 and mod 2 both touch the same piece of the game, I was thinking about multiplayer sync.
What if someone rewrites the color mod and assigns it to 0.0.1 and then someone makes a multitek2 multiplayer scenario and assigns it 0.0.2. Both mods may be compatible, but which SerialNumber do they use when both are loaded? I think the right answer is that a third mod is created, say multitek2Color that takes 0.0.3. It can then deal with integrating them both as needed.
We can warn devs in the op2ext ReadMe of these problems. We can also limit to one mod to keep it less likely that conflicts arrive. Some mods may be pretty safe to combine if they affect different parts of the game and don’t really create a multiplayer sync issue.
All that being said, I’m okay punting multiple command line mods until a different release even if we want it (unless it is a big deal to support for someone in the near future). I can go in and clean the code some more so it is clear that only one mod is getting loaded. From there it should not be too difficult to expand as needed. (I did rewrite the code to use std::string among other things, but the original code also only loaded one mod.)
I had a few concerns come to mind recently. While converting the OP2Helper project, I found I was able to run the source through the MinGW compiler, but it wouldn't link due to differences in name mangling. This got me thinking, there is a reason the interface for op2ext uses C rather than C++, the C ABI is highly portable, and allows modules to be written in a variety of languages using a variety of compilers. The C++ ABI is much less portable, and unlikely to interface well with other langues, and possibly not even with other C++ compilers.
I’ve actually been thinking about this as well. I noticed that FreeImage’s public interface only uses the C ABI, probably for similar reasons as you brought up. Currently all of the EXPORT functions remain C functions, except for the new GetGameDirectory. I’d like to remove the dllexport from this function. Then we can create a new version of the function that uses the C ABI for export, taking into account input for the buffer size.
In regards to the EXPORT functions, if we eliminate public.h, and declare the EXPORT functions in op2ext.h instead, would this be considered a breaking change? If so are we okay with this?
The other reason I’m not a fan of public.h is because we are declaring the functions in 2 separate places. I’m not sure if there is precedence for this, but it seems like a possible source of some insidious bugs.
If we want to remove public.h, I’m happy to work this, if not, then I’m happy to leave it alone for this release.
We may want to consider switching to a branching work model on this project, with pull requests for code review. Though that could be a painful transition when making extensive changes to so many areas of the project. A painful transition that will likely teach you about separating concerns and packaging commits nicely.
I would probably fair better working with branches and waiting on code reviews than you are giving me credit for. I’m fine with code reviews and branches, but it would require someone who can reliably compile the code be available to review the code on a regular basis.
Perhaps if we rework the filesystem code to something compatible with minGW, you could fill this role? Is anyone interested? Being hobby time for everyone, this might be too much of a commitment regularly?
I'm looking to wind down work on op2ext, so besides working on eliminating public.h (if we want to do that) I'm looking at moving into more cleaning and testing than expanding functionality and ripping apart.
I would like to create a sample module simply designed to test most functionality of op2ext, like the EXPORT functions and the different initialization and cleanup hooks. Maybe this should be the next pair programming session? I should be able to squeak this in on the Sunday, 17DEC if someone is interested. I'll post in Pair Programming about it in the near future.
IpDropDown
I am having difficulty forming IpDropDown into a class. Something to do with the code below. I don’t really understand what is happening as it looks like we are assigning functions to DWORDs (unsigned int) and then pushing them into Outpost 2’s executable’s memory. Anyways, I can get the class to compile, but the program would blow up when running.
BOOL __stdcall EnableWindowNew(HWND hWnd, BOOL bEnable)
{
. . . MORE CODE . . .
}
unsigned long __stdcall inet_addrNew(const char* cp)
{
. . . MORE CODE . . .
}
// Data constants for InstallIpDropDown
DWORD newEnableWindowAddr = (DWORD)EnableWindowNew;
DWORD newInetAddr = (DWORD)inet_addrNew;
DWORD* populateComboBoxAddr = (DWORD*)0x004197C1;
DWORD* saveIpTextAddr = (DWORD*)0x004C0E36;
void* nopDataAddr = (void*)0x0041988F;
void InstallIpDropDown()
{
// patch the call to EnableWindow so we can add strings.
Op2MemSetDword(populateComboBoxAddr, (int)&newEnableWindowAddr);
Op2MemSetDword(saveIpTextAddr, (int)&newInetAddr);
Op2MemSet(nopDataAddr, 0x90, 14);
}
All that being said, perhaps it would be best to just leave IpDropDown alone. As Hooman mentioned earlier, it would probably make more sense to make it a separate module that op2ext loads into Outpost 2 as opposed to including it in op2ext. Maybe a project for another release?
So, I’m looking at dropping further work on IpDropDown off the list.
Hooman, is the Outpost2DLL code on GitHub stable yet? If so, I’ll work on setting up a submodule.
I’ll place the submodule in ./Outpost2DLL. Unless we want some sort of different folder structure like ./submodules/Outpost2DLL. I’m happy with either setup.
Current task list
* Remove public.h and push declarations for EXPORT functions to op2ext.h (if this is what we want?)
* Include in ReadMe.txt a section on recommended use of SetSerialNumber for mods involving multiplayer maps that will not interfere with incrementing the main game’s version number over time.
* Include more details in ReadMe.txt on how to load modules using op2ext.
* Include details in ReadMe.txt on available features in op2ext for developing and hooking modules into Outpost 2
* Write a post build script that auto-zips the ReadMe.txt, dll, and PDB file for posting on GitHub.
* Set Outpost2DLL project as a submodule once it exists on GitHub.
-Brett
First, what is the benefit of making it a class?
It has several functions that should not be called by code external to ipDropDown.cpp
BOOL __stdcall EnableWindowNew(HWND hWnd, BOOL bEnable);
unsigned long __stdcall inet_addrNew(const char* cp);
void WriteAddressesToIniFile();
Okay, so this is only sort of true, because I think inet_addrNew and EnableWindowNew get injected into Outpost2's memory. I'm guessing they shouldn't be encapsulated. However, I inet_addrNew should be separated into several more private functions since it is almost 50 lines long and handles all the sorting when new IP addresses are noted.
Also, it has member variables that should only be updated by ipDropDown.cpp. In particular:
char ipStrings[10][47];
int numIpStrings = 0;
So, I'm not really sure the right way forward reconciling the fact that ipDropDown manipulates Outpost2's memory but also contains functions and variables that should be encapsulated. Again though, this can just be left alone for now too. It works and is fairly organized.
If we move the declaration of a function or a variable to the .cpp file, is this considered encapsulated? It might be a nice alternative. Since the rest of op2ext doesn't need to call WriteAddressesToIniFile, we could move its declaration to the .cpp file. Would this be a positive move?
This would not break binary compatibility. Existing compiled code would continue to run. This could break source code compatibility, if they are requiring public.h and the file disappears. Though that is an easy fix, updated the #include line, so in practice not a big concern.
In this case, I'd like to move the version to 2.0.0 on this release since it would be a breaking change for source code compatibility. I'd also like to leave some wiggle room in case someone ever wants to go back and assign version numbers to previous releases that I/we? don't really have enough documentation on right now.
I finished a rewrite of the ReadMe. Please let me know if there are any mistakes or if it is too detailed, not detailed enough, etc. I'd like to take a final pass reviewing it once everything else is completed anyways. It is in the repo, but below is a copy for ease of access.
Outpost 2 Extension Module
------------------------------------------
Outpost 2 extension modules (op2ext) is used to implement various extensions to the game Outpost 2. The most current version of Outpost 2 is maintained by the Outpost 2 community and may be found at https://www.outpost2.net/.
Authors: op2hacker (aka BlackBox)
Hooman
Arklon
Brett208 (Vagabond)
Summary
------------------------------------------
Outpost 2 Extension is a dynamic link (DLL) library written in C++. Source code may be found at https://github.com/OutpostUniverse/op2ext. OP2Ext compiles against x86 Windows. The current solution/project files use Visual Studio.
The following extensions are implemented for Outpost 2:
* Addon a single module using the command line argument /loadmod
- Allows for modifying the behavior of Outpost 2 by placing modified files in a folder and calling '/loadmod directoryName' when executing Outpost 2. For example, Outpost2.exe /loadmod multitek2.
- Designed for modules the user wants to include in Outpost 2 sometimes, but not on a semi-permanent basis.
* Addon modules using ini file
- Allows for adding modules to Outpost 2 without calling a command line switch.
- Allows setting properties for modules in the Outpost2.ini file.
- Designed for modules the user wants to include in Outpost 2 on a permanent or semi-permanent basis.
* Extra VOL file loading
- Allows loading any vol file contained in the root Outpost 2 directory and in the mod folder.
- Outpost 2 may only load a maximum of 31 vol files, including vol files loaded by modules.
* IP dropdown box for net play
- Allows saving previously used IP address in a drop down menu for easy selection when joining repeat matches.
Writing Custom Modules
------------------------------------------
op2ext.dll is included in releases of Outpost 2 on Outpost Universe. This allows modules to be written and integrated into anyone's copy of Outpost 2. Custom modules must be designed to hook into op2ext.dll through functions that export using the C Application Binary Interface (ABI). See modlibrary.c for a basic template.
Each new module for Outpost 2 should be placed in a separate directory that resides in the root directory of Outpost 2. The module should be designed to interface with Outpost 2 by either being added as an argument to the command line when opening Outpost 2 or by being specified in the Outpost2.ini file.
If the module contains configurable settings, history, or other data, and it is designed to be loaded through the Outpost2.ini file, it should store those settings in the Outpost2.ini file
Available functions for a module to export to op2ext.dll (see modlibrary.c of details):
- mod_init()
- mod_run()
- mod_destroy()
op2ext also provides the following functions for use by the custom module (see public.h for details):
- void GetCurrentModDir_s(char* buffer, int bufferSize);
- [DEPRECATED] char* GetCurrentModDir();
- void GetGameDir(char* buffer);
- void AddVolToList(char* volName);
- void SetSerialNumber(char major, char minor, char revision);
Module Serial Numbers
------------------------------------------
Outpost 2 contains an internal serial number that is used during multiplayer to ensure both the host's and guest's copies of Outpost 2 are compatible. This check occurs when the guest initially joins the match's chatroom. If the copies are not compatible, the guest will be immediately booted, and the 2 incompatible version numbers are listed for reference.
When an Outpost 2 module affects part of the game that could cause problems during multiplayer matches, the module must change the internal serial number with the function SetSerialNumber. This will prevent someone from joining the match that does not have the mod and possibly causing crashing bugs.
The serial number is represented by three single digit numbers (0-9). Serials 0.0.0 to 0.9.9 are reserved for new modules affecting multiplayer.
Using SetSerialNumber does not change the in game serial number listed in Outpost 2 under the about section.
Vol Files
------------------------------------------
Vol files are a custom Outpost 2 storage format used to store in game files like maps, tech trees, tile sets (wells), etc. Editing vol files requires custom software. See the Outpost 2 mapper or OP2Archive (https://github.com/OutpostUniverse/OP2Archive) for vol file manipulation.
Vol files must be loaded into Outpost 2 before the game initializes. A module should accomplish this by adding vol files to Outpost 2 within the function mod_init by calling AddVolToList.
Outpost 2 only supports 31 vol files. If loading more than 31 vol files is requested, op2ext will not load any vol file past 31 and will warn the user on which vol files were not loaded. Outpost 2 will continue to load, but depending on what data was contained in the discarded vol file(s), the program may or may not be able to function normally.
Change Log
------------------------------------------
Version 2.0.0
* Add ability to load vol files with any name into Outpost 2.
* Deprecated function GetGameDir(char* buffer).
* Allow retrieving data from Outpost2.ini greater in size than 1024 characters.
* Improve error messages when Outpost 2 has difficulty loading a module or vol files.
Version 1.0.0
* Several releases of op2ext were made prior to 2.0.0 which are not currently documented.
My work on Outpost 2 is currently getting crowded out with other priorities. I wouldn't expect more than some minor lifting on my part until some time in January. Sorry guys. This just took longer than I had available. (Probably because I kept working on random parts of op2ext not related to loading any vol file... but oh well).
-Brett
After saying there would be no time for major updates until January, I found some time. Life is funny like that. :)
First up, function declarations in IpDropDown not meant for public consumption were moved from IpDropDown.h to IpDropDown.cpp.
Full content of IpDropDown.h
#pragma once
void InstallIpDropDown();
Ok, now how do classes differ from namespaces?
(A question I didn't think to ask for many years)
Namespaces are a tool for preventing naming collisions and organizing code into logical sections. Classes are for grouping code with a similar responsibility into an object that has a clearly defined public interface and hidden internal workings. Well, that is the answer from my C# hobby perspective anyways... (I didn't look that up, just came out of my brain so it is probably a bit off).
The public interface has been moved from public.h to op2ext.h.
Additionally, I wrote bool GetGameDir_s(char* buffer, unsigned int bufferSize). It returns false if the provided buffer size is too small for the path, but still fills the buffer with as much of the path as possible. Looking for feedback if the bool return is a good addition or not...
op2ext.h's new contents
// Outpost 2 Extensions (op2ext) external interface.
// Exposes functions for use when applying modules to Outpost 2 via the C ABI.
// See ReadMe for specific use instructions. For help, post on the Outpost Universe Forum (https://www.outpost2.net/).
#pragma once
#define EXPORT extern "C" __declspec(dllexport)
// Retrieves the current absolute directory of the Outpost 2 executable with a trailing slash.
// If bufferSize is smaller than required to copy entire path, buffer is provided as much of path as possible and false is returned.
EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize);
// DEPRECATED as of version 2.0.0. Use GetGameDir_s instead.
// Retrieves the current absolute directory of the Outpost 2 executable with a trailing slash.
// @buffer Pass a buffer of size MAX_PATH length.
EXPORT||deprecated("GetGameDir was deprecated in op2ext ver2.0.0. Use GetGameDir_s instead.")||
void GetGameDir(char* buffer);
// Returns a pointer to a buffer of [MAX_PATH+1] representing the parameter(s) passed into Outpost 2 after the /loadmod argument.
// The consumer must free the buffer when finished with it.
EXPORT char* GetCurrentModDir();
// Allows loading a vol file into Outpost 2. volFilename must be passed before Outpost 2
// initializes by passing by calling this function from mod_init.
// @param volFilename The vol filename ending in a null terminated string.
// Must be a relative path from the directory containing the Outpost 2 executable.
EXPORT void AddVolToList(char* volFilename);
// Overwrites the default Outpost 2 version string.
// Required if the module affects multiplayer to detect incompatibilities between different copies of Outpost 2.
// See the ReadMe for detailed usage. Each variable must be a numeric value between 0-9 and not an ASCII character.
EXPORT void SetSerialNumber(char num1, char num2, char num3);
The one area I'm not sure about here is that I had to include 3 extern calls in op2ext.cpp to bring in the mod loader, and volList, and one bool variable. It doesn't seem too clean, but in order to give EXPORT functions access to the different functions like adding vol files, getting the game directory, and the mod directory, op2ext.cpp will need access to this code.
Also, can anyone shed light on what EXPORT int StubExt = 0 does? It is available externally, but only declared and defined in op2ext.cpp. It should probably be declared in op2ext.h if it is truly meant for consumption outside of the op2ext dll...
op2ext.cpp
. . . Includes . . .
extern ConsoleModuleLoader consoleModLoader;
extern VolList volList;
extern bool modStarting;
EXPORT int StubExt = 0;
EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize)
{
. . .
}
EXPORT void GetGameDir(char* buffer)
{
. . .
}
EXPORT char* GetCurrentModDir()
{
. . .
}
EXPORT void AddVolToList(char* volFilename)
{
. . .
}
char *verStrAddr = (char*)0x004E973C;
EXPORT void SetSerialNumber(char num1, char num2, char num3)
{
. . .
}
Listing of recent commits. Maybe I need to take what Hooman said to heart and try to reduce the number of commits that I make a little by combining them locally first before pushing to the remote repository...
* Improve encapsulation of IpDropDown
- Moved function declarations not meant for public consumption to .cpp file.
- Moved includes to .cpp file since they are no longer required in IpDropDown.h.
* Move all non EXPORT declares from op2ext.h into op2ext.cpp
- Preparation work for moving all EXPORT function declarations into op2ext.h
* Move non-EXPORT code from op2ext.cpp into DllMain.cpp
In preparation for moving all EXPORT functions into op2ext.h/op2ext.cpp.
* Move public interface for op2ext from public.h to op2ext.h
- Added EXPORT bool GetGameDir_s(char* buffer, unsigned int bufferSize)
- Allows for a safer version of GetGameDir that is compatible with the C ABI
- Moved definitions of all EXPORT functions to op2ext.cpp
- Improved comments on EXPORT functions
* Rename ModMgr to ConsoleModuleLoader
* Remove dllexport from GetGameDirectory function
It returns a string and is not compatible with the C ABI. See GetGameDir_s for new EXPORT function.
* Remove unnecessary extern call for a modManager in op2ext.cpp
* Rename class CommandLineModuleLoader to ConsoleModuleLoader
Lines up with new filename ConsoleModuleLoader.h/ConsoleModuleLoader.cpp and is a little shorter in length when typing class name.
Namespaces are a tool for preventing naming collisions and organizing code into logical sections. Classes are for grouping code with a similar responsibility into an object that has a clearly defined public interface and hidden internal workings.
Sounds like you've said the same thing twice. What's the difference? "organizing code into logical sections" versus "grouping code with similar responsibliity". Having a clearly defined public interface and hidden internal workings could also be said of namespaces: Wrap the private stuff in a nested anonymous namespace.
To me, the main difference is instance data being tied to functions.
Additionally, I wrote bool GetGameDir_s(char* buffer, unsigned int bufferSize). It returns false if the provided buffer size is too small for the path, but still fills the buffer with as much of the path as possible. Looking for feedback if the bool return is a good addition or not...
Yes, this is a proper way to handle this. Personally, I've come to detest success/failure return values of methods that are supposed to do something (as opposed to true/false return values of methods designed to check something). Having to always check success/failure at the point of call quickly leads to messy code. The cleaner way is to use exceptions. However, due to C interface restrictions on the exported functions, an exception is not possible here. Hence you have to fall back to the more C way of doing things with a success/failure return value.
#define EXPORT extern "C" __declspec(dllexport)
Careful about putting this in the header used for importing. ;)
Often macros like this are conditionally defined for a shared import/export header. If nothing special is done, the default is typically dllimport. If a project specific define is set, it can use dllexport. Another option is to try moving the dllexport part to the implementation file, which may actually make more sense if you can get it to work.
The one area I'm not sure about here is that I had to include 3 extern calls in op2ext.cpp to bring in the mod loader, and volList, and one bool variable. It doesn't seem too clean, but in order to give EXPORT functions access to the different functions like adding vol files, getting the game directory, and the mod directory, op2ext.cpp will need access to this code.
You can potentially use a master internal header file. Perhaps op2ext-internal.h, which includes op2ext.h, and then adds additional declarations for project private methods. Though that is mostly useful if those same extra methods are used among multiple files. If they're only used in one file (while possibly being defined in another), writing a declaration once in a header versus once in the only source file they are used is still the same amount of typing. Having to create tiny headers to export 1 item often doesn't feel worth it. A declaration versus an include is still one line. If you're declaring 3 items all from the same external file, maybe it's time to consider a header file for those methods, or perhaps consider if the functionality is well encapsulated.
Also, can anyone shed light on what EXPORT int StubExt = 0 does? It is available externally, but only declared and defined in op2ext.cpp. It should probably be declared in op2ext.h if it is truly meant for consumption outside of the op2ext dll...
Hmm, curious indeed. This probably need to be documented better. I grepped the source and found no uses of it. I also checked git blame, and it was never modified since the project's initial checkin. With no documentation, and no use, I initially assumed it was dead code that had accidentally been left in and should be removed. Though being external, it could be used by another project. With no documentation, you can't much expect a mod to use it. Though a quick check of Outpost 2:
grep -rn "StubExt" *
Binary file op2ext.dll matches
Binary file OP2Shell.dll matches
Binary file Outpost2.exe matches
It's likely a dummy export to satisfy a linking requirement from Outpost2.exe and OP2Shell.dll. Try removing it and see if the op2ext DLL fails to load at runtime. This is also likely why it's just defined as an int which is set to 0. There is no type information nor type checks with exported symbols. It's just a name and an address. Declaring an int, would be an easy way to generate an export with the required name.
I've also verified there are no "StubExt" matches with an original CD install. I think BlackBox added a dummy entry to the import table of Outpost2.exe to force loading op2ext.dll. Likely no other reason for that entry.
In which case, it doesn't need to be public, so doesn't need to be in op2ext.h. You can move it to the .cpp file.
Listing of recent commits. Maybe I need to take what Hooman said to heart and try to reduce the number of commits that I make a little by combining them locally first before pushing to the remote repository...
I don't really object to lots of small commits. They are easy to verify that way. Though once you push, you can't really amend things easily. Hence, my suggestion is more to not push your changes right away after each commit. Wait until the end of your programming session to push. Plus, you'll have a better sense of how you should have done the work by then, so you can repackage your commits nicer if you realize any were botched or out of order.
leeor,
Sorry I forgot to address your comment earlier. I'm split about the use of markdown syntax. While I like the nice formatting when reviewing the ReadMe on GitHub, when I download the repo onto my computer, there isn't a standard program to open a .md file. So I end up having to manually tell Windows 10 to use Notepad/Wordpad or something. These programs open the file in raw format (without the markdown formatting applied). So now I have to review the readme in raw form or hunt around the internet for a program that shows the markdown formatted. Fortunately, the markdown is fairly clear to read even in raw form, but this is really annoying to me. Just using a .txt file seems to be most compatible.
Often macros like this are conditionally defined for a shared import/export header. If nothing special is done, the default is typically dllimport. If a project specific define is set, it can use dllexport. Another option is to try moving the dllexport part to the implementation file, which may actually make more sense if you can get it to work.
I'm not exactly following you here. Maybe a short example would be helpful. Are you saying that instead of EXPORT, we should use OP2EXT_API for the macro in op2ext.h?
#define OP2EXT_API extern "C" __declspec(dllimport)
None of the functions marked for EXPORT are used within op2ext.
When commenting out SubExt, I get the dialog box with the following error and the game does not load:
Entry Point Not Found
The procedure entry point StubExt could not be located in the DLL Outpost2.exe.
I added the following comment, and StubExt was already in the .cpp file.
//Dummy export for linking requirements from Outpost2.exe and OP2Shell.dll. Forces Outpost2.exe to load op2ext.dll.
EXPORT int StubExt = 0;
Please update the comment if I worded it wrong.
You can potentially use a master internal header file. Perhaps op2ext-internal.h, which includes op2ext.h, and then adds additional declarations for project private methods.
op2ext.h is not used anywhere in the rest of the application, so it wouldn't need to be included in op2ext-internal.h. op2ext.h is currently strictly for consumption by external modules, not within op2ext.
Both dllMain.cpp and op2ext.cpp would #include op2ext-internal.h. Then the declarations for the static instances of the following variable and classes would exist inside op2ext-internal.h instead.
op2ext-internal.h
#include "ConsoleModuleLoader.h"
#include "VolList.h"
extern ConsoleModuleLoader consoleModLoader;
extern VolList volList;
extern bool modStarting;
Moving the declaration of volList from VolList.h to op2-internal.h and consoleModLoader from ConsoleModLeader.h to op2ext-internal.h seems like a good idea to me. I think we are getting there...
-Brett
Are you saying that instead of EXPORT, we should use OP2EXT_API for the macro in op2ext.h?
That would perhaps be a better descriptive name. The macro will need to expand to dllexport when compiling op2ext, and expand to dllimport when compiling a project that links to op2ext. Expansion can be controlled by a second macro, which is set in the op2ext project settings, under preprocessor definitions. If the macro is set, you should use dllexport. A client project which uses op2ext won't have this macro set, so it's absence would indicate dllimport is desired.
#ifdef OP2EXT_PROJECT_SPECIFIC_MACRO // Defined in op2ext project settings
#define OP2EXT_API __declspec(dllexport)
#else
#define OP2EXT_API __declspec(dllimport)
#endif
None of the functions marked for EXPORT are used within op2ext.
That's fine, and largely irrelevant. There is a distinction between the function declarations and the function definitions. The function definitions (in the .cpp file) will always be exported. The function definitions are only ever seen when compiling op2ext. The function declarations (in the .h file) may need to change. Only the header file is used by both the op2ext project, and other projects that use op2ext. The function declarations in the header will need to specify import, when compiling from other projects.
Note that I left out what the header file should say when compiling the op2ext project itself. It should be correct to specify export. That certainly won't conflict with the function definitions. I suspect it might be fine to not declare export, if perhaps forward declares don't need to match perfectly. Or maybe you can choose just one of either the definition or the declaration to mark as export. I'm not actually sure. In fact, you shouldn't even need to use the header file at all when compiling op2ext, since you're not using those functions internally, and so don't need to see them to call them from anywhere. Think back to the public.h that existed previously, but wasn't used within the project itself. Though I would recommend a #include for the header in the file that define the functions, since projects change over time, and the compiler can warn if the declarations and definitions drift apart, provided it can see both.
Maybe:
// Dummy export for linking requirements from Outpost2.exe and OP2Shell.dll. Outpost2.exe and OP2Shell.dll reference this dummy entry, causing op2ext.dll to load. It is not used in any way, but must exist to prevent Windows loader errors.
Really though, I'm wondering why Outpost2.exe and OP2Shell don't just reference one of the standard export functions. I'm also wondering why both have references, rather than just one of them.
You may be right about moving those extern declarations. It seems a bit messy to define an extern of a type in the same header file that declares that type. Though it can make sense in the case of singletons.
Btw, don't take the header file name I used too seriously. It's probably not a great name, and the format is not consistent with other header files.
I spent some more time on op2ext today.
I implemented the macro for switching between dllimport and dllexport within op2ext.h.
#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.
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):
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.
// 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
Did you notice the old vs. new op2ext code that you had to merge earlier makes a slight change to the console mod loader setting ART_PATH: it hooks the 2 places where OP2 reads that INI setting and fills in the mod path, instead of actually writing ART_PATH to the ini and deleting it every time you ran the game with a mod, just because that felt silly to me. :P I should look at those 2 places in OP2 again.
/* 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).
Ahh, nice. I like the submodule change.
I noticed something a bit misleading in the ini sample module:
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?
It was required in VolList.cpp in order to call the macro DBG. Otherwise, I would get the following error:
Error C3861 'OutputDebugString': identifier not found
That's about what I expected.
I changed the macro out for a function named OutputDebug. This way Windows.h could be included in GlobalDefines.cpp and cover the function's use of the Windows header instead of forcing all calling files to include Windows.h. I also removed the 3 line call to the macro sense it is now a function.
I think this is another case of macros making C++ code less intuitive. No reason not to just use a function here.
That's a really good way of solving the problem. Doesn't clutter up a global header with a large include. Also results in very nice syntax at the call point.
One difference I want to point out, the macro only expanded when DEBUG was set, otherwise it was silently swallowed by the preprocessor. This resulted in no call overhead, nor argument computation in release mode. I don't think we need to replicate that behaviour though. I think it makes more sense to have debug output available even in release mode builds. It may even make more sense to log that information now that it is dynamic.
In op2ext.h and op2ext.cpp, for method GetGameDir_s, consider declaring bufferSize as size_t rather than unsigned int.
Fixed.
I just noticed size_t is declared in the <stddef.h> header. I added an include for this. There is also a <cstddef> header, which declares std::size_t. We want to stick to using a C interface for the publicly exported stuff though.
We should consider adding a .c test file, so the compiler can warn if we use C++ features in op2ext.h.
IpDropDown.h shows which functions from the .cpp file are meant for public exposure to the rest of the program. The functions declared only in the .cpp file are meant for internal to IpDropDown use. This seems like a pretty useful convention to me. Additionally, I thought separating code into .h/.cpp files in general is supposed to speed compilation. Could you explain why you want to get rid of the .h file?
The contents of IpDropDown.h consists of:
void InstallIpDropDown();
We could just replace the #include line with that contents of the file. It's only used in DllMain.cpp, which already contains plenty of other forward declares. Though I do accept the reason of convention and documentation.
The DestroyModFunc typedef is used in the struct IniModuleEntry which is declared in the header file. IniModuleEntry is used by a couple of functions and must remain in the header file. Only the InitModFunc could be moved to the .cpp file.
Ahh, I see it now. Good point. Yeah, keep them together then for consistency.
In my opinion, it is better to keep the implementation details involved in how the function is called separated out from the LoadModules code. I created a new CallModuleDestruction function to remove the implementation details of module destruction from UnloadModules.
Hmm, I'm looking at this now and wondering why we cache the DestroyMod function. We already have a handle to the module, which is kept to be passed to FreeLibrary later. The function is only called once. It can simply be looked up when it is needed, similar to the InitMod function. That would reduce the amount of data being stored in memory at runtime. I like the idea of being able to cache values alongside the module handle, so maybe preserve the struct, it may be useful if we extend the module system later.
I'm still on the fence about this part of the code, but I think it's actually more because of the inconsistency between InitMod and DestroyMod.
Quote
Should LoadModuleDll be responsible for displaying the error message? It's convenient, because that's where information is available, but something about it feels a bit odd. A cleaner solution might be to format and throw an exception, and have the caller display a dialog box or otherwise handle or log the error.
What do you mean by caller in this case? op2ext as a whole or the module's DLL? I think it is appropriate for the loader to create the dialog box.
Yes, op2ext should display that dialog box. I think it would be better display at the point of call, in the LoadModules function. The LoadModuleDll function could format the dllName into a string, and throw that as a runtime_error, which LoadModules could catch, and display in the contents of a dialog box. That separates out error detection from error reporting. It gives the caller more flexibility in how errors are reported, or if they are collated before reporting.
I see MaxVolFileCountReached was cleaned up. It's now a proper boolean function. I still find the name a bit wordy and unclear though. Perhaps IsFull?
It's very likely size_t was included indirectly through another header file. That's a common issue with #include, and how it basically does a public import, so you end up with everything recursively included in your namespace.
Including a C Test module seems like a good idea. I do not want to repeat all the tests in the current TestModule in both C++ and C code though. Perhaps the correct thing to do would be to rewrite the C++ test module as a C TestModule. I have not written a lick of C code beyond working on these C ABI interfaces. So, for myself to do the work would probably take a lot longer than expected as I muddled through it, which again is something I'm looking to avoid.
At bare minimum, have an empty .c file which includes the header, and make sure the compiler doesn't throw any errors. By changing the extension, hopefully the compiler is smart enough to compile it in C mode, and throw errors for C++ specific features. I don't think we need to test calling in C mode, or rewrite any tests, just make sure the interface passes basic parsing in C mode (and that C++ features don't pass parsing).
I tried this quickly from Linux (the lazy way, without a .c file):
i686-w64-mingw32-gcc -DOP2EXT_INTERNAL_BUILD op2ext.h
I quickly discovered a few small issues.
One error being that extern "C" is not valid C code. I moved that to it's own #ifdef __cplusplus block that wraps the whole header. That seems to be a common practice for designing a header that is used from both C and C++.
Another issue is the use of C++ type bool as the return type for GetGameDir_s. We should consider changing the return type, with int being the most logical candidate. That could still hold a true/false result, or it might also make sense to return the required size if the supplied buffer is too small.
The C compiler also objected to the deprecated part added to GetGameDir. I don't have a suggested workaround for that. (Edit: Using __declspec(deprecated("Message")) is compatible with C.)
I also tested on Linux with a C++ compiler (using an alias for the Mingw compiler name):
mingw -std=c++11 -DOP2EXT_INTERNAL_BUILD *.cpp
It doesn't build due to some platform differences, though the output looks mostly clean.
There are some errors about <filesystem>. That's not surprising.
It picks up an error in BOOL EnableWindowNew(HWND, BOOL) in IpDropdown:29:
error: too few arguments to function ‘errno_t _itoa_s(int, char*, size_t, int)’
_itoa_s(i, tmpStr, 10);
...
/usr/share/mingw-w64/include/sec_api/stdlib_s.h:18:27: note: declared here
_CRTIMP errno_t __cdecl _itoa_s(int _Value,char *_DstBuf,size_t _Size,int _Ra
That last one surprised me a little. Might be worth taking a quick look at, just to make sure the code is really doing what we think it's doing. Or it might be the method is non-standard, and has different signatures and implementations between platforms, which is not really such an issue, since we only expect to build this from Windows.
The Microsoft documentation on _itoa_s (https://msdn.microsoft.com/en-us/library/0we9x30h.aspx) suggests this form is being used:
template <size_t size>
errno_t _itoa_s(
int value,
char (&buffer)[size],
int radix
); // C++ only
C++ Reference - itoa (http://www.cplusplus.com/reference/cstdlib/itoa/) says the method is non-standard, but suggests it is common. It suggests using sprintf as an alternative.
I replaced it with the following, which will be consistent with other uses of sprintf-like calls in the code.
_snprintf_s(tmpStr, sizeof(tmpStr), "%d", i);
Having a return value of true to indicate success is just as arbitrary as equating 0 with false. It's all just a matter of convention and practicality.
Btw, the convention in Bash is to actually reverse the values of true and false. Try running the true and false commands (actual executable files), and checking their result code (return value from main) with $?:
true; echo $? # => 0
false; echo $? # => 1
This stems from having typical program return values indicate an error code, while wanting to preserve the notion of using the return value as meaning bool success. You have one sentinel value to indicate one response, while all other values indicate the other response.