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

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1267
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #25 on: November 20, 2017, 10:16:21 PM »
It would have been nice if this newer op2ext code had been committed overtop of the op2ext project in the SVN repository or Hooman and I told about it before we cloned the subversion repository. I'm starting to understand how Leeor posted 1.3.6 with an outdated version of op2ext.dll. Oh well...
I told Leeor about it when we were putting together 1.3.6, and I thought he committed it to the SVN, but I guess that never ended up happening? Supposedly it went in on Nov 15 2016.
« Last Edit: November 20, 2017, 11:46:08 PM by Arklon »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #26 on: November 22, 2017, 05:39:40 AM »
Wow. You've been busy.

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

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

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

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



I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.

Offline Arklon

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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #28 on: November 22, 2017, 02:38:24 PM »
Quote
I agree, but I when I was talking about him supposedly committing it to the SVN, I was talking about a year ago before we had the ball rolling on GitHub.

All the more reason why I'm expecting things will improve in the future.  :)

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #29 on: November 22, 2017, 07:35:45 PM »
Quote
Quote from: Hooman on Today at 04:39:40 AM
I think moving to GitHub will help keep code up to date. People can create their own accounts there, suggest changes without first getting permission, faster repo access times, and presumably more reliable servers. Just feels like a better environment for collaborating and keeping things up to date.

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

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

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

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

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

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

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

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



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

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

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



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

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

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

Thanks,
-Brett

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1267
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #30 on: November 22, 2017, 10:58:31 PM »
Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: int __thiscall TApp::Init(void)" (__imp_?Init@TApp@@QAEHXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
   
Error   LNK2019   unresolved external symbol "__declspec(dllimport) public: void __thiscall TApp::ShutDown(void)" (__imp_?ShutDown@TApp@@QAEXXZ) referenced in function "bool __cdecl Op2MemSetDword(void *,void *)" (?Op2MemSetDword@@YA_NPAX0@Z)
It needs to link against the OP2 SDK .lib file.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #31 on: November 23, 2017, 02:42:41 AM »
Indeed. TApp is one of the exported classes from Outpost2.exe. Hence why op2ext is declaring it with dllimport. You need a reference to Outpost2.exe in the import table of op2ext.dll to use it. You can include the same .lib file the level DLLs use to create that reference.

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

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

I don't remember enough detail on the version/serial numbers to answer your question.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #32 on: November 23, 2017, 04:05:03 AM »
Arklon, Hooman,

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

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

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

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

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

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



Op2ext.h included in all op2ext subcomponents

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

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

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

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

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

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

Thanks,
-Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #33 on: November 23, 2017, 06:42:02 AM »
Quote
When I saw __declspec in front of the definition for TApp, I just assumed it was __declspec(dllexport).
I figured as much  ;)

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

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

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

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



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



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

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

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


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



Meanwhile, I should look into getting the SDK up on GitHub.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #34 on: November 24, 2017, 03:32:10 AM »
Hooman, I agree this thread is starting to blow up into a lot more than originally intended. (Especially you bringing up combining Outpost2DLL and Outpost2App :-) )

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

CURRENT PLAN (subject to change of course…)

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

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

#define OP2EXT_API extern "C" __declspec(dllimport)

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

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

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

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

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

Rewriting history would likely stall this project for good, so no thank you. :)

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1267
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #35 on: November 25, 2017, 01:37:23 AM »
SetSerialNumber Function

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

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

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

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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #36 on: November 25, 2017, 04:26:44 AM »
Quote
but good luck convincing me to do that when I could just be lazy instead.
Hah! At least you're honest about it. Good to know yourself, I suppose.


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

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

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #37 on: November 25, 2017, 10:15:45 PM »

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

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

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



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

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

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

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



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

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

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

 

Offline Hooman

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

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

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

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

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #39 on: November 27, 2017, 03:23:03 PM »
Quote
It probably makes the most sense to name the public export header after the project itself: op2ext.h. When people try to use the library, that's what they'll try importing. An internal master header file should probably be named something else. Though if the headers are split and organized nicely, there might not be a master header file.

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

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

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

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

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

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

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

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

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

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

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



Moving public.h to op2ext.h...

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

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

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

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

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

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



Rewrite of IniMods

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

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

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

Summary of commits:

* Minor cleanup of code in IpDropDown.cpp.

* Refactor IniMods.cpp into the class IniModuleLoader

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

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

* Make GetPrivateProfileStdString dynamically resize buffer as required

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

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

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

Not necessarily happy with the name GlobalDefines though.


-Brett

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

