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

[Hooman]

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.

[Arklon]

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.

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.

[Vagabond]

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

[Vagabond]

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.

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.

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.

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

Fixed.

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

Fixed.

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?

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.

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.

IniModuleLoader::CallModuleInitialization has a memory leak.

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

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.

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.

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

Inconsistency: IniModuleLoader uses HINSTANCE, while ConsoleModuleLoader uses HMODULE.

Codebase updated to use HInstance only for standardization.

Thanks,

Brett

[Hooman]

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

Error   C3861   'OutputDebugString': identifier not found

That's about what I expected.

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

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

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

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

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

Fixed.

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

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

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

The contents of IpDropDown.h consists of:

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.

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

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

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

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

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

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

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

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

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

Code: [Select]

if (!volList.IsFull())

[Vagabond]

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.

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.

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.

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

[Vagabond]

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.

[Hooman]

It's very likely size_t was included indirectly through another header file. That's a common issue with #include, and how it basically does a public import, so you end up with everything recursively included in your namespace.

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

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


I tried this quickly from Linux (the lazy way, without a .c file):

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

[Vagabond]

Hooman,

I had to update the StubExt variable in op2ext.cpp to work with your modifications of op2ext.h. Easy fix though.

I'm glad we took the time to move all the "C" code to a single isolated header. It made your changes trivial to make it compatible with both C++ and C.

While testing the StubExt, I found a small error in the project settings for the TestModule when compiled in RELEASE mode. Not a big deal since it is a test case anyways, but fixed.

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.

Returns an int now.

I sort of like returning the required size, but it gets a bit weird. I would prefer to return 0 if the size is large enough, but this would look like returning FALSE, which is confusing. If I always return the size of the buffer used, you would have to test the returned int to make sure it is smaller than the supplied buffer, which is awkward as well. So, probably best to just return the INT acting like a C++ bool value and let the user guess how much bigger to make their buffer.


Test C file added to codebase. Only included in the project on DEBUG builds. I tested that it is compiling in C with the bool value. The compiler complained about it until changing GetGameDir_s was changed to int return.


I'm done contributing for this release but will continue to compile on Windows and test until everyone else is finished as well. Mostly because Hooman is running Linux which cannot compile code for Windows using the filesystem namespace yet.

I will be waiting on the final version of op2ext to rerrange the vol files and push the newer map and tech tree files as this relies on op2ext being able to load vol files with any names.

-Brett

[Hooman]

Good catch with StubExt. That's very easy for me to miss with my setup.

Perhaps move the test .c file out of the project and into the test module. It is essentially just test code anyway, not something that actually gets compiled into the project. Also, either way, there is no benefit from the DEBUG restriction, since no code is emitted anyway.

I sort of like returning the required size, but it gets a bit weird. I would prefer to return 0 if the size is large enough, but this would look like returning FALSE, which is confusing. If I always return the size of the buffer used, you would have to test the returned int to make sure it is smaller than the supplied buffer, which is awkward as well. So, probably best to just return the INT acting like a C++ bool value and let the user guess how much bigger to make their buffer.

I was also thinking return 0 if the buffer is large enough. That's much easier to check against. I don't think it's odd, or confusing. The function name doesn't imply any sort of semantics for the return value. Even if you assumed 0 meant FALSE, does that mean success or failure? I'd say, given the function name, you'd need to check documentation to know how to interpret the return value.

It'd be nice to make a proactive decision on this. Once the new op2ext is packaged with a new game release, breaking changes become a much bigger concern.

[Vagabond]

Even if you assumed 0 meant FALSE, does that mean success or failure?

If a function named something like GetGameDir_s returned false, it would be unintuitive to me if it didn't mean problem/failure. I would not check documentation on the bool unless it didn't perform as expected. I'm not experienced enough with C coding to know if the user would just assume int here really meant bool, although I might see myself falling into this logic?

How about size_t GetGameDir_s(char* buffer, size_t bufferSize)?

Seeing size_t would indicate to me a range value and not a bool. It would also make more sense comparing it against the passed size_t bufferSize.

Moving the test c code to the testModule is a nice call. This would render the DEBUG guard useless and allow testing in both DEBUG and RELEASE mode.

[leeor_net]

In C generally speaking a function returning an int other than 0 indicates failure. But, this is also subjective and dependent on API (though I can't think of an API I've used in C or C++ that indicates failure via 0).

Often times for 'boolean' values preprocessor definitions are used (or in the case with my Quake 2 based project an enumeration is used) where false == 0 and true == anything else.

[Hooman]

size_t GetGameDir_s(char* buffer, size_t bufferSize)

Yes, that would actually make the most sense. If it returns a required buffer size, it should be size_t.

In C generally speaking a function returning an int other than 0 indicates failure.

I would agree that is often the case. Many functions return 0 on success, and an error code on failure. That allows many possible error cases to be distinguished.

An example is program error codes returned to the shell (the value returned from main). Usually 0 is returned upon successful completion, while a non-zero value is assumed to be an error code. A lot of command line processing tools will terminate batch processing when they encounter a non-zero result, based on the assumption that indicates an error.

[Vagabond]

Out of the infinite numbers that mean TRUE by default, we have collectively chosen the only number that means FALSE to indicate TRUE (more specifically that something occurred successfully/without error). Nice.

Guess this is showing my lack of C/C++ experience and lower system level design.

I'll plan to make the GetGameDir_s and C test code location changes Monday or Tuesday and then make a release candidate compilation. This op2ext RC will be pushed into the SVN Outpost2 copy. I'll wait to tag the op2ext 2.0.0 release until after a final round of testing once all other changes are made in case anything unfavorable is discovered. If everything is good, the final RC will just become 2.0.0.

-Brett

[leeor_net]

Out of the infinite numbers that mean TRUE by default, we have collectively chosen the only number that means FALSE to indicate TRUE (more specifically that something occurred successfully/without error). Nice.

Welcome to the C branch of languages where nothing makes sense and you can trash the stack and bring an operating system to its knees just by typo-ing.

Anyway, a better way to look at it with return values is 0 indicates nothing out of the ordinary and anything else is some sort of status code vs. true/false. But we may be beating a dead horse now -- it's why I usually opt to indicate error codes via named enumarations vs. int values.

[Hooman]

Returns bool failed, which could be true in many possible ways

Having a return value of true to indicate success is just as arbitrary as equating 0 with false. It's all just a matter of convention and practicality.

Btw, the convention in Bash is to actually reverse the values of true and false. Try running the true and false commands (actual executable files), and checking their result code (return value from main) with $?:

Code: [Select]

true; echo $?   # => 0
false; echo $?  # => 1

This stems from having typical program return values indicate an error code, while wanting to preserve the notion of using the return value as meaning bool success. You have one sentinel value to indicate one response, while all other values indicate the other response.