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

@@ -28,6 +28,7 @@
     Bug #7042: Weapon follow animations that immediately follow the hit animations cause multiple hits
     Bug #7044: Changing a class' services does not affect autocalculated NPCs
     Bug #7084: Resurrecting an actor doesn't take into account base record changes
+    Bug #7121: Crash on TimeStamp construction with invalid hour value
     Feature #6447: Add LOD support to Object Paging
     Feature #6933: Support high-resolution cursor textures
     Feature #6945: Support S3TC-compressed and BGR/BGRA NiPixelData

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"
@@ -60,11 +61,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)
@@ -31,11 +33,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);
+        }
+    }
+}