I forgot to mention earlier but I've noticed that you can only include 1 mod using the command line (in addition to all the mods in the .ini file). Some references in the source code allude to loading multiple modules through the command line. Unless we specifically want to work towards supporting this, I'd like to edit the source to be more clear that only one mod is being loaded via command line.
« Last Edit: November 27, 2017, 08:34:21 PM by Vagabond »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #40 on: November 28, 2017, 04:38:47 PM »
I don't know anything about the NLS or Localization stuff. BlackBox would be a better source of info on that.

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

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



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

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

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

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

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #41 on: November 29, 2017, 04:43:56 PM »
Quote
I don't know anything about the NLS or Localization stuff. BlackBox would be a better source of info on that.

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

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

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

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

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

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

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

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

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



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

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

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



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

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

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

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

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



IpDropDown

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

Code: [Select]

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

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

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

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

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

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



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

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



Current task list

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

-Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #42 on: November 29, 2017, 09:30:51 PM »
Quote
All that being said, I’m okay punting multiple command line mods until a different release even if we want it (unless it is a big deal to support for someone in the near future). I can go in and clean the code some more so it is clear that only one mod is getting loaded.

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

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

That sounds like a good plan.

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

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

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

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

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

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

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

Probably, yes.

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

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

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

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

Quote
Hooman, is the Outpost2DLL code on GitHub stable yet? If so, I’ll work on setting up a submodule.
No, there are some things I still haven't worked out yet with my second conversion attempt.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #43 on: December 05, 2017, 07:10:06 PM »
Quote
First, what is the benefit of making it a class?

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

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

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

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

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

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

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



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

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



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

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

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

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

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

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

The following extensions are implemented for Outpost 2:

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

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

 * Addon modules using ini file

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

 * Extra VOL file loading

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

 * IP dropdown box for net play

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


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

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

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

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

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

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

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


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

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

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

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

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

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

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


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

Version 2.0.0

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

Version 1.0.0

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



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

-Brett
« Last Edit: December 05, 2017, 10:27:59 PM by leeor_net »

Offline leeor_net

  • Administrator
  • Hero Member
  • *****
  • Posts: 2350
  • OPHD Lead Developer
    • LairWorks Entertainment
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #44 on: December 05, 2017, 10:26:43 PM »
Want to suggest (again I think?) that markdown is probably a better format to use for readme's.

Also minor edit to your post, I put the readme file into a code tag for the sake of readability.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #45 on: December 05, 2017, 11:21:06 PM »
Quote
It has several functions that should not be called by code external to ipDropDown.cpp

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

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

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

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

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

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

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

That would be a good idea. Whenever possible, I like to keep things only in the .cpp file rather than the .h file. It reduces namespace pollution, and can improve compile times. Moving a function from a .h to a .cpp doesn't exactly make it private, but it does make it less visible. Another file could still access the function by explicitly declaring its signature.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #46 on: December 07, 2017, 02:03:47 AM »
After saying there would be no time for major updates until January, I found some time. Life is funny like that. :)

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

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

void InstallIpDropDown();

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

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



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

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

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

#pragma once

#define EXPORT extern "C" __declspec(dllexport)

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


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


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


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


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

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

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

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

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

EXPORT int StubExt = 0;

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

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

EXPORT char* GetCurrentModDir()
{
    . . .
}

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

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



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

* Improve encapsulation of IpDropDown

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

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

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

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

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

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

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

* Rename ModMgr to ConsoleModuleLoader

* Remove dllexport from GetGameDirectory function

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

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

* Rename class CommandLineModuleLoader to ConsoleModuleLoader

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

« Last Edit: December 07, 2017, 02:06:43 AM by Vagabond »

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #47 on: December 07, 2017, 04:10:56 AM »
Quote
Namespaces are a tool for preventing naming collisions and organizing code into logical sections. Classes are for grouping code with a similar responsibility into an object that has a clearly defined public interface and hidden internal workings.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

I don't really object to lots of small commits. They are easy to verify that way. Though once you push, you can't really amend things easily. Hence, my suggestion is more to not push your changes right away after each commit. Wait until the end of your programming session to push. Plus, you'll have a better sense of how you should have done the work by then, so you can repackage your commits nicer if you realize any were botched or out of order.

Offline Vagabond

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 1013
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #48 on: December 07, 2017, 02:36:13 PM »
leeor,

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



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

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

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

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



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

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


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

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

Please update the comment if I worded it wrong.



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

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

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

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

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

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

-Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4954
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #49 on: December 12, 2017, 09:51:14 PM »
Quote
Are you saying that instead of EXPORT, we should use OP2EXT_API for the macro in op2ext.h?

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

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

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

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

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



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

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



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

Btw, don't take the header file name I used too seriously. It's probably not a great name, and the format is not consistent with other header files.