Updates for Outpost 2 1.3.7

[Vagabond]

I was wanting to work some minor improvements on Outpost 2 for inclusion in the next release. Not sure how far past the anniversary of Outpost 2 a release can be made and still call it an anniversary release. I'm also not really sure how far along other projects are for inclusion in 1.3.7.

I'd like to keep game balance changes outside the realm of this thread. While I'm not necessarily against some balancing changes, they tend to take over the discussion and bury the rest of the administrative work that I am interested in.

COMPLETED

• Replace the in game tile sets (Well00XX.bmp) with non-Outpost 2 specific BMP files. (The game can load either format, so this will make it easier for people to open and modify the wells in a generic graphics editor easier. It would also make it easier to use the OP2MapImager with the stock game download.)
• Replace dtek_e.txt with a modified version that allows the Pre-Release Eden demo to be beaten. See http://forum.outpost2.net/index.php/topic,5795.msg82861.html#msg82861.
• Remove Punwick Junction's scenario DLL. The associated map file isn't included in the official download.
• Reorder the change log show the most current release at the top and the oldest release at the bottom (so you see the newest stuff when you first start reading.)

OUTSTANDING

• Remove the outdated 1.34 version string from the in game options screen. The correct version seems to now be listed on the About Outpost 2 Screen. See image below.
• Rewrite the vol loader code to load all vol files in the Outpost 2 directory instead of just the split maps.vol files. the https://forum.outpost2.net/index.php/topic,5915.0.html.
• Add a devReadMe to the repository detailing a list of what should be done or looked into each release cycle.
• Add a list of known bugs to the change log.
• Add completed scenarios to the official download. See https://forum.outpost2.net/index.php/topic,5925.msg84326.html#msg84326. I would appreciate other people's opinion testing scenarios, but am happy doing the work myself if no one else if available. Besides running the scenarios and making sure they don't crash, etc, I won't be reviewing the DLLs for security concerns. I'm assuming if someone put the time into making a complete scenario, they probably didn't intentionally put anything negative in the file. If we want to go further than making sure they work right with Outpost 2, I would need some assistance.

If these things seem agreeable, I'll start pushing them into the active GameDownload trunk of the REPO with decent COMMIT messages. We can use the commits to verify the change log before release.  https://svn.outpostuniverse.org:8443/!/#outpost2/view/head/GameDownload/Outpost2/trunk

-Brett

Old Version String contained in Outpost 2 1.3.6

[Hooman]

This sounds like a good plan. Very clear, and actionable. I support these changes.

My one concern is I suspect #6 could cause things to get a bit bogged down. If things do start to get bogged down, I would consider a partial list to be sufficient for a new release.

[Vagabond]

Thanks Hooman,

Since no one has responded negatively, I will start slowly working these suggestions into the repository.

I'll post in the pair programming listing about finding a date for working on the vol loader code.

My one concern is I suspect #6 could cause things to get a bit bogged down. If things do start to get bogged down, I would consider a partial list to be sufficient for a new release.

Agreed. Once the rest of the list is knocked out and tested, unless there are specific dates set aside for testing scenarios, we really should go ahead with a partial list.

-Brett

[Vagabond]

Just wanted to post a quick update on progress here, below are the SVN commit messages.

Replace Outpost 2 specific wells (tile sets) with standard BMP files
    * All files located in maps.vol. Files are well0000.bmp through well0012.bmp. New format is standard 8 bit BMP. Standard 9 bit BMP files are fully compatible with Outpost 2.

Update tech tree dtek_e.txt so Pre Release Eden Demo can be beaten
    * Updated file is located in maps01.vol.

Remove Punwick Junction (four players, Last One Standing) DLL
    * Punwick Junction does not have an associated map file and cannot be played in its current form. The map was based on La Corrida without a peacekeeper AI in the center of the map.

Beyond that, some good progress has been made on op2ext.

 * Better error messaging for problems when loading mods or vol files.
 * Allows loading vol files with any name (*.vol) into the game.
 * Some security fixes involving char* manipulation and copying code.
 * The code is about halfway through being rewritten using C++11/14 practices and a general cleaning (at least best of my ability to clean in C++).
 * A bug in SetSerialNumber that was setting the values in the incorrect order was fixed.
 * Merged changes between the two different versions of op2ext floating around.

Besides a couple of implementation details with public.h and OP2 Memory Management, I should have the skills to finish the rewrite on op2ext (although anyone else's help is still much welcome).


My current head hurter is removing or at least updating the outdated 1.34 version string from the in game options screen (see picture from first post for what I'm talking about). If someone has knowledge on how this is being set, it would be appreciated. I haven't dug into hxd or Ollydbg or anything myself yet to try to find it...


I'd like to add the following items to the list for this release:

 * Have the change log show the most current release at the top and the oldest release at the bottom (so you see the newest stuff when you first start reading.)

 * Add a list of known bugs to the change log.

 * Add some sort of devReadMe to the repository detailing a list of what should be done or looked into each release cycle. Just looking to make it easier to prep a release without missing something.

I'll keep from adding these 3 changes to the first post for a couple of days in case anyone has issue with them or a different plan to discuss.


Concerning testing, before this goes live, we need to make sure that everything still works with the most current version of the OP2Mapper concerning the addition of non Outpost 2 specific BMP tile sets and changing the names of some of the vol files. I can't think of any other programs that would be affected, but if they exist, please post and we can test them. I don't want to push content that makes the mapper harder to use or other programs.

-Brett

[Hooman]

New format is standard 8 bit BMP. Standard 9 bit BMP files are fully compatible with Outpost 2.

Umm, 9 bit BMP? What?

Actually, I had to verify they were indeed 8 bit BMP. For some reason I remembered them being 16 bit, but they're not.

* Have the change log show the most current release at the top and the oldest release at the bottom (so you see the newest stuff when you first start reading.)

Yes, good idea. That would be a bit more standard. My one suggestion is to please do the flipping in it's own commit, without any additions or edits.

Concerning testing, before this goes live, we need to make sure that everything still works with the most current version of the OP2Mapper concerning the addition of non Outpost 2 specific BMP tile sets and changing the names of some of the vol files.

That's actually a good thing to test. I hadn't thought of that. Though I would expect it to handle either format just fine.

[lordpalandus]

I thought a byte was 8 bits... where did the extra bit come from, to make it a 9 bit BMP file? From what I understand of color, you can only have arrays of 8 bits each; 8 bit, 16 bit, 32 bit, 64 bit, etc. So how could one have a 9 bit color array?

[Vagabond]

sigh, just a typo on my part.

It should read:

All files located in maps.vol. Files are well0000.bmp through well0012.bmp. New format is standard 8 bit BMP. Standard 8 bit BMP files are fully compatible with Outpost 2.

I just tried, but do not have permission to go in and edit my commit message. Perhaps Hooman, Leviathan, or leeor_net could fix it?

Hooman,

No problem with reordering the change log in a separate commit from any other changes.

-Brett

[Hooman]

I just tried, but do not have permission to go in and edit my commit message. Perhaps Hooman, Leviathan, or leeor_net could fix it?

Err, no. It's set in stone now.

Subversion doesn't allow rewriting history. The only way to change it now would be to have an admin dump the repository, filter the dump file to make the change, and then reload the dump back into the repo, replacing it's contents. That generally means taking the repo offline during the replacement process. It also means anyone who did an update that includes a modified commit now has a borked local repo and must redownload. Not generally worth it, especially for small changes. You'd normally only ever do this in the case of legal considerations, such as copyright violations, or privacy concerns, such as when a confidential file is accidentally committed to a public repo.

With Git you can do it. Git supports history rewriting. Though it's generally discouraged to rewrite history after you've published.

Btw, you shouldn't push after every commit in Git. Just push once at the end of a session, or if you've just completed an urgent change that really needs to get out right away. As long as you haven't pushed yet, it's really easy to make changes like this. In particular, git commit --amend lets you fix typos in commit messages.


Edit: Oh the irony of all the typos I made in this post. I just edited it like 5 times.

[Vagabond]

The first post for this topic has been updated to show which items have been completed vs which are still outstanding.

I reordered the change log to be newest changes at top of file. I also added a 1st draft of Known Bugs to the top of the Changelog. See below for what I wrote. These were all I could think of off the top of my head. If anyone else can think of other bugs to add, please post them/add them yourself.

I'm open to changing the format of the bugs. Less information, different format for list, etc. Also Fractured Alliance isn't in Outpost 2 yet, but since I plan on adding it, I went ahead and listed the bug.


Outpost 2 Changelog & Known Bugs
--------------------------------

Known Bugs
----------

* Menus on title screen occasionally do not appear
  - only a single horizontal red line appears at top of menu with rest of menu nonexistent.
  - Menu is still clickable even though it isn't being drawn.
  - Typically clicking until managing to open a different menu will restore proper behavior.

* Starting building construction without committing the ConVec or structure kit
  - If interrupting the ConVec on the exact right frame, the building will start construction without tying up the ConVec. The structure kit will remain in the ConVec.

* Scenario: 4P, SRV, Forsaken World
  - Some starting colonies begin with buildings intermingled with what should be impassible terrain. One colony cannot access its starting ore deposit.

* Scenario: Plymouth Cold War
  - Attacking the AI base often creates a crashing bug soon after initial engagement.

* Scenario: 5P, SRV, Fractured Alliance
  - Occasionally the scenario will crash on initial load.


Changelog
---------

1.3.7
 . . . MORE TEXT . . .[/tt]

Err, no. It's set in stone now.

Subversion doesn't allow rewriting history.

Interesting. There is an easy to find button in TotoiseSVN for editing previous commit messages. Since actually changing the commits is so difficult, I wouldn't have expected them to expose the functionality so nicely in TortoiseSVN.



EDIT
-----------

Using the standard BMP format affects the mapper. The minimap display inside the editor looks a bit weird. Colors are generally correct, but there is some 'darker noise' inside the tiles. Saving the minimap to file via the mapper produces the same results. You can use the mapper otherwise without problem. Bitmap minimaps produced by the mapper are attached to this post for comparison.

New BMP minimap produced by Mapper


Old BMP minimap produced by Mapper


Now that I think about it, Greenworld tilesets were doing this to me earlier when I was playing with them in the Mapper.

Thoughts?


EDIT 2

I found where in Outpost2.exe the version strings are being set. It was actually really easy using HxD. I just searched for the text 1.3 and found them both immediately.

Offset 0xE1A00 - Sets the OPU Ver within the in game menu preferences section. I modded it to 1.36 locally and it didn't crash and showed the new number. I guess now we need to decide if we want to rip it out or start updating it with version changes. I tend towards removing it, but I don't actually know how to remove it, just edit it...

Offset 0xE7F3C - The beginning of the text 1.3.0.4. I'm betting this is the value for checking multiplayer matches. I haven't tested it out yet. Perhaps I'll find some time to mod one copy of Outpost 2 and then try to run up a multiplayer match and see what happens.

Strangely, 0xE7F3C doesn't match the hex address we are using in SetSerialNumber in op2ext.

Code: [Select]

char *verStrAddr = (char*)0x004E973C;

Thanks,
Brett

[Hooman]

Interesting. There is an easy to find button in TotoiseSVN for editing previous commit messages. Since actually changing the commits is so difficult, I wouldn't have expected them to expose the functionality so nicely in TortoiseSVN.

Ahh, right, I remember now. The commit message can be changed, but changes to it are not versioned. As changing it will overwrite and discard the old value, you can't do it without some configuration. There are details on this on StackOverflow:
How to edit log message already committed in Subversion?

Unfortunately, I don't have admin access on the current SVN server to make such a change. You'd have to ask Lev if you want to make such a configuration change.

As a side note, SVN doesn't hash all the files and the commit message to create a secured history. Couple that with log messages not being downloaded into the working copy, and only visible by directly contacting the server, so yes it is possible to make such changes without ensuing havoc caused by conflicting data in other people's working copy. Still, editing history is always something you want to be careful about, as it can be abused to ill effect.


Interesting note about the dark colors for the mapper minimaps. I assume the full sized tiles still look right? If so, it might be a format error for the minimap bitmaps. Perhaps bit field sizes are off by 1 bit or something, which can have such an effect. Guess I'd have to look at the code. Maybe there's a bug in the mapper?


I believe version strings were updated with a hex editor in the past, so that's probably the right way to do it. I like your idea of removing the version string. If the information is available elsewhere, it may be better to remove duplicates. More maintainable that way, and then we don't have to go digging through the executable again for future releases.

To remove the strings, you can truncate them by placing a zero byte where the new end should be. Make sure it's a binary/hex 00, and not an ASCII "0". Overwriting strings like that is usually quite safe, including early truncation while preserving the space the string used to take up. In contrast, changing the length of strings, by deleting or adding bytes, would shift the remaining contents of the exe file and likely corrupt it.

There may be version numbers in both Outpost2.exe and OP2Shell.dll.

[Vagabond]

To remove the strings, you can truncate them by placing a zero byte where the new end should be. Make sure it's a binary/hex 00, and not an ASCII "0". Overwriting strings like that is usually quite safe, including early truncation while preserving the space the string used to take up. In contrast, changing the length of strings, by deleting or adding bytes, would shift the remaining contents of the exe file and likely corrupt it.

There may be version numbers in both Outpost2.exe and OP2Shell.dll.

Took me a bit to figure out how to insert hexadecimal and not ANSI text 0 using HxD, but I finally figured it out. This erased the string from the Game Options Section. The starting address for OPU Ver: is 0xE46E4, which is not collocated with the associated version string at 0xE1A00.

I still cannot find where we set the version number that appears in the top of the ABOUT section. It was updated to 1.3.6, so it had to have been modified recently. If Leeor or Arklon know where it is at and have the time, the assist would be appreciated.

Interesting note about the dark colors for the mapper minimaps. I assume the full sized tiles still look right? If so, it might be a format error for the minimap bitmaps. Perhaps bit field sizes are off by 1 bit or something, which can have such an effect. Guess I'd have to look at the code. Maybe there's a bug in the mapper?

The full sized map inside the editor looks perfect. The bug only pertains to the minimap and when saving the minimap to BMP on disk. If this was an easy fix on the backend, I think it would be worth making. Even if we don't change out the original tilesets, it would make working with the Greenworld set more natural. I'm testing with version 2.2.1 of the mapper.

I'm leaning towards replacing the wells with the standard format files even though it affects the map editor. We can document in the change log where to get the original wells if the minimap discoloration bothers someone using the mapper. In the long term though, if anyone manages to rewrite the mapper, it would be nice to not worry about trying to support the older format.

If anyone disagrees with changing them out though, I can push the Outpost 2 specific ones back in.

Thanks,
Brett

[Hooman]

I took a look at the OP2Editor backend code. It contains some functions used to calculate minimap colors. I believe that's where we'd need to look. Curiously they are commented for removal. I can't say I remember why. I'm not even certain who wrote that code.

In CTileSet.cpp on line 712 there is a function:

Code: [Select]

// **TODO** Remove
void CTileSet::CalcMiniMapColors(int startTile, int endTile)[/tt]
...
  case 16: // Assume 5/5/5 for RGB components **TODO** test this code

The comment stands out, particularly given my earlier off by 1 bit assumption, and knowing that many 16-bit bitmaps are actually encoded 5/6/5, not 5/5/5 (Red, Green, Blue). Though this might not be the problem, since the bitmaps you're repacking are 8-bit, not 16-bit, so that block shouldn't be active.

We could add this to our next pair programming session.

[Vagabond]

I took a look at the OP2Editor backend code. It contains some functions used to calculate minimap colors. I believe that's where we'd need to look. Curiously they are commented for removal. I can't say I remember why. I'm not even certain who wrote that code.

...

We could add this to our next pair programming session.

I'm game to look at it. I'm guessing we would need to the pull the project into a modern version of Visual Studio first. Would we work in the GitHub version of the code here: https://github.com/OutpostUniverse/OP2Editor?

I'd like to treat this as a bugfix only and not make major changes to the code structure. Otherwise, op2ext and Outpost 3.1.7 will never get through to completion. 

Would this simply be recompiling the backend into a DLL, placing it next to the mapper, then testing? I don't think the mapper frontend exists in our repository, but once we have the C++ backend compiling, we could step-through debug the backend code as needed. If we fix and release, we can start saving the PDB file so future debugging can be done without having to update and recompile the code.

-Brett

[Hooman]

Ahh, well there we go. I didn't even realize it'd already been ported to GitHub. I guess it was an easy case though, since the project was developed largely outside of version control, and so has no real commit history.

Yes, a modern project file would be a good place to start.

Would this simply be recompiling the backend into a DLL, placing it next to the mapper, then testing?

Yes. It might even be simpler. I believe the build system can auto register COM DLLs, and if the front end is loading the DLL through the COM libraries, you shouldn't even need to copy it to the same folder as the mapper, it should just work. But failing that, copying it should do the trick. No need to recompile the front end.

Which reminds me, we should find and post the front end source somewhere.

I'm noticing a bit of dyslexia on the version number there.

[Vagabond]

I tested the version string in Outpost2.exe located at 0xE7F3C. It indeed controls the multiplayer compatibility. I'd like to update this to 1.3.0.7 since we are including the new multitek tech tree in 1.3.7 that would break compatibility with previous multiplayer sessions.


Below is a rough draft of the first Publishing ReadMe. Suggestions on formatting and other steps that should be included are welcome.


Outpost 2 Publishing ReadMe
---------------------------

This ReadMe contains instructions for publishing a new version of Outpost 2. While preparing to release a new version of the game, ensure these instructions are updated with any new requirements for publishing so they are taken on subsequent releases.


1. Ensure the newest version of op2ext has been compiled and included (https://github.com/OutpostUniverse/op2ext).


2. Ensure the newest version of all bundled extension modules (like NetHelper) are included

   A. Ensure proper settings have been set for bundled extension modules in Outpost2.ini.


3. Add new scenarios that seem free of bugs and are complete enough to play through.

   A. If tech tree modifications are made and multiple scenarios rely on the tech tree, ensure the changes do not negatively affect existing scenarios by changing their tech trees.

   B. Place associated new maps in maps.vol archive. For vol archive editing, see the console application OP2Archive (https://github.com/OutpostUniverse/OP2Archive).


4. Update version strings in all DLLs and Outpost2.exe to reflect the new release version info.

   A. Resources may be edited with Resource Hacker (http://www.angusj.com/resourcehacker/) or popular IDEs.


5. Update version string located in the about section.

   A. ? ? ?


6. Update internal version string for multiplayer.

   A. Open Outpost2.exe using a memory edit tool like HxD (https://mh-nexus.de/en/hxd/).

   B. Find version string at starting at Offset 0xE7F3C (Or search for version text).

   C. String format should be A.B.0.C. Where ABC represents Semantic Versioning A.B.C (https://semver.org/).


8. Review Changelog. Ensure all changes are recorded and fixed bugs removed from known bugs list.


9. When all changes have been committed to the repository, tag the release.


In the very long term, I'm thinking that OP2Utility and OP2Editor could be merged into one project. They are both designed to support manipulation of data and files associated with Outpost 2 as a library for other frontend applications. We would have to decide what to do about the COM interoperability code. Also, I'd like to see C++17 filesystem leave experiemental on the major compilers before continuing to spread it through all the projects. The combination of my lack of knowledge of COM and unfamiliarity with C++ made OP2Editor pretty intimidating for me to review/use several months ago.

-Brett

[Vagabond]

I was reading through this post by TH300 on how to use NetFix: https://forum.outpost2.net/index.php/topic,5313.msg81424.html#msg81424

The current default .ini settings for Outpost 2 1.3.6 for NetFix is:


[NetFix]
Dll = "NetFix\NetFixV3.dll"
GameServerAddr = "outpost2.net:47800"
ProtocolIndex = 3


As per the NetFix post, should this be updated to read:
GameServerAddr = "server.outpostuniverse.org:47800"

Thanks,
Brett

[Arklon]

I was reading through this post by TH300 on how to use NetFix: https://forum.outpost2.net/index.php/topic,5313.msg81424.html#msg81424

The current default .ini settings for Outpost 2 1.3.6 for NetFix is:


[NetFix]
Dll = "NetFix\NetFixV3.dll"
GameServerAddr = "outpost2.net:47800"
ProtocolIndex = 3


As per the NetFix post, should this be updated to read:
GameServerAddr = "server.outpostuniverse.org:47800"

Thanks,
Brett

No, the first one is correct. TH300's post is outdated.

[Vagabond]

I've uploaded the new op2ext.dll to the repository and restructured the vol files as discussed. So now we have:

 - art.vol
 - tech.vol
 - maps.vol

I also removed the tech files located in sheets.vol and placed them in tech.vol. Newer tech files had been sprinkled into maps0X.vol and I wanted to consolidate them all. Check SVN revision 1235 for the current state of affairs.

I started adding the new scenarios. Unfortunately, for some reason any scenario that uses survtech.txt fails to load. survtech is used on several popular multiplayer scenarios. I think sirbomber is the curator for it. The message I get is:

local load failed

and

Host could not load the scenario

They fail to load when survtech is loaded from a vol file. When survtech is placed loose in the root game directory, the scenarios work.

Has anyone encountered this sort of behavior before? I guess in the grand scheme we could leave it loose for the release and be okay, although I'd like to figure out what is wrong. I will try a couple other random troubleshooting steps another day as I'm out of time to work now.

To complicate things, there seems to be a small bug in op2archive when using the ADD command. Not sure if it is a bug though and need to troubleshoot some more. The CREATE command works fine though.

No, the first one is correct. TH300's post is outdated.

Thanks Arklon.

Also, thank you Leeor for making this a sticky thread.

-Brett

[leeor_net]

In the very long term, I'm thinking that OP2Utility and OP2Editor could be merged into one project. They are both designed to support manipulation of data and files associated with Outpost 2 as a library for other frontend applications. We would have to decide what to do about the COM interoperability code. Also, I'd like to see C++17 filesystem leave experiemental on the major compilers before continuing to spread it through all the projects. The combination of my lack of knowledge of COM and unfamiliarity with C++ made OP2Editor pretty intimidating for me to review/use several months ago.

-Brett

My personal suggestion is to dump COM code altogether. It's not portable, it's ugly as hell and there are better ways to achieve interoperability than through COM.

[Vagabond]

Quoting Myself:

They fail to load when survtech is loaded from a vol file. When survtech is placed loose in the root game directory, the scenarios work.

Has anyone encountered this sort of behavior before? I guess in the grand scheme we could leave it loose for the release and be okay, although I'd like to figure out what is wrong. I will try a couple other random troubleshooting steps another day as I'm out of time to work now.

To complicate things, there seems to be a small bug in op2archive when using the ADD command. Not sure if it is a bug though and need to troubleshoot some more. The CREATE command works fine though.

I rooted out some bugs in OP2Archive dealing with not placing files into volumes in alphabetical order. I'll be putting out a patch for OP2Archive to fix this behavior before continuing the work on Outpost 2 1.3.7. Thanks to Hooman for some help with the troubleshooting as usual.

-Brett

[Vagabond]

I placed 8 'new' scenarios and updated the tech tree of one scenario in Outpost 2. I listed the creator(s) in the change log. Hopefully this list includes everyone who should be credited, so please let me know if I missed anyone. (Or if people prefer anonymity, I can pull the names alltogether)

 * Updated survtech.txt, the tech tree used by Forsaken World (4P, SRV), by sirbomber.
 * Added scenario Bomber's Big Blast (4P, LoS), sirbomber.
 * Added scenario Hidden Treasure (4P, LoS), Flashy.
 * Added scenario Caught in the Crossfire (4P, SRV), Mcshay.
 * Added scenario Darkest Hour (4P, SRV), sirbomber.
 * Added scenario Danger Zone (5P, SRV), Flashy.
 * Added scenario Fractured Alliance (5P, SRV), by sirbomber.
 * Added scenario Judgement Day (6P, SRV), by sirbomber.
 * Added scenario Outcaster's Starship (Colony Game, Starship), by Lord of Pain, ECC, & Flashy.

There is another problem with OP2Archive. The sorting is working, however it sorts all capital letters before any lowercase letters. I've pushed a patch to GitHub and will distribute a newer binary soon that fixes this sort.


I've noticed that oftentimes the working copy of Outpost 2 does not fully close on exit on my machine. This manifests by vol files still being accessed by Outpost 2 on closure. The task manager will move Outpost 2 from the APP list to the x86 backround process list on closure. I've also noticed the Visual Studio debugger will not automatically terminate when Outpost 2 is closed and the debugger is attached.

Does anyone know where to start looking for solving this and/or a tool to use to see which processes remain open or something?

Thanks,
Brett

[Hooman]

I've noticed that oftentimes the working copy of Outpost 2 does not fully close on exit on my machine. This manifests by vol files still being accessed by Outpost 2 on closure. The task manager will move Outpost 2 from the APP list to the x86 backround process list on closure. I've also noticed the Visual Studio debugger will not automatically terminate when Outpost 2 is closed and the debugger is attached.

Does anyone know where to start looking for solving this and/or a tool to use to see which processes remain open or something?

Hmm, maybe investigate if op2ext changes are the culprit.

If the debugger is still running, it might be helpful to pause the program and see what's on the call stack, or what threads are still active (which could mean multiple call stacks).

[Vagabond]

Thanks for the tip Hooman.

I used the debugger to step through the detach process for op2ext and found the problem. When op2ext calls the DestroyMod function on NetHelper, it just hangs for a long time. Sometimes it is only maybe 3-5 seconds. Other times it just seems to hang a really long time. After the interval, op2ext finishes closing out and everything is fine.

If I do not add the NetHelper module to Outpost 2, then everything exits correctly.

Looking through NetHelper's code, DestroyMod contains the following line:

Code: [Select]

WaitForSingleObject(hFwdThread, INFINITE);

I don't know for sure, but I'm wondering if this is what is causing NetHelper to not close out on my computer for a variable long length of time. I'm not really sure what this line does or why it is passed INFINITE for the wait time. Below is the entire DestroyMod function for context, but checking it out in the repo is easy enough.

It is probably incorrect that NetHelper can force Outpost 2 to not close out for such a long time. However, when closing out of version 1.3.6 on my computer, Outpost 2 seems to close just fine. It will transfer from the active applications to a background application for maybe 1-2 seconds at most and then disappear, which seems reasonable behavior. So, I don't know what is different between 1.3.6 and the new version that is causing the hang. Perhaps there is still some underlying problem that I added to op2ext.

Code: [Select]

extern "C" __declspec(dllexport) bool DestroyMod() {
  bool result = true;

  if (!SetBindPatches(false)) {
    result = false;
  }

  if (mode != noForward) {
    if (!SetGetIPPatch(false)) {
      result = false;
    }

    if (hFwdThread) {
      shuttingDown = true;
      WaitForSingleObject(hFwdThread, INFINITE);
    }

    PortForwarder forwarder(mode == pmpOrUpnp || mode == upnpOnly,
                            mode == pmpOrUpnp || mode == pmpOnly);

    for (int i = startPort; i <= endPort; ++i) {
      forwarder.Unforward(true, i);
    }
  }

  return result;
}

I tried compiling NetHelper so I could step debug through it and verify that line is called and is the culprit, but I cannot get it to compile. I'm getting a lot of LINKER errors associated with miniupnpcd.lib.

Severity   Code   Description   Project   File   Line   Suppression State
Error   LNK2001   unresolved external symbol __imp__fprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(upnpreplyparse.obj)   1   
Error   LNK2001   unresolved external symbol __imp__fprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minissdpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp__fprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(connecthostport.obj)   1   
Error   LNK2001   unresolved external symbol __imp__fprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniwget.obj)   1   
Error   LNK2001   unresolved external symbol __imp__fprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(portlistingparse.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minisoap.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniwget.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(igd_desc_parse.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniupnpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(receivedata.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(upnpreplyparse.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minissdpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp__printf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(connecthostport.obj)   1   
Error   LNK2019   unresolved external symbol __imp__sscanf referenced in function _UPNP_GetStatusInfo   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(upnpcommands.obj)   1   
Error   LNK2001   unresolved external symbol __imp___snprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(connecthostport.obj)   1   
Error   LNK2001   unresolved external symbol __imp___snprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minisoap.obj)   1   
Error   LNK2001   unresolved external symbol __imp___snprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniwget.obj)   1   
Error   LNK2001   unresolved external symbol __imp___snprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniupnpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp___snprintf   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minissdpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp____iob_func   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(upnpreplyparse.obj)   1   
Error   LNK2001   unresolved external symbol __imp____iob_func   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(minissdpc.obj)   1   
Error   LNK2001   unresolved external symbol __imp____iob_func   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(connecthostport.obj)   1   
Error   LNK2001   unresolved external symbol __imp____iob_func   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(miniwget.obj)   1   
Error   LNK2001   unresolved external symbol __imp____iob_func   NetHelper   C:\Users\Brett\Documents\OutpostUniverseGitHub\NetHelper\src\miniupnpcd.lib(portlistingparse.obj)   1   

I tried playing with adding the folder with the miniupnp header files to the C++ include list and the 2 miniupnp lib files to the linker list, but it didn't seem to matter. Although I find getting these files to register in Visual Studio can be very finicky.

If I had the PDB file for either NetHelper or op2ext that is bundled with Outpost 2 1.3.6, I could step debug and see a little more what is going on.

-Brett

[Hooman]

Arklon may be able to help here.

What you found seems to make sense. The NetHelper is clearing out the port forwarding rules in the router, which requires communication with a remote device. Hence, the waiting. If there are any dropped packets or other communication problems, that could potentially be a long wait. It also looks like the cleanup may be done over a range of ports, in a serial manner, which can make the wait quite significant. I can't tell for certain look at this code, but perhaps it can be made quicker by doing the whole range of ports in parallel.

It might make sense to do the forwarding and cleanup when entering and exiting the multiplayer menu section of the game. That could hide the cleanup time, so there's no long delays during shutdown.


We should probably add some compile notes, or perhaps have a standard way to include additional dependencies in the repository, or as submodules. This is an issue we've kind of neglected for a few projects.

[Vagabond]

For compiling NetHelper, I read through the linker errors closer, and it appears the problem is related to Microsoft making breaking changes to their toolset regarding the header stdio.h. This change was introduced with VS2015 and it looks like NetHelper was compiled with the Visual Studio 2010 (v100) toolset.

See quote below from this Microsoft article: https://msdn.microsoft.com/en-us/library/bb531344.aspx#BK_CRT

The printf and scanf family of functions are now defined inline. The definitions of all of the printf and scanf functions have been moved inline into <stdio.h>, <conio.h>, and other CRT headers. This is a breaking change that leads to a linker error (LNK2019, unresolved external symbol) for any programs that declared these functions locally without including the appropriate CRT headers. If possible, you should update the code to include the CRT headers (that is, add #include <stdio.h>) and the inline functions, but if you do not want to modify your code to include these header files, an alternative solution is to add an additional library to your linker input, legacy_stdio_definitions.lib.

To add this library to your linker input in the IDE, open the context menu for the project node, choose Properties, then in the Project Properties dialog box, choose Linker, and edit the Linker Input to add legacy_stdio_definitions.lib to the semi-colon-separated list.

If your project links with static libraries that were compiled with a release of Visual C++ earlier than 2015, the linker might report an unresolved external symbol. These errors might reference internal stdio definitions for _iob, _iob_func, or related imports for certain stdio functions in the form of _imp_*. Microsoft recommends that you recompile all static libraries with the latest version of the Visual C++ compiler and libraries when you upgrade a project. If the library is a third-party library for which source is not available, you should either request an updated binary from the third party or encapsulate your usage of that library into a separate DLL that you compile with the older version of the Visual C++ compiler and libraries.

I'm looking at downloading the v100 toolset to test if this is actually the culprit.

What you found seems to make sense. The NetHelper is clearing out the port forwarding rules in the router, which requires communication with a remote device. Hence, the waiting. If there are any dropped packets or other communication problems, that could potentially be a long wait. It also looks like the cleanup may be done over a range of ports, in a serial manner, which can make the wait quite significant. I can't tell for certain look at this code, but perhaps it can be made quicker by doing the whole range of ports in parallel.

The time taken to cleanup NetHelper seems acceptable to me. The problem is if it hangs up and never completes destruction (or decides to take 5 minutes to complete). While maybe improvements could be made to the time taken I don't think it is needed. I want better proof it is actually that INDEFINITE wait call causing the hang before assuming though. Hopefully getting the code to compile on my machine with the older toolset will allow proving that.
See updated information at: http://forum.outpost2.net/index.php/topic,6067.0.html

It might make sense to do the forwarding and cleanup when entering and exiting the multiplayer menu section of the game. That could hide the cleanup time, so there's no long delays during shutdown.

What happens if I immediately click on multiplayer again after ending a match and cleanup is in process? Or what happens if I try to create a new match while NetHelper is still initializing? I think it is a lot cleaner to handle on opening the program and on program closing as is right now. Although I'd certainly defer tp what Arklon prefers here.

We should probably add some compile notes, or perhaps have a standard way to include additional dependencies in the repository, or as submodules. This is an issue we've kind of neglected for a few projects.

I think Arklon actually hooked the code to the dependencies nicely. I just jumped to conclusions when all the linker errors showed up. Although some standardization might be a good thing here.

Thanks,
Brett