diff --git a/Source/engine/animationinfo.cpp b/Source/engine/animationinfo.cpp index 1bdfc016d..6215000a0 100644 --- a/Source/engine/animationinfo.cpp +++ b/Source/engine/animationinfo.cpp @@ -27,16 +27,16 @@ int8_t AnimationInfo::getFrameToUseForRendering() const if (currentFrame >= relevantFramesForDistributing_) return currentFrame; - int16_t ticksSinceSequenceStarted = ticksSinceSequenceStarted_; + TicksFixed ticksSinceSequenceStarted = ticksSinceSequenceStarted_; if (ticksSinceSequenceStarted_ < 0) { - ticksSinceSequenceStarted = 0; - Log("getFrameToUseForRendering: Invalid ticksSinceSequenceStarted_ {}", ticksSinceSequenceStarted_); + ticksSinceSequenceStarted = TicksFixed::fromInt(0); + Log("getFrameToUseForRendering: Invalid ticksSinceSequenceStarted_ {}", ticksSinceSequenceStarted_.raw()); } // we don't use the processed game ticks alone but also the fraction of the next game tick (if a rendering happens between game ticks). This helps to smooth the animations. - const int32_t totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted; + const FixedPoint totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted; - auto absoluteAnimationFrame = static_cast(totalTicksForCurrentAnimationSequence * tickModifier_ / baseValueFraction / baseValueFraction); + auto absoluteAnimationFrame = static_cast(totalTicksForCurrentAnimationSequence.multiplyWithoutWidening(tickModifier_).whole()); if (skippedFramesFromPreviousAnimation_ > 0) { // absoluteAnimationFrames contains also the Frames from the previous Animation, so if we want to get the current Frame we have to remove them absoluteAnimationFrame -= skippedFramesFromPreviousAnimation_; @@ -61,8 +61,8 @@ int8_t AnimationInfo::getFrameToUseForRendering() const uint8_t AnimationInfo::getAnimationProgress() const { - int16_t ticksSinceSequenceStarted = std::max(0, ticksSinceSequenceStarted_); - int32_t tickModifier = tickModifier_; + TicksFixed ticksSinceSequenceStarted = std::max(TicksFixed::fromInt(0), ticksSinceSequenceStarted_); + RateFixed tickModifier = tickModifier_; if (relevantFramesForDistributing_ <= 0) { if (ticksPerFrame <= 0) { @@ -71,13 +71,15 @@ uint8_t AnimationInfo::getAnimationProgress() const } // This logic is used if animation distribution is not active (see getFrameToUseForRendering). // In this case the variables calculated with animation distribution are not initialized and we have to calculate them on the fly with the given information. - ticksSinceSequenceStarted = ((currentFrame * ticksPerFrame) + tickCounterOfCurrentFrame) * baseValueFraction; - tickModifier = baseValueFraction / ticksPerFrame; + ticksSinceSequenceStarted = TicksFixed::fromInt((currentFrame * ticksPerFrame) + tickCounterOfCurrentFrame); + tickModifier = RateFixed(RateFixed::fromInt(1) / ticksPerFrame); } - const int32_t totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted; - const int32_t progressInAnimationFrames = totalTicksForCurrentAnimationSequence * tickModifier; - const int32_t animationFraction = progressInAnimationFrames / numberOfFrames / baseValueFraction; + const FixedPoint totalTicksForCurrentAnimationSequence = getProgressToNextGameTick() + ticksSinceSequenceStarted; + const FixedPoint progressInAnimationFrames = totalTicksForCurrentAnimationSequence.multiplyWithoutWidening(tickModifier); + // The returned fraction stays in the same baseValueFraction-raw scale (not a whole number): + // the fixed*fixed multiply above already performs the one rescale this needs. + const int animationFraction = (progressInAnimationFrames / numberOfFrames).raw(); assert(animationFraction <= baseValueFraction); return static_cast(animationFraction); } @@ -102,9 +104,9 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb currentFrame = numSkippedFrames; tickCounterOfCurrentFrame = 0; this->ticksPerFrame = ticksPerFrame; - ticksSinceSequenceStarted_ = 0; + ticksSinceSequenceStarted_ = TicksFixed::fromInt(0); relevantFramesForDistributing_ = 0; - tickModifier_ = 0; + tickModifier_ = RateFixed::fromInt(0); isPetrified = false; if (numSkippedFrames != 0 || flags != AnimationDistributionFlags::None) { @@ -133,7 +135,7 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb // The Animation Distribution Logic needs to account how many game ticks passed since the Animation started. // Because processAnimation will increase this later (in same game tick as setNewAnimation), we correct this upfront. // This also means Rendering should never happen with ticksSinceSequenceStarted_ < 0. - ticksSinceSequenceStarted_ = -baseValueFraction; + ticksSinceSequenceStarted_ = TicksFixed::fromInt(-1); } if ((flags & AnimationDistributionFlags::SkipsDelayOfLastFrame) == AnimationDistributionFlags::SkipsDelayOfLastFrame) { @@ -159,25 +161,30 @@ void AnimationInfo::setNewAnimation(OptionalClxSpriteList celSprite, int8_t numb // The truncated Frames from previous Animation will also be shown, so we also have to distribute them for the given time (game ticks) relevantAnimationTicksForDistribution += (skippedFramesFromPreviousAnimation_ * ticksPerFrame); - // At this point we use fixed point math for the fragment calculations - relevantAnimationTicksForDistribution *= baseValueFraction; - relevantAnimationTicksWithSkipping *= baseValueFraction; + // At this point we use fixed point math for the fragment calculations. + // relevantAnimationTicksForDistribution/WithSkipping are bounded by int8_t products + // (numberOfFrames/ticksPerFrame are each at most int8_t), so an int32_t-backed FixedPoint + // here has enough headroom on its own; we use divideWithoutWidening below to avoid + // promoting to a 64-bit intermediate that this bounded ratio never needs. + using WideTicksFixed = FixedPoint; + WideTicksFixed relevantTicksForDistribution = WideTicksFixed::fromInt(relevantAnimationTicksForDistribution); + WideTicksFixed relevantTicksWithSkipping = WideTicksFixed::fromInt(relevantAnimationTicksWithSkipping); // The preview animation was shown some times (less than one game tick) // So we overall have a longer time the animation is shown - ticksSinceSequenceStarted_ += previewShownGameTickFragments; - relevantAnimationTicksWithSkipping += previewShownGameTickFragments; + ticksSinceSequenceStarted_ += TicksFixed::fromRaw(previewShownGameTickFragments); + relevantTicksWithSkipping += WideTicksFixed::fromRaw(previewShownGameTickFragments); // if we skipped Frames we need to expand the game ticks to make one game tick for this Animation "faster" - int32_t tickModifier = 0; - if (relevantAnimationTicksWithSkipping != 0) - tickModifier = baseValueFraction * relevantAnimationTicksForDistribution / relevantAnimationTicksWithSkipping; + WideTicksFixed tickModifier = WideTicksFixed::fromInt(0); + if (relevantTicksWithSkipping != 0) + tickModifier = relevantTicksForDistribution.divideWithoutWidening(relevantTicksWithSkipping); // tickModifier specifies the Animation fraction per game tick, so we have to remove the delay from the variable tickModifier /= ticksPerFrame; relevantFramesForDistributing_ = relevantAnimationFramesForDistributing; - tickModifier_ = static_cast(tickModifier); + tickModifier_ = RateFixed(tickModifier); } } @@ -192,9 +199,9 @@ void AnimationInfo::changeAnimationData(OptionalClxSpriteList celSprite, int8_t this->numberOfFrames = numberOfFrames; this->ticksPerFrame = ticksPerFrame; - ticksSinceSequenceStarted_ = 0; + ticksSinceSequenceStarted_ = TicksFixed::fromInt(0); relevantFramesForDistributing_ = 0; - tickModifier_ = 0; + tickModifier_ = RateFixed::fromInt(0); } this->sprites = celSprite; } @@ -202,30 +209,30 @@ void AnimationInfo::changeAnimationData(OptionalClxSpriteList celSprite, int8_t void AnimationInfo::processAnimation(bool reverseAnimation /*= false*/) { tickCounterOfCurrentFrame++; - ticksSinceSequenceStarted_ += baseValueFraction; + ticksSinceSequenceStarted_ += TicksFixed::fromInt(1); if (tickCounterOfCurrentFrame >= ticksPerFrame) { tickCounterOfCurrentFrame = 0; if (reverseAnimation) { --currentFrame; if (currentFrame == -1) { currentFrame = numberOfFrames - 1; - ticksSinceSequenceStarted_ = 0; + ticksSinceSequenceStarted_ = TicksFixed::fromInt(0); } } else { ++currentFrame; if (currentFrame >= numberOfFrames) { currentFrame = 0; - ticksSinceSequenceStarted_ = 0; + ticksSinceSequenceStarted_ = TicksFixed::fromInt(0); } } } } -uint8_t AnimationInfo::getProgressToNextGameTick() const +FixedPoint AnimationInfo::getProgressToNextGameTick() const { if (isPetrified) - return 0; - return ProgressToNextGameTick; + return FixedPoint::fromRaw(0); + return FixedPoint::fromRaw(ProgressToNextGameTick); } } // namespace devilution diff --git a/Source/engine/animationinfo.h b/Source/engine/animationinfo.h index ae0947f7a..c6ca66531 100644 --- a/Source/engine/animationinfo.h +++ b/Source/engine/animationinfo.h @@ -9,6 +9,7 @@ #include #include "engine/clx_sprite.hpp" +#include "utils/fixed_point.hpp" namespace devilution { @@ -109,15 +110,28 @@ public: void processAnimation(bool reverseAnimation = false); /** - * @brief Fractions in AnimationInfo are stored as fixed point (baseValueFraction/128 correspondents to 1/100%). + * @brief The scale of the raw fixed point fractions still used at some boundaries (see + * getAnimationProgress() and previewShownGameTickFragments); baseValueFraction/128 + * corresponds to 1/100%. Internally, AnimationInfo uses FixedPoint<..., 7> instead, which is + * the same scale expressed as a type rather than a raw value. */ constexpr static uint8_t baseValueFraction = 128; private: /** - * @brief returns the progress as a fraction in time to the next game tick or no progress if the animation is frozen (see baseValueFraction) + * @brief A fixed point count of game ticks, signed because ProcessAnimationPending + * distribution deliberately starts it out negative. */ - [[nodiscard]] uint8_t getProgressToNextGameTick() const; + using TicksFixed = FixedPoint; + /** + * @brief A fixed point rate of animation-fractions advanced per game tick. + */ + using RateFixed = FixedPoint; + + /** + * @brief returns the progress as a fraction in time to the next game tick or no progress if the animation is frozen + */ + [[nodiscard]] FixedPoint getProgressToNextGameTick() const; /** * @brief Animation Frames that will be adjusted for the skipped Frames/game ticks @@ -128,13 +142,13 @@ private: */ int8_t skippedFramesFromPreviousAnimation_; /** - * @brief Specifies how many animations-fractions (see baseValueFraction) are displayed between two game ticks. this can be more than one frame, if animations are skipped or less than one frame if the same animation is shown in multiple times (delay specified). + * @brief Specifies how many animation-fractions are displayed between two game ticks. this can be more than one frame, if animations are skipped or less than one frame if the same animation is shown in multiple times (delay specified). */ - uint16_t tickModifier_; + RateFixed tickModifier_; /** * @brief Number of game ticks after the current animation sequence started */ - int16_t ticksSinceSequenceStarted_; + TicksFixed ticksSinceSequenceStarted_; }; } // namespace devilution diff --git a/Source/utils/fixed_point.hpp b/Source/utils/fixed_point.hpp index d3db58801..5b205ec25 100644 --- a/Source/utils/fixed_point.hpp +++ b/Source/utils/fixed_point.hpp @@ -130,6 +130,53 @@ public: return *this; } + /** + * @brief Multiplies by another fixed point value like `operator*`, but without the extra wide intermediate. + * + * Computes entirely in `Promoted` (the type `raw() * factor.raw()` would naturally have), with + * no wider scratch type. Only use this where the caller can already show that the pre-rescale + * product of the two raw values fits in `Promoted` - otherwise the result is silently wrong, not + * just imprecise. Prefer `operator*` unless that guarantee holds and the wider intermediate's + * cost (e.g. a 64-bit multiply on a 32-bit target) actually matters here. + */ + template + [[nodiscard]] DVL_ALWAYS_INLINE constexpr auto multiplyWithoutWidening(FixedPoint factor) const + { + using Promoted = decltype(raw_ * factor.raw()); + const Promoted product = static_cast(raw_) * static_cast(factor.raw()); + return FixedPoint::fromRaw(static_cast(product >> FractionalBits)); + } + + /** + * @brief Divides by another fixed point value, rescaling the result and narrowing it back to this type. + */ + template + DVL_ALWAYS_INLINE constexpr FixedPoint &operator/=(FixedPoint divisor) + { + using Promoted = decltype(raw_ / divisor.raw()); + using Wide = fixed_point_detail::DoubleWidth; + const Wide dividend = static_cast(raw_) << FractionalBits; + raw_ = static_cast(dividend / static_cast(divisor.raw())); + return *this; + } + + /** + * @brief Divides by another fixed point value like `operator/`, but without the extra wide intermediate. + * + * Computes entirely in `Promoted` (the type `raw() / divisor.raw()` would naturally have), with + * no wider scratch type. Only use this where the caller can already show that this value's raw + * value shifted left by `FractionalBits` fits in `Promoted` - otherwise the result is silently + * wrong, not just imprecise. Prefer `operator/` unless that guarantee holds and the wider + * intermediate's cost (e.g. a 64-bit division on a 32-bit target) actually matters here. + */ + template + [[nodiscard]] DVL_ALWAYS_INLINE constexpr auto divideWithoutWidening(FixedPoint divisor) const + { + using Promoted = decltype(raw_ / divisor.raw()); + const Promoted dividend = static_cast(raw_) << FractionalBits; + return FixedPoint::fromRaw(static_cast(dividend / static_cast(divisor.raw()))); + } + template [[nodiscard]] DVL_ALWAYS_INLINE constexpr bool operator==(FixedPoint other) const { return raw_ == other.raw(); } template @@ -197,6 +244,22 @@ template return FixedPoint::fromRaw(static_cast(product >> FractionalBits)); } +/** + * @brief Divides two fixed point values, rescaling the result. + * + * The result is backed by whatever storage type dividing the two raw storage types would + * naturally promote to (see the class documentation), computed using an even wider intermediate + * so scaling the dividend up before dividing doesn't overflow. + */ +template +[[nodiscard]] DVL_ALWAYS_INLINE constexpr auto operator/(FixedPoint a, FixedPoint b) +{ + using Promoted = decltype(a.raw() / b.raw()); + using Wide = fixed_point_detail::DoubleWidth; + const Wide dividend = static_cast(a.raw()) << FractionalBits; + return FixedPoint::fromRaw(static_cast(dividend / static_cast(b.raw()))); +} + template [[nodiscard]] DVL_ALWAYS_INLINE constexpr auto operator-(FixedPoint a) { diff --git a/test/fixed_point_test.cpp b/test/fixed_point_test.cpp index eeaf6be49..1a858238b 100644 --- a/test/fixed_point_test.cpp +++ b/test/fixed_point_test.cpp @@ -95,6 +95,36 @@ TEST(FixedPointTest, MultiplyByFixedPointDoesNotOverflowWideStorage) EXPECT_EQ(large * doubled, Fixed26_6::fromInt(4000)); } +TEST(FixedPointTest, MultiplyWithoutWideningMatchesOperatorStarWhenItFits) +{ + const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5 + EXPECT_EQ(Fixed10_6::fromInt(10).multiplyWithoutWidening(half), Fixed10_6::fromInt(5)); + EXPECT_EQ(Fixed10_6::fromInt(3).multiplyWithoutWidening(Fixed10_6::fromInt(4)), Fixed10_6::fromInt(12)); +} + +TEST(FixedPointTest, DivideByFixedPoint) +{ + const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5 + EXPECT_EQ(Fixed10_6::fromInt(10) / half, Fixed10_6::fromInt(20)); + EXPECT_EQ(Fixed10_6::fromInt(12) / Fixed10_6::fromInt(4), Fixed10_6::fromInt(3)); +} + +TEST(FixedPointTest, DivideByFixedPointDoesNotOverflowWideStorage) +{ + const Fixed26_6 large = Fixed26_6::fromInt(2000); + const Fixed26_6 tiny = Fixed26_6::fromRaw(1); // smallest positive representable value + // Scaling `large` up by 2^FractionalBits before dividing would overflow a narrower + // intermediate; this only works out to fromInt(2000 * 64) if the wide intermediate holds. + EXPECT_EQ(large / tiny, Fixed26_6::fromInt(2000 * 64)); +} + +TEST(FixedPointTest, DivideWithoutWideningMatchesOperatorSlashWhenItFits) +{ + const Fixed10_6 half = Fixed10_6::fromRaw(32); // 0.5 + EXPECT_EQ(Fixed10_6::fromInt(10).divideWithoutWidening(half), Fixed10_6::fromInt(20)); + EXPECT_EQ(Fixed10_6::fromInt(12).divideWithoutWidening(Fixed10_6::fromInt(4)), Fixed10_6::fromInt(3)); +} + TEST(FixedPointTest, WideningConversion) { const Fixed10_6 narrow = Fixed10_6::fromInt(5);