From 9c34ef87204be4b5484822faa382c717af60df09 Mon Sep 17 00:00:00 2001 From: elsid Date: Tue, 27 Dec 2022 22:24:28 +0100 Subject: [PATCH] Fix hour modulo expression Round result of std::fmod(hours, 24) to the nearest float below 24 on double to float conversion when it is not. Add special type and conversion function along with tests to be used in all places where such conversion happens. To avoid producing hours equal to 24 due to double to float precision loss. --- CHANGELOG.md | 1 + apps/openmw/mwworld/datetimemanager.cpp | 9 +-- apps/openmw/mwworld/duration.hpp | 39 +++++++++++ apps/openmw/mwworld/timestamp.cpp | 9 +-- apps/openmw_test_suite/CMakeLists.txt | 4 ++ .../mwworld/testduration.cpp | 63 +++++++++++++++++ .../mwworld/testtimestamp.cpp | 67 +++++++++++++++++++ 7 files changed, 184 insertions(+), 8 deletions(-) create mode 100644 apps/openmw/mwworld/duration.hpp create mode 100644 apps/openmw_test_suite/mwworld/testduration.cpp create mode 100644 apps/openmw_test_suite/mwworld/testtimestamp.cpp 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); + } + } +}