From 25ead80d8b4655d974f01e1be3af4cbc70b857cf 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 9eb66475bb..55f2185763 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/apps/openmw/mwworld/datetimemanager.cpp b/apps/openmw/mwworld/datetimemanager.cpp index 2d137518cf..f2dd55d9dd 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" @@ -60,11 +61,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 22fa504d45..c21d90f285 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) @@ -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(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 81f6fadabe..cf1be3a18a 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); + } + } +}