Merge branch 'fix_hour_modulo' into 'master'

Fix hour modulo expression (#7121)

Closes #7121

See merge request OpenMW/openmw!2556

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