diff --git a/CHANGELOG.md b/CHANGELOG.md index 43ffadd808..73e5c797a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -156,6 +156,7 @@ Bug #6923: Dispose of corpse prevents respawning after load Bug #6937: Divided by Nix Hounds quest is broken Bug #7008: Race condition on initializing a vector of reserved node names + Bug #7121: Crash on TimeStamp construction with invalid hour value Feature #890: OpenMW-CS: Column filtering Feature #1465: "Reset" argument for AI functions Feature #2491: Ability to make OpenMW "portable" diff --git a/apps/openmw/mwworld/datetimemanager.cpp b/apps/openmw/mwworld/datetimemanager.cpp index 67d1ce1fbc..65cad51dff 100644 --- a/apps/openmw/mwworld/datetimemanager.cpp +++ b/apps/openmw/mwworld/datetimemanager.cpp @@ -3,6 +3,7 @@ #include "../mwbase/environment.hpp" #include "../mwbase/world.hpp" +#include "duration.hpp" #include "esmstore.hpp" #include "globals.hpp" #include "timestamp.hpp" @@ -48,11 +49,11 @@ namespace MWWorld if (hour < 0) hour = 0; - int days = static_cast(hour / 24); - hour = std::fmod(hour, 24); - mGameHour = static_cast(hour); + const Duration duration = Duration::fromHours(hour); - if (days > 0) + mGameHour = duration.getHours(); + + if (const int days = duration.getDays(); days > 0) setDay(days + mDay); } diff --git a/apps/openmw/mwworld/duration.hpp b/apps/openmw/mwworld/duration.hpp new file mode 100644 index 0000000000..78693ca0dd --- /dev/null +++ b/apps/openmw/mwworld/duration.hpp @@ -0,0 +1,39 @@ +#ifndef GAME_MWWORLD_DURATION_H +#define GAME_MWWORLD_DURATION_H + +#include +#include + +namespace MWWorld +{ + inline const double maxFloatHour = static_cast(std::nextafter(24.0f, 0.0f)); + + class Duration + { + public: + static Duration fromHours(double hours) + { + if (hours < 0) + throw std::runtime_error("Negative hours is not supported Duration"); + + return Duration( + static_cast(hours / 24), static_cast(std::min(std::fmod(hours, 24), maxFloatHour))); + } + + int getDays() const { return mDays; } + + float getHours() const { return mHours; } + + private: + int mDays; + float mHours; + + explicit Duration(int days, float hours) + : mDays(days) + , mHours(hours) + { + } + }; +} + +#endif diff --git a/apps/openmw/mwworld/timestamp.cpp b/apps/openmw/mwworld/timestamp.cpp index 82a247a214..f49248826b 100644 --- a/apps/openmw/mwworld/timestamp.cpp +++ b/apps/openmw/mwworld/timestamp.cpp @@ -6,6 +6,8 @@ #include +#include "duration.hpp" + namespace MWWorld { TimeStamp::TimeStamp (float hour, int day) @@ -30,11 +32,10 @@ namespace MWWorld if (hours<0) throw std::runtime_error ("can't move time stamp backwards in time"); - hours += mHour; + const Duration duration = Duration::fromHours(mHour + hours); - mHour = static_cast (std::fmod (hours, 24)); - - mDay += static_cast(hours / 24); + mHour = duration.getHours(); + mDay += duration.getDays(); return *this; } diff --git a/apps/openmw_test_suite/CMakeLists.txt b/apps/openmw_test_suite/CMakeLists.txt index 40d9c57b13..a90624d7de 100644 --- a/apps/openmw_test_suite/CMakeLists.txt +++ b/apps/openmw_test_suite/CMakeLists.txt @@ -11,7 +11,11 @@ file(GLOB UNITTEST_SRC_FILES ../openmw/mwworld/store.cpp ../openmw/mwworld/esmstore.cpp + ../openmw/mwworld/timestamp.cpp + mwworld/test_store.cpp + mwworld/testduration.cpp + mwworld/testtimestamp.cpp mwdialogue/test_keywordsearch.cpp diff --git a/apps/openmw_test_suite/mwworld/testduration.cpp b/apps/openmw_test_suite/mwworld/testduration.cpp new file mode 100644 index 0000000000..ce48397403 --- /dev/null +++ b/apps/openmw_test_suite/mwworld/testduration.cpp @@ -0,0 +1,63 @@ +#include + +#include + +#include "apps/openmw/mwworld/duration.hpp" + +namespace MWWorld +{ + namespace + { + TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndHoursFor0) + { + const Duration duration = Duration::fromHours(0); + EXPECT_EQ(duration.getDays(), 0); + EXPECT_EQ(duration.getHours(), 0); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceOneDayAndZeroHoursFor24) + { + const Duration duration = Duration::fromHours(24); + EXPECT_EQ(duration.getDays(), 1); + EXPECT_EQ(duration.getHours(), 0); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceOneDayAndRemainderHoursFor42) + { + const Duration duration = Duration::fromHours(42); + EXPECT_EQ(duration.getDays(), 1); + EXPECT_EQ(duration.getHours(), 18); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndZeroHoursForMinDouble) + { + const Duration duration = Duration::fromHours(std::numeric_limits::min()); + EXPECT_EQ(duration.getDays(), 0); + EXPECT_EQ(duration.getHours(), 0); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndSomeHoursForMinFloat) + { + const Duration duration = Duration::fromHours(std::numeric_limits::min()); + EXPECT_EQ(duration.getDays(), 0); + EXPECT_GT(duration.getHours(), 0); + EXPECT_FLOAT_EQ(duration.getHours(), std::numeric_limits::min()); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndRemainderHoursForValueJustBelow24InDoublePrecision) + { + const Duration duration = Duration::fromHours(std::nextafter(24.0, 0.0)); + EXPECT_EQ(duration.getDays(), 0); + EXPECT_LT(duration.getHours(), 24); + EXPECT_FLOAT_EQ(duration.getHours(), 24); + } + + TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndRemainderHoursForValueJustBelow24InFloatPrecision) + { + const Duration duration = Duration::fromHours(std::nextafter(24.0f, 0.0f)); + EXPECT_EQ(duration.getDays(), 0); + EXPECT_LT(duration.getHours(), 24); + EXPECT_FLOAT_EQ(duration.getHours(), 24); + } + } +} diff --git a/apps/openmw_test_suite/mwworld/testtimestamp.cpp b/apps/openmw_test_suite/mwworld/testtimestamp.cpp new file mode 100644 index 0000000000..8a1a4c2aa8 --- /dev/null +++ b/apps/openmw_test_suite/mwworld/testtimestamp.cpp @@ -0,0 +1,67 @@ +#include + +#include + +#include "apps/openmw/mwworld/timestamp.hpp" + +namespace MWWorld +{ + namespace + { + TEST(MWWorldTimeStampTest, operatorPlusShouldNotChangeTimeStampForZero) + { + TimeStamp timeStamp; + timeStamp += 0; + EXPECT_EQ(timeStamp.getDay(), 0); + EXPECT_EQ(timeStamp.getHour(), 0); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldProperlyHandleDoubleValuesCloseTo24) + { + TimeStamp timeStamp; + timeStamp += std::nextafter(24.0, 0.0); + EXPECT_EQ(timeStamp.getDay(), 0); + EXPECT_LT(timeStamp.getHour(), 24); + EXPECT_FLOAT_EQ(timeStamp.getHour(), 24); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldProperlyHandleFloatValuesCloseTo24) + { + TimeStamp timeStamp; + timeStamp += std::nextafter(24.0f, 0.0f); + EXPECT_EQ(timeStamp.getDay(), 0); + EXPECT_LT(timeStamp.getHour(), 24); + EXPECT_FLOAT_EQ(timeStamp.getHour(), 24); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldAddDaysForEach24Hours) + { + TimeStamp timeStamp; + timeStamp += 24.0 * 42; + EXPECT_EQ(timeStamp.getDay(), 42); + EXPECT_EQ(timeStamp.getHour(), 0); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldAddDaysForEach24HoursAndSetRemainderToHours) + { + TimeStamp timeStamp; + timeStamp += 24.0 * 42 + 13.0; + EXPECT_EQ(timeStamp.getDay(), 42); + EXPECT_EQ(timeStamp.getHour(), 13); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldAccumulateExistingValue) + { + TimeStamp timeStamp(13, 42); + timeStamp += 24.0 * 2 + 17.0; + EXPECT_EQ(timeStamp.getDay(), 45); + EXPECT_EQ(timeStamp.getHour(), 6); + } + + TEST(MWWorldTimeStampTest, operatorPlusShouldThrowExceptionForNegativeValue) + { + TimeStamp timeStamp(13, 42); + EXPECT_THROW(timeStamp += -1, std::runtime_error); + } + } +}