Game crashes sporadically when selecting a different world

[Brockengespenst]

SuperTux version: Development state b77810534218881a1dfd8a8a5ca384188c17bcff

System information: macOs Ventura 13.6.6

Expected behavior

No crash when selecting another world

Actual behavior

Game crashes sporadically with segfault

Steps to reproduce actual behavior

• Start game in story mode.
• Repeat following until crash: Press action button and select a different world.

Additional debugging information

Stacktrace:

1  std::vector<std::unique_ptr<GameObject, std::default_delete<GameObject>>>::begin() const vector                   1547 0x10099f2a0    
2  GameObjectRange<worldmap::Tux>::begin() const                                            game_object_iterator.hpp 118  0x10099f16c    
3  worldmap::Tux& GameObjectManager::get_singleton_by_type<worldmap::Tux>() const           game_object_manager.hpp  120  0x10099d051    
4  worldmap::Camera::get_camera_pos_for_tux() const                                         camera.cpp               85   0x100afcba9    
5  worldmap::Camera::update(float)                                                          camera.cpp               47   0x100afca8e    
6  worldmap::WorldMapSector::update(float)                                                  worldmap_sector.cpp      278  0x100b2a9f1    
7  worldmap::WorldMap::update(float)                                                        worldmap.cpp             148  0x100b1d3a1    
8  worldmap::WorldMapScreen::update(float, Controller const&)                               worldmap_screen.cpp      57   0x100b25cec    
9  ScreenManager::update_gamelogic(float)                                                   screen_manager.cpp       312  0x1009d4317    
10 ScreenManager::loop_iter()                                                               screen_manager.cpp       625  0x1009d568f    
11 ScreenManager::run()                                                                     screen_manager.cpp       665  0x1009d5bba    
12 Main::launch_game(CommandLineArguments const&)                                           main.cpp                 653  0x10083238c    
13 Main::run(int, char * *)                                                                 main.cpp                 739  0x10083405e    
14 main                                                                                     main.cpp                 30   0x1000077e3    

The stacktrace is a bit misleading, the problem seems to be that in WorldMapSector& get_sector() const { return *m_sector; } m_sector is nullptr. My assumption is, that on world change a new WorldMap is allocated which leads to an updated worldmap currenton. WorldMapSector::current()->get_sector() refers to the new WorldMap currenton which has no sector yet setup via set_sector and thus WorldMap::get_sector() tries to dereference a nullptr.

[Brockengespenst]

This seems to be related to the amount of update steps that are done between drawing a frame. I played around with the steps variable in void ScreenManager::loop_iter(). If hardcoded set to 1, I was not able to reproduce the crash. Another test with step = 16; it crashed on the first try.

[tobbi]

Potentially having get_sector() return a pointer to a sector instead of a reference could fix this issue but then we'd have to add null checks to every function calling that.

Does anyone have a better idea?

[Vankata453]

I think we should just make sure a sector is always available, since initialization. It's not supposed to not have a current sector set.

[Brockengespenst]

Maybe I found a solution. The worldmap camera is a member of WorldMapSector, so it belongs to a specific worldmap sector. When WorldMapSector::update() invokes m_camera->update(), Camera::update() calls Camera::get_camera_pos_for_tux() which uses WorldMapSector::current() that refers to the newly created WorldMap with m_sector being nullptr. Instead we could pass the WorldMapSector as argument to Camera's constructor, store it as member (reference) and replace the calls to WorldMapSector::current() with the member. A first quick and dirty test seems to work.

But in general I agree to @Vankata453 , that having always a valid current sector would be preferrable.

[Vankata453]

@Brockengespenst Thanks for checking this out! Would you like to create a PR for this?

[Brockengespenst]

Sure, no problem. Please check #2921.