novelrt / NovelRT

A cross-platform 2D game engine accompanied by a strong toolset for visual novels.
MIT License
183 stars 43 forks source link

If you create a NovelRT::Ecs::SystemScheduler as a variable which is then later initialised, this causes a crash #596

Open dndn1011 opened 7 months ago

dndn1011 commented 7 months ago

Note: for support questions, please use the #engine-user-help channel in our Discord or create a discussion. This repository's issues are reserved for feature requests and bug reports.

Describe the issue: If you create a NovelRT::Ecs::SystemScheduler as a variable which is then later initialised, this generates no compile error or warnings, but the engine will fail due to conflicts caused by thread shutdown in the move function.

    SystemScheduler::SystemScheduler(SystemScheduler&& other) noexcept
        : _systemIds(std::move(other._systemIds)),
          _typedSystemCache(std::move(other._typedSystemCache)),
          _systems(std::move(other._systems)),
          _entityCache(std::move(other._entityCache)),
          _componentCache(std::move(other._componentCache)),
          _workerThreadCount(other._workerThreadCount),
          _currentDelta(other._currentDelta),
          _threadAvailabilityMap((1ULL << other._workerThreadCount) - 1),
          _shouldShutDown(false),
          _threadsAreSpinning(false)
    {
        if (other.GetThreadsAreSpinning())
        {
            other.ShutDown();
            SpinThreads();
        }
    }

I suspect it should not be shutting down the threads when the SystemScheduler is moved.

Please provide the steps to reproduce if possible:

  1. Clone the repo
  2. Go to a sample
  3. Instead of initialising the scheduler directy with an initialiser, define the scheduler first and the assign to it:
    
    auto scheduler =
    Configurator()
    ...

|| V

NovelRT::Ecs::SystemScheduler scheduler; scheduler = Configurator() ...



4. Build and run 
5. Observe crash

**Expected behaviour:**
It should not crash in this case, or this use of SystemScheduler should be disabled.

**Please tell us about your environment:**  
N/A

**Additional context:**
I did attempt to remove the threads shut down in the move function, but this just resulted in a crash in ucrtbased with no useful stack information. I do wonder about there being two SystemScheduler s in existence briefly. One is constructed with its default constructor, and the second with arguments - I have a feeling that this is not intended behaviour or that if intended there is some contention between instances of system resources. I did try deleting the default constructor, but this caused compile issues elsewhere in the code.

I am working around this for now by simply putting SystemScheduler in a unique_ptr and initialising in one go with make_unique  
RubyNova commented 7 months ago

What's most likely happening is that its trying to stop threads from spinning that never started. When the scheduler moves it has to terminate any spinning threads because std::thread objects cannot be moved from what I recall, which creates a whole hassle of issues.

I'll look into it later today.