calref / cboe

Classic Blades of Exile
http://spiderwebforums.ipbhost.com/index.php?/forum/12-blades-of-exile/
Other
167 stars 41 forks source link

Fix silent crash on Windows & Standardize command line arguments #363

Closed NQNStudios closed 2 weeks ago

NQNStudios commented 2 weeks ago

It turns out that on Windows, you could launch with a save file loaded by passing that save file as a command-line argument. But only on Windows, because this functionality was implemented in a platform-specific implementation of set_up_apple_events() which seems wrong on many levels.

It turns out, this could also lead to a silent crash if you tried to launch BoE with a party file, and the loading failed. load_party() would try to call showError(), which relies on the UI being initialized. This would crash.

So I modified showError() to check if UI is initialized. If it's not, it just prints the error to console instead.

Once that was fixed, I figure we want all platforms to support the same command-line launch arguments, so I moved that logic out of the Windows-only file and into boe.main.cpp

NQNStudios commented 2 weeks ago

Looks like this breaks the editor builds because they don't have finished_init globally

NQNStudios commented 2 weeks ago

Something is fishy here--I don't think loading the party from the command line actually works. I think it might be better to remove that feature from Windows than add it to Mac and Linux, if it's going to require fixing and also conflict with the command-line processing of the replay system.

NQNStudios commented 2 weeks ago

Oh-- I found the original intention for this functionality being implemented on Windows specifically https://github.com/calref/cboe/commit/459c68a6688dd547b3d92d0fafc08aea14fc5387

NQNStudios commented 2 weeks ago

There, now it works.

CelticMinstrel commented 2 weeks ago

Does set_up_apple_events still need the command-line args passed? Looks like it probably doesn't, so I'd say you may as well remove those.

I don't think it was technically wrong to put the logic in set_up_apple_events before though – what that function was doing on Windows and what that function was doing on MacOS was exactly the same thing, if by a different method. The apple events that it sets up allow you to load a saved game by double-clicking it on MacOS (even if the game is already running, actually), and the command-line processing allows you to load a saved game by double-clicking it on Windows. The name of the function was wrong for legacy reasons because I never bothered to rename it, but what it was doing was in fact cohesive across the two platforms.

…but it does make sense to support command-line processing in addition to apple events on MacOS, I guess.

NQNStudios commented 2 weeks ago

Once I saw the historical context for why the code was in that method, it did make a lot more sense.

The game itself is my priority, so I don't want to make a habit of fixing low-priority things in the scenario and party editors, but they do both share the same idiosyncrasy where Windows handles command-line arguments in a set_up_apple_events method, and the other platforms don't. I think it makes sense in this case to make the same change for both editors, before we forget that it might matter eventually.

So, the game and both editors will gain cross-platform support for opening a file on launch via the command line.

CelticMinstrel commented 2 weeks ago

And all this means Linux can now also load a save file from the command-line, which it couldn't before. So that's a plus.