Change Details

There is a chance that the CMapReader object created in either `RegisterInit()` or `RegisterInitRMS()` in World.cpp will leak when closing the game during the loading screen. This ends up causing the assertion failure `ScriptEngine.h(43): Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!""`, because the MapGenerator thread used in CMapReader contains a JS runtime that isn't deallocated if CMapReader leaks. CMapreader objects delete themselves in `DelayLoadFinished()` when progressive loading has finished ("self deleting" can be considered bad practice), but this may not happen if the game is closed while loading. Removing this self deleting behavior (i.e. the method `DelayLoadFinished()`) and holding the CMapReader pointer as a member variable in CWorld allows to better manage its lifetime. This ensures that CMapReader is properly deleted in the destructor of CWorld, as the destructor of CWorld is guaranteed to be called when closing the game during loading. A method could be added to CWorld to explicitly delete (`SAFE_DELETE`) the mapreader member var once map loading has finished completely (to be called from `CGame::ReallyStartGame()`). This is not done currently in the diff, but it may be preferable to do this, as the destructor of CWorld is not called until the game is closed/ended, and the map reader will never be used after the start of the game. The only other use of the CMapReader class is in Simulation2.cpp (for serialization tests and rejoin tests), and in some disabled tests in test_Serializer.h and test_Pathfinder.h, so those have been updated. Using `unique_ptr`s in the tests (it may be preferable to use one for m_MapReader as well instead of a raw pointer) allows to mimic the "self deleting" behavior originally intended for CMapReader.

There is a chance that the CMapReader object created in either `RegisterInit()` or `RegisterInitRMS()` in World.cpp will leak when closing the game during the loading screen. This ends up causing the assertion failure `ScriptEngine.h(43): Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!""`, because the MapGenerator thread used in CMapReader contains a JS runtime that isn't deallocated if CMapReader leaks. CMapreader objects delete themselves in `DelayLoadFinished()` when progressive loading has finished ("self deleting" can be considered bad practice), but this may not happen if the game is closed while loading. Removing this self deleting behavior (i.e. the method `DelayLoadFinished()`) and holding the CMapReader pointer as a member variable in CWorld allows to better manage its lifetime. This ensures that CMapReader is properly deleted in the destructor of CWorld (or `SAFE_DELETE()`ed when loading is finished), as the destructor of CWorld is guaranteed to be called when closing the game during loading. ~~A method could be added to CWorld to explicitly delete (SAFE_DELETE) the mapreader member var once map loading has finished completely (to be called from CGame::ReallyStartGame()). This is not done currently in the diff, but it may be preferable to do this, as the destructor of CWorld is not called until the game is closed/ended, and the map reader will never be used after the start of the game.~~ m_MapReader is now `SAFE_DELETE()`ed when loading is finished. The only other use of the CMapReader class is in Simulation2.cpp (for serialization tests and rejoin tests), and in some disabled tests in test_Serializer.h and test_Pathfinder.h, so those have been updated. Using `unique_ptr`s in the tests (it may be preferable to use one for m_MapReader as well instead of a raw pointer) allows to mimic the "self deleting" behavior originally intended for CMapReader.

There is a chance that the CMapReader object created in either `RegisterInit()` or `RegisterInitRMS()` in World.cpp will leak when closing the game during the loading screen. This ends up causing the assertion failure `ScriptEngine.h(43): Assertion failed: "m_Runtimes.size() == 0 && "All runtimes must be destroyed before calling JS_ShutDown!""`, because the MapGenerator thread used in CMapReader contains a JS runtime that isn't deallocated if CMapReader leaks. CMapreader objects delete themselves in `DelayLoadFinished()` when progressive loading has finished ("self deleting" can be considered bad practice), but this may not happen if the game is closed while loading. Removing this self deleting behavior (i.e. the method `DelayLoadFinished()`) and holding the CMapReader pointer as a member variable in CWorld allows to better manage its lifetime. This ensures that CMapReader is properly deleted in the destructor of CWorld (or `SAFE_DELETE()`ed when loading is finished), as the destructor of CWorld is guaranteed to be called when closing the game during loading. ~~A method could be added to CWorld to explicitly delete (`SAFE_DELETE`) the mapreader member var once map loading has finished completely (to be called from `CGame::ReallyStartGame()`). This is not done currently in the diff, but it may be preferable to do this, as the destructor of CWorld is not called until the game is closed/ended, and the map reader will never be used after the start of the game.~~ m_MapReader is now `SAFE_DELETE()`ed when loading is finished. The only other use of the CMapReader class is in Simulation2.cpp (for serialization tests and rejoin tests), and in some disabled tests in test_Serializer.h and test_Pathfinder.h, so those have been updated. Using `unique_ptr`s in the tests (it may be preferable to use one for m_MapReader as well instead of a raw pointer) allows to mimic the "self deleting" behavior originally intended for CMapReader.