Index: source/graphics/TextureConverter.h =================================================================== --- source/graphics/TextureConverter.h +++ source/graphics/TextureConverter.h @@ -25,12 +25,32 @@ #if CONFIG2_NVTT #include "ps/Future.h" +#include #include #include #endif class MD5; +/** + * The exception which is thrown if the conversion fails. + */ +class ConversionError : public std::exception +{ +public: + ConversionError(CTexturePtr tex) : + texture{std::move(tex)} + {} + + CTexturePtr GetTexture() + { + return texture; + } + +private: + CTexturePtr texture; +}; + /** * Texture conversion helper class. * Provides an asynchronous API to convert input image files into compressed DDS, @@ -194,7 +214,7 @@ * If the conversion failed, it sets ok to false and doesn't touch the other arguments, * then returns true. */ - bool Poll(CTexturePtr& texture, VfsPath& dest, bool& ok); + bool Poll(CTexturePtr& texture, VfsPath& dest); /** * The TextureConverter schouldn't utilize the CPU/TaskManager fully, so Index: source/graphics/TextureConverter.cpp =================================================================== --- source/graphics/TextureConverter.cpp +++ source/graphics/TextureConverter.cpp @@ -97,7 +97,11 @@ VfsPath dest; CTexturePtr texture; BufferOutputHandler output; - bool ret; // true if the conversion succeeded +}; + +struct WriteFileError : ConversionError +{ + using ConversionError::ConversionError; }; #endif // CONFIG2_NVTT @@ -475,8 +479,10 @@ // Perform the compression nvtt::Compressor compressor; - result->ret = compressor.process(request->inputOptions, request->compressionOptions, + const auto ret = compressor.process(request->inputOptions, request->compressionOptions, request->outputOptions); + if (!ret) + throw ConversionError{request->texture}; return result; }, Threading::TaskPriority::LOW)); @@ -489,7 +495,7 @@ #endif // !CONFIG2_NVTT } -bool CTextureConverter::Poll(CTexturePtr& texture, VfsPath& dest, bool& ok) +bool CTextureConverter::Poll(CTexturePtr& texture, VfsPath& dest) { #if CONFIG2_NVTT if (m_ResultQueue.empty() || !m_ResultQueue.front().IsReady()) @@ -501,29 +507,17 @@ std::unique_ptr result = m_ResultQueue.front().Get(); m_ResultQueue.pop(); - if (!result->ret) - { - // conversion had failed - ok = false; - return true; - } - // Move output into a correctly-aligned buffer size_t size = result->output.buffer.size(); std::shared_ptr file; AllocateAligned(file, size, maxSectorSize); memcpy(file.get(), &result->output.buffer[0], size); if (m_VFS->CreateFile(result->dest, file, size) < 0) - { - // error writing file - ok = false; - return true; - } + throw WriteFileError{result->texture}; // Succeeded in converting texture texture = result->texture; dest = result->dest; - ok = true; return true; #else // CONFIG2_NVTT Index: source/graphics/TextureManager.cpp =================================================================== --- source/graphics/TextureManager.cpp +++ source/graphics/TextureManager.cpp @@ -677,9 +677,16 @@ { CTexturePtr textureOut; VfsPath dest; - bool ok; - if (m_TextureConverter.Poll(textureOut, dest, ok)) - return ok; + + try + { + if (m_TextureConverter.Poll(textureOut, dest)) + return true; + } + catch (ConversionError& e) + { + return false; + } std::this_thread::sleep_for(std::chrono::microseconds(1)); } @@ -697,18 +704,22 @@ CTexturePtr texture; VfsPath dest; bool ok; - if (m_TextureConverter.Poll(texture, dest, ok)) + try { - if (ok) + if (m_TextureConverter.Poll(texture, dest)) { LoadTexture(texture, dest); + texture->m_State = CTexture::LOADED; + return true; } - else - { - LOGERROR("Texture failed to convert: \"%s\"", texture->m_Properties.m_Path.string8()); - texture->ResetBackendTexture( - nullptr, m_ErrorTexture.GetTexture()->GetBackendTexture()); - } + } + catch (ConversionError& e) + { + LOGERROR("Texture failed to convert: \"%s\"", + e.GetTexture()->m_Properties.m_Path.string8()); + e.GetTexture()->ResetBackendTexture(nullptr, + m_ErrorTexture.GetTexture()->GetBackendTexture()); + texture->m_State = CTexture::LOADED; return true; } Index: source/graphics/tests/test_TextureConverter.h =================================================================== --- source/graphics/tests/test_TextureConverter.h +++ source/graphics/tests/test_TextureConverter.h @@ -58,12 +58,10 @@ for (size_t i = 0; i < 100; ++i) { CTexturePtr texture; - bool ok; - if (converter.Poll(texture, dest, ok)) - { - TS_ASSERT(ok); + bool didSomething; + TS_ASSERT_THROWS_NOTHING(didSomething = converter.Poll(texture, dest)) + if (didSomething) break; - } SDL_Delay(10); } Index: source/ps/Future.h =================================================================== --- source/ps/Future.h +++ source/ps/Future.h @@ -22,6 +22,7 @@ #include #include +#include #include #include #include @@ -48,12 +49,11 @@ * Holds all relevant data. */ template -class SharedState : public ResultHolder +class SharedState { static constexpr bool VoidResult = std::is_same_v; public: SharedState(std::function&& func) : - ResultHolder{std::nullopt}, m_Func(std::move(func)) {} ~SharedState() @@ -98,7 +98,7 @@ if (m_Status == Status::DONE) m_Status = Status::CANCELED; if constexpr (!VoidResult) - this->reset(); + std::get<0>(m_Outcome).reset(); m_ConditionVariable.notify_all(); return cancelled; } @@ -108,21 +108,35 @@ /** * Move the result away from the shared state, mark the future invalid. */ - template - std::enable_if_t, ResultType> GetResult() + ResultType GetResult() { // The caller must ensure that this is only called if we have a result. - ENSURE(this->has_value()); + if constexpr (!std::is_void_v) + ENSURE(std::get<0>(m_Outcome).has_value() || std::get<1>(m_Outcome)); + m_Status = Status::CANCELED; - ResultType ret = std::move(**this); - this->reset(); - return ret; + + if (std::get<1>(m_Outcome)) + std::rethrow_exception(std::exchange(std::get<1>(m_Outcome), {})); + + if constexpr (std::is_void_v) + return; + else + { + ResultType ret = std::move(*std::get<0>(m_Outcome)); + std::get<0>(m_Outcome).reset(); + return ret; + } } std::atomic m_Status = Status::PENDING; std::mutex m_Mutex; std::condition_variable m_ConditionVariable; + // There cant be a result and a value at the same time. + // TODO: Use std::variant. + std::tuple, std::exception_ptr> m_Outcome{std::nullopt, + std::exception_ptr{}}; std::function m_Func; }; @@ -169,21 +183,14 @@ * If the future is not complete, calls Wait(). * If the future is canceled, asserts. */ - template - std::enable_if_t, ResultType> Get() + ResultType Get() { ENSURE(!!m_SharedState); Wait(); - if constexpr (VoidResult) - return; - else - { - ENSURE(m_SharedState->m_Status != Status::CANCELED); - - // This mark the state invalid - can't call Get again. - return m_SharedState->GetResult(); - } + ENSURE(m_SharedState->m_Status != Status::CANCELED); + // This mark the state invalid - can't call Get again. + return m_SharedState->GetResult(); } /** @@ -246,10 +253,18 @@ if (!m_SharedState->m_Status.compare_exchange_strong(expected, Future::Status::STARTED)) return; - if constexpr (VoidResult) - m_SharedState->m_Func(); - else - m_SharedState->emplace(m_SharedState->m_Func()); + try + { + if constexpr (VoidResult) + m_SharedState->m_Func(); + else + std::get<0>(m_SharedState->m_Outcome).emplace(m_SharedState->m_Func()); + + } + catch(...) + { + std::get<1>(m_SharedState->m_Outcome) = std::current_exception(); + } // Because we might have threads waiting on us, we need to make sure that they either: // - don't wait on our condition variable Index: source/ps/tests/test_Future.h =================================================================== --- source/ps/tests/test_Future.h +++ source/ps/tests/test_Future.h @@ -19,6 +19,7 @@ #include "ps/Future.h" +#include #include #include @@ -132,4 +133,60 @@ task2(); TS_ASSERT_EQUALS(future.Get(), 7); } + + void test_exception() + { + struct TestException : std::exception + { + using std::exception::exception; + }; + + { + Future future; + auto packedTask = future.Wrap([] + { + throw TestException{}; + }); + + packedTask(); + TS_ASSERT(future.IsReady()); + TS_ASSERT_THROWS(future.Get(), TestException&); + } + + { + Future future; + auto packedTask = future.Wrap([]() -> int + { + throw TestException{}; + }); + + packedTask(); + TS_ASSERT(future.IsReady()); + TS_ASSERT_THROWS(future.Get(), TestException&); + } + + // If the function does not throw but it's the cause something is thrown the exception should + // be reported to the thread which is waiting on the future. + { + class ThrowsOnMove + { + public: + ThrowsOnMove() = default; + ThrowsOnMove(ThrowsOnMove&&) + { + throw TestException{}; + } + }; + + Future future; + auto packedTask = future.Wrap([] + { + return ThrowsOnMove{}; + }); + + packedTask(); + TS_ASSERT(future.IsReady()); + TS_ASSERT_THROWS(future.Get(), TestException&); + } + } }; Index: source/simulation2/components/CCmpPathfinder.cpp =================================================================== --- source/simulation2/components/CCmpPathfinder.cpp +++ source/simulation2/components/CCmpPathfinder.cpp @@ -800,10 +800,9 @@ m_ShortPathRequests.Compute(*this, m_VertexPathfinders.front()); m_LongPathRequests.Compute(*this, *m_LongPathfinder); } - // We're done, clear futures. - // Use CancelOrWait instead of just Cancel to ensure determinism. + // We're done, get the exception from the futures. for (Future& future : m_Futures) - future.CancelOrWait(); + future.Get(); { PROFILE2("PostMessages");