C7-Game / Prototype

An early-stage, open-source 4X strategy game
https://c7-game.github.io/
MIT License
34 stars 9 forks source link

GetCiv3Path() returns null if Civ3 isn't in the Windows registry, and that causes issues #79

Closed QuintillusCFC closed 2 years ago

QuintillusCFC commented 2 years ago

Stumbled upon this while reviewing Jim's PR.

Steps to reproduce:

  1. Find a Windows box without Civ3 installed, or (more plausibly, since we all have Civ3 installed), rename the key HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Infogrames Interactive\Civilization III to something else temporarily, so C7 won't find it.
  2. Run C7

Expected: You are able to select the Civ3 home. Actual: C7 crashes. Output:

Unhandled Exception:
System.ArgumentNullException: Value cannot be null.
Parameter name: path1
  at System.IO.Path.Combine (System.String path1, System.String path2) [0x00003] in <1400ea11d6fd49d4939be686a9549fb4>:0
  at Util.FileExistsIgnoringCase (System.String exactCaseRoot, System.String ignoredCaseExtension) [0x00001] in C:\Development\Civ Related Projects\Prototype\C7\Util.cs:60
  at Util.Civ3MediaPath (System.String relPath, System.String relModPath) [0x0008e] in C:\Development\Civ Related Projects\Prototype\C7\Util.cs:109
  at MainMenuMusicPlayer._Ready () [0x00002] in C:\Development\Civ Related Projects\Prototype\C7\MainMenuMusicPlayer.cs:14

The fact that it can't find the main menu music is a red herring, the real problem is that it's even trying to despite the registry not containing the expected key. The relevant code in GetCiv3Path():

                // Look up in Windows registry if present
        path = Civ3PathFromRegistry("");
        if (path != "") return path;

It turns out that the method we use in Civ3PathFromRegistry to check the registry returns null if it doesn't find anything, but out code expects it to return the empty string.

Of note is that I've tested this on Windows 8.1 64-bit. I know the Windows registry format has not been entirely the same for the past 30 years, so it's possible that it does return the empty string on another version of Windows. I have separate registry-parsing code for pre-and-post-Vista in my editor, so it's not a wholly theoretical possibility.

JimOfLeisure commented 2 years ago

Ah, I seem to have misunderstood the default value parameter: https://docs.microsoft.com/en-us/dotnet/api/microsoft.win32.registry.getvalue?view=net-6.0 . The registry is weird sometimes, but I get the problem now. I'll see if I can fix it real quick.

JimOfLeisure commented 2 years ago

Created branch with fix: https://github.com/C7-Game/Prototype/tree/winregfix

I haven't tested it on Windows yet, but it doesn't crash Mac. Yay.

JimOfLeisure commented 2 years ago

I just tried it on Windows. It works as expected with the registry right, when the install_path value missing, and with the key missing.

I originally thought there was a problem because the button didn't show up, but the button PR hasn't been merged yet, and that's why it's "missing".

Should I make a PR for winregfix or just merge it into PR #78? They're both dealing with the Civ3 home path.

QuintillusCFC commented 2 years ago

I'd just as soon merge the existing approved PR, and open up a separate one for this. TBH it wouldn't matter a ton in this case, but "I'll just toss one more thing on this PR" can lead to really large PRs.

Also going to assign it to you since it's now in progress.