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

@@ -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"

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<int>(hour / 24);
-        hour = std::fmod(hour, 24);
-        mGameHour = static_cast<float>(hour);
+        const Duration duration = Duration::fromHours(hour);
 
-        if (days > 0)
+        mGameHour = duration.getHours();
+
+        if (const int days = duration.getDays(); days > 0)
             setDay(days + mDay);
     }
 

apps/openmw/mwworld/duration.hpp

@@ -0,0 +1,39 @@
+#ifndef GAME_MWWORLD_DURATION_H
+#define GAME_MWWORLD_DURATION_H
+
+#include <cmath>
+#include <stdexcept>
+
+namespace MWWorld
+{
+    inline const double maxFloatHour = static_cast<double>(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<int>(hours / 24), static_cast<float>(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

apps/openmw/mwworld/timestamp.cpp

@@ -6,6 +6,8 @@
 
 #include <components/esm/defs.hpp>
 
+#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<float> (std::fmod (hours, 24));
-
-        mDay += static_cast<int>(hours / 24);
+        mHour = duration.getHours();
+        mDay += duration.getDays();
 
         return *this;
     }

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
 

apps/openmw_test_suite/mwworld/testduration.cpp

@@ -0,0 +1,63 @@
+#include <gtest/gtest.h>
+
+#include <cmath>
+
+#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<double>::min());
+            EXPECT_EQ(duration.getDays(), 0);
+            EXPECT_EQ(duration.getHours(), 0);
+        }
+
+        TEST(MWWorldDurationTest, fromHoursShouldProduceZeroDaysAndSomeHoursForMinFloat)
+        {
+            const Duration duration = Duration::fromHours(std::numeric_limits<float>::min());
+            EXPECT_EQ(duration.getDays(), 0);
+            EXPECT_GT(duration.getHours(), 0);
+            EXPECT_FLOAT_EQ(duration.getHours(), std::numeric_limits<float>::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);
+        }
+    }
+}

apps/openmw_test_suite/mwworld/testtimestamp.cpp

@@ -0,0 +1,67 @@
+#include <gtest/gtest.h>
+
+#include <cmath>
+
+#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);
+        }
+    }
+}