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

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4138
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #75 on: January 10, 2018, 06:18:03 AM »
Quote
Perhaps if Visual Studio had a way to specify post build scripts in the .user files instead of the project file, it would be more helpful.

Agreed.



I took a look through some of the code.

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

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

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

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

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

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

Do we need IpDropdown.h?

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

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

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

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

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

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

Inconsistency: IniModuleLoader uses HINSTANCE, while ConsoleModuleLoader uses HMODULE.


I'm going to stop here for now. There are a few sections I didn't look at in much detail.
« Last Edit: January 10, 2018, 06:22:50 AM by Hooman »

Offline Arklon

  • Administrator
  • Hero Member
  • *****
  • Posts: 1150
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #76 on: January 10, 2018, 05:11:36 PM »
I updated the expected location of post build scripts to assume that the Outpost 2 game code is in the root directory of op2ext (./Outpost2). I think this is more appropriate. Earlier, the scripts were copying files from op2ext to the SVN repo where I had Outpost 2. This directory scheme will likely be different for other developers.

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

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

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

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

Offline Vagabond

  • Hero Member
  • *****
  • Posts: 557
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #77 on: January 11, 2018, 12:11:18 AM »
Thanks for the feedback.

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

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

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

-Brett

Offline Vagabond

  • Hero Member
  • *****
  • Posts: 557
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #78 on: January 13, 2018, 01:02:39 AM »
Again, thanks for the feedback Arklon and Hooman. I just quoted large sections of Hooman's feedback and then replied underneath each section. I'll push an rc2 in the other thread shortly.

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

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

Error   C3861   'OutputDebugString': identifier not found

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

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

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

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

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

Fixed.

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


Fixed.

Quote
Do we need IpDropdown.h?

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

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

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

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

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

Quote
IniModuleLoader::CallModuleInitialization has a memory leak.

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

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

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

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

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

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

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

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

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

Codebase updated to use HInstance only for standardization.

Thanks,

Brett

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4138
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #79 on: January 13, 2018, 08:13:05 AM »
Quote
It was required in VolList.cpp in order to call the macro DBG. Otherwise, I would get the following error:

Error   C3861   'OutputDebugString': identifier not found

That's about what I expected.

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

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

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

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

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

Fixed.

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

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

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

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

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

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

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

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

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

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

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

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

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

I see MaxVolFileCountReached was cleaned up. It's now a proper boolean function. I still find the name a bit wordy and unclear though. Perhaps IsFull?
Code: [Select]
if (!volList.IsFull())

Offline Vagabond

  • Hero Member
  • *****
  • Posts: 557
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #80 on: January 15, 2018, 11:24:07 AM »
I'm limited on time to work on this, so sorry for the delays in getting back. After looking into throwing runtime_errors for missing DLLs, I'd like to unroll the actual final version of op2ext for this release. So this is sort of coming to a last chance to write on changes before I'm done working.


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

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

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

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

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

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

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

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

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

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

Thanks,

-Brett

Offline Vagabond

  • Hero Member
  • *****
  • Posts: 557
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #81 on: January 16, 2018, 10:20:02 AM »
Quote
The LoadModuleDll function could format the dllName into a string, and throw that as a runtime_error, which LoadModules could catch, and display in the contents of a dialog box.

Done.

Offline Hooman

  • Administrator
  • Hero Member
  • *****
  • Posts: 4138
Re: Allowing Outpost2 to load any vol file (*.vol)
« Reply #82 on: Today at 04:18:06 AM »
It's very likely size_t was included indirectly through another header file. That's a common issue with #include, and how it basically does a public import, so you end up with everything recursively included in your namespace.


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

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



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

I quickly discovered a few small issues.

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

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

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



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

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

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

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

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

The Microsoft documentation on _itoa_s suggests this form is being used:
Code: [Select]
template <size_t size>  
errno_t _itoa_s( 
   int value, 
   char (&buffer)[size], 
   int radix   
); // C++ only

C++ Reference - itoa says the method is non-standard, but suggests it is common. It suggests using sprintf as an alternative.

I replaced it with the following, which will be consistent with other uses of sprintf-like calls in the code.
Code: [Select]
_snprintf_s(tmpStr, sizeof(tmpStr), "%d", i);
« Last Edit: Today at 06:08:28 AM by Hooman